ipld / go-ipld-adl-hamt

Other
4 stars 4 forks source link

Unusable API #21

Open Stebalien opened 3 years ago

Stebalien commented 3 years ago

With https://github.com/filecoin-project/go-hamt-ipld/, I can easily:

  1. Load a HAMT.
  2. Put/delete a few values.
  3. Flush the hamt.

This is how a map should work.

With this library, I'm not even sure how I can load a HAMT (no tests cover opening an existing HAMT). After that, it looks like I need to build key/values and perform a bunch of magical type-assertions to actually do anything. There's no simple Put/Get/Delete API that I'd expect on any reasonable map type.

I appreciate having a low-level API for hyper-efficient auto-generated code, but there needs to be an API that's usable by humans. One that doesn't require writing 20 lines of code where 5 should suffice and one that doesn't include any type assertions. I mean, this is the "basic" Filecoin test:

buf := new(bytes.Buffer)

fenc, err := hex.DecodeString("82412081818243666f6f43626172")
qt.Assert(t, err, qt.IsNil)

builder := hamt.FilecoinV3Prototype{}.NewBuilder()
assembler, err := builder.BeginMap(0)
qt.Assert(t, err, qt.IsNil)

qt.Assert(t, assembler.AssembleKey().AssignString("foo"), qt.IsNil)
qt.Assert(t, assembler.AssembleValue().AssignBytes([]byte("bar")), qt.IsNil)
qt.Assert(t, assembler.Finish(), qt.IsNil)

// TODO: can we do better than these type asserts?
node := builder.Build().(*hamt.Node)
nodeRepr := node.Substrate().(hamt.HashMapNode).Representation()
buf.Reset()
qt.Assert(t, dagcbor.Encoder(nodeRepr, buf), qt.IsNil)
enc := buf.Bytes()
t.Logf("go-ipld-adl-hamt: %x", fenc)

qt.Assert(t, enc, qt.DeepEquals, fenc)
mvdan commented 3 years ago

I fully agree - see https://github.com/ipld/go-ipld-adl-hamt/issues/17. The API in this ADL is by no means done, and there's more work to do here before it is useful.

Stebalien commented 3 years ago

Ok, that makes me much happier. But I'm also including things like:

node := builder.Build().(*hamt.Node)
nodeRepr := node.Substrate().(hamt.HashMapNode).Representation()

That kind of code is completely opaque.

Feel free to close this issue if you consider this covered.