Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unusable API #21

Open
Stebalien opened this issue May 25, 2021 · 2 comments
Open

Unusable API #21

Stebalien opened this issue May 25, 2021 · 2 comments

Comments

@Stebalien
Copy link

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
Copy link
Contributor

mvdan commented May 25, 2021

I fully agree - see #17. The API in this ADL is by no means done, and there's more work to do here before it is useful.

@Stebalien
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants