make hamt a go-ipld-prime ipld.Node #70
Conversation
|
Thanks for showing what this would look like. My first reaction here is that it's a lot of noise with little immediate value to the core use case of actors. Since the Filecoin actors code is so security-critical, and with very strict compatibility requirements, our general desire is to minimize any unnecessary code and dependencies. I didn't see a This HAMT is, unfortunately, not built with I infer that you wanted this compatibility for some downstream processing. Would it work to locally extend the HAMT struct, instead of modifying it here? |
|
This does not need to depend on |
|
longer term, i'll transition that to the in-progress ipld-schema'd hamt, but this works as a branch for ipld tools to use in the short term. I don't think it needs to be merged, but it's useful for me to maintain it as a branch on this repo so that i can pull in the v3 changes as they finalize. |
| @@ -6,11 +6,10 @@ require ( | |||
| github.com/ipfs/go-cid v0.0.6 | |||
| github.com/ipfs/go-ipld-cbor v0.0.4 | |||
| github.com/ipfs/go-ipld-format v0.0.2 // indirect | |||
| github.com/kr/pretty v0.1.0 // indirect | |||
| github.com/ipld/go-ipld-prime v0.12.3 | |||
There was a problem hiding this comment.
are you staying on an older go-ipld-prime on purpose?
| } | ||
|
|
||
| func loadPointer(p ipld.Node) (*Pointer, error) { | ||
| if p.Kind() != ipld.Kind_Link { |
|
|
||
| func loadKV(n ipld.Node) (*KV, error) { | ||
| if n.Kind() != ipld.Kind_List { | ||
| return nil, fmt.Errorf("kv should be of kind list") |
| } | ||
| return &Pointer{KVs: kvs}, nil | ||
| } | ||
| return nil, fmt.Errorf("unsupported pointer kind") |
| return nil, e | ||
| } | ||
| return n.LookupByString(s) | ||
| } else if key.Kind() == ipld.Kind_Bytes { |
There was a problem hiding this comment.
ditto switch and actual kind in the error
| } | ||
|
|
||
| func (n *Node) IsNull() bool { | ||
| return n.Bitfield == nil |
There was a problem hiding this comment.
Beware that "is null" should only ever return true if the node is nullable, e.g. a nullable struct field, which I imagine is not the case here. It's not a generic "is empty/zero" that can be useful for any node.
|
|
||
| // LSBlockstore creates a blockstore (get/put) interface over a link system | ||
| type LSBlockstore struct { | ||
| *ipld.LinkSystem |
There was a problem hiding this comment.
nit: embedding this means you're also exposing its methods on your API, so perhaps you want to make it a named and unexported field instead
|
2022-04-05 conversation: the potential areas of concern that we need in review:
|
|
This is a complementary code path and will not affect any current use cases. |
|
@willscott: 2022-04-19 conversation notes:
This is a good change, but it does increase the amount that someone needs to understand to properly maintain this library. |
ipld.NodePrototypeis a bit janky. Would be interested in suggestions on how to make that cleaner.NodeBuilder/NodeAssembler, so won't automatically load.context.Backgroundis used. Ideally, a context on the node would allow for graceful cancellation of in-progress operations.