-
Notifications
You must be signed in to change notification settings - Fork 38
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
Use ipld-prime for encode & decode in DAG Service, construct ipld-prime compatible nodes #64
Conversation
8d8a67c
to
ef7bdb0
Compare
coding.go
Outdated
format "github.com/ipfs/go-ipld-format" | ||
pb "github.com/ipfs/go-merkledag/pb" | ||
dagpb "github.com/ipld/go-ipld-prime-proto" | ||
"github.com/ipld/go-ipld-prime/fluent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warpfork - is fluent the right dialect of prime to be using here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it is -- fluent is basically the way to do a bunch of stuff without an incredibly verbose syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also try either quip
or qp
dialects, if you like -- they're questing for good syntax but both should have a lower performance overhead than the fluent
package does.
quip
and qp
are more recently introduced than fluent
, and someday we'll probably try to narrow down how many of these there are, but for now all three are considered valid (and battle-testing so we can get hands-on feedback about the ergonomics is highly welcome :)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was my question: should we prefer quip / qp vs fluent for this dev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AND I was unaware of quip/qp so now I need ot check those out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
coding.go
Outdated
nb := dagpb.Type.PBNode.NewBuilder() | ||
err := fluent.Recover(func() { | ||
fb := fluent.WrapAssembler(nb) | ||
fb.CreateMap(-1, func(fmb fluent.MapAssembler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we know there are 2 entries in the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's the case? I honestly copied this real quick from the go-ipld-prime-proto code and I can't remember why I used -1 here.
@@ -382,3 +387,17 @@ func (n *ProtoNode) Tree(p string, depth int) []string { | |||
} | |||
return out | |||
} | |||
|
|||
func ProtoNodeConverter(b blocks.Block, nd ipld.Node) (legacy.UniversalNode, error) { | |||
pbNode, ok := nd.(dagpb.PBNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a todo to add here to do codec stuff if it isn't dagpb, rather than failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other codecs should "just work" based on the design of go-ipld-prime. Protobuf & Raw are special cases.
// | ||
// If idx is out of range, a nil node and an error will be returned. | ||
func (n *ProtoNode) LookupByIndex(idx int64) (ipld.Node, error) { | ||
_, err := n.EncodeProtobuf(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully this is cached? is it preferable to have a n.GetEncoded
that provides the pointer to the internal node and computes it if it doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's actually what the somewhat inaccurately named EncodeProtobuf actually does -- the boolean paramenter, assuming it's false, means don't re-encode unless the data has changed.
Fix functionality for decode function -- was using incorrect decoder and not handling maybe cases
superseded by #67 |
This is a minimal shim to put IPLD prime into IPFS.
It means that: