feat(dagjose): add encoder and decoder methods #13

smrz2001 · 2021-09-22T19:12:39Z

fixes #4

alexjg · 2021-09-22T19:18:36Z

Do the round trip tests cover some code path which wasn't being exercised by the property based round trip testing in dagjose_test.go? If so it might be worth documenting that and maybe seeing if the property tests could be extended to cover the missing code paths?

smrz2001 · 2021-09-22T20:21:38Z

Do the round trip tests cover some code path which wasn't being exercised by the property based round trip testing in dagjose_test.go? If so it might be worth documenting that and maybe seeing if the property tests could be extended to cover the missing code paths?

Apologies @alexjg, I should have put this up as a draft PR. I'm still working some things out before it's ready. The round-trip tests are "borrowed" and I need to clean them up.

I'll keep your comment in mind as I do.

smrz2001 · 2021-10-07T23:29:44Z

@alexjg @warpfork I'm adding you as reviewers even though the PR isn't quite ready yet.

I think I have the adapter code between Alex's version and go-ipld-prime in place (with Eric's help) but I'm still working through a few issues.

Feel free to look at and comment on the code but please note that there will be more changes coming soon.

I will definitely be adding rainy day tests for encoding of invalid DAG-JOSE objects.

dagjose/dagjose_assembler.go

go.mod

dagjose/dagjose_assembler.go

dagjose/dagjose_test.go

decentralgabe · 2021-10-09T00:18:21Z

dagjose/jose_node.go

@@ -39,7 +39,7 @@ func (d dagJOSENode) LookupByString(key string) (ipld.Node, error) {
 	if key == "recipients" {
 		if d.recipients != nil {
 			return fluent.MustBuildList(


this is quite hard to read, I know you didn't do this either 😄

As the person who did do this I'm sorry you find it hard to read 🙁 . Any chance you would be able to articulate why you find it hard to read and what alternative you would find easier to read?

dagjose/json_serialization.go

dagjose/multicodec.go

decentralgabe

overall looks good, mostly nits. the panic is what we should definitely remove.

dagjose/multicodec.go

dagjose/jws_signature_assembler.go

Geo25rey

LGTM

oed · 2021-10-14T07:58:05Z

Do we have tests that could run in CI for this repo?

Geo25rey · 2021-10-14T11:19:52Z

@oed go test ./dagjose from the root of the repo

dagjose/dagjose_assembler.go

warpfork · 2021-10-20T09:09:03Z

dagjose/multicodec.go

+func Decode(na datamodel.NodeAssembler, r io.Reader) error {
+	return cbor.Decode(na, r)
+}


To match the strictness that Encode has, by leaning on the enforcements done in the NodeAssembler implementation in this package, this should probably do one of:

ignore the caller's na parameter and just use dagjose.NewBuilder regardless;

return an error if the caller's na parameter isn't *dagJOSENodeBuilder;

check if na is *dagJOSENodeBuilder and go straight into it if possible; otherwise make one and use it, and then do Copy into the caller's na at the end.

Option 3 is probably the most consistent with how Encode is currently written, and also throws the least flak at the caller, so seems likely to be best.

Excellent, thank you, this is very useful. Will have changes up today.

Done, please check.

warpfork · 2021-10-20T09:12:32Z

dagjose/multicodec.go

+
+// Encode walks the given datamodel.Node and serializes it to the given
+// io.Writer. Encode fits the codec.Encoder function interface.
+func Encode(n datamodel.Node, w io.Writer) error {


I'd recommend adding a fastpath through here if the n given by the caller is already using the concrete type from this package. Just a quick simple type assertion should do.

If the node given is already in this package's node implementation, then that data's already been validated, and we don't need to do that again, nor allocation new memory and be copying stuff. Should save a fair amount of time.

Sweet, added.

warpfork · 2021-10-27T17:06:04Z

dagjose/multicodec.go

+	// CBOR is a superset of DAG-JOSE and can be used to decode valid DAG-JOSE
+	// objects without decoding the CID (as expected by the DAG-JOSE spec:
+	// https://specs.ipld.io/block-layer/codecs/dag-jose.html).
+	if reflect.TypeOf(na) == reflect.TypeOf(new(dagJOSENodeBuilder)) {


You don't need reflect to do this. :) Just specificType, castWasOk := na.(*dagJOSENodeBuilder); castWasOk should do here.

smrz2001 · 2021-11-13T13:39:53Z

Closing in favor of new PR.

fix(pkg): minor updates

c403bc1

smrz2001 self-assigned this Sep 22, 2021

smrz2001 marked this pull request as draft September 22, 2021 20:24

smrz2001 force-pushed the feat/plugin-dagjose branch from 40d11b0 to fe0aeef Compare October 7, 2021 22:11

feat(dagjose): go-ipfs plugin integration updates

58b8c0d

smrz2001 force-pushed the feat/plugin-dagjose branch from c0717ca to 58b8c0d Compare October 7, 2021 22:32

smrz2001 requested a review from oed October 7, 2021 23:25

smrz2001 requested a review from alexjg October 7, 2021 23:29

oed assigned decentralgabe and unassigned decentralgabe Oct 8, 2021

oed requested a review from decentralgabe October 8, 2021 07:22

alexjg reviewed Oct 8, 2021

View reviewed changes

dagjose/dagjose_assembler.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

review updates and minor cleanup

f679812

decentralgabe reviewed Oct 9, 2021

View reviewed changes

dagjose/dagjose_assembler.go Outdated Show resolved Hide resolved

decentralgabe reviewed Oct 9, 2021

View reviewed changes

dagjose/dagjose_test.go Show resolved Hide resolved

decentralgabe reviewed Oct 9, 2021

View reviewed changes

dagjose/json_serialization.go Outdated Show resolved Hide resolved

decentralgabe reviewed Oct 9, 2021

View reviewed changes

dagjose/multicodec.go Outdated Show resolved Hide resolved

decentralgabe approved these changes Oct 9, 2021

View reviewed changes

Geo25rey reviewed Oct 9, 2021

View reviewed changes

dagjose/multicodec.go Outdated Show resolved Hide resolved

dagjose/jws_signature_assembler.go Show resolved Hide resolved

Geo25rey and others added 3 commits October 13, 2021 16:23

feat(dagjose): fix cbor encoding (#15)

d1f4425

review updates and minor cleanup

9b36168

more cleanup

ca0cc34

Geo25rey approved these changes Oct 14, 2021

View reviewed changes

more cleanup

afca06d

warpfork reviewed Oct 20, 2021

View reviewed changes

dagjose/dagjose_assembler.go Outdated Show resolved Hide resolved

warpfork reviewed Oct 20, 2021

View reviewed changes

smrz2001 requested a review from v-stickykeys October 21, 2021 18:06

smrz2001 added 9 commits October 21, 2021 16:37

revert formatting changes

9eb7584

revert formatting changes

3a71121

revert formatting changes

7def0d0

revert formatting changes

6c02ab3

revert formatting changes

b0f7472

revert formatting changes

5aaefc2

revert formatting changes

de5f0f4

revert formatting changes

28b35d7

revert formatting changes

fdfa0dc

smrz2001 marked this pull request as ready for review October 22, 2021 00:26

smrz2001 added 3 commits October 21, 2021 20:39

revert formatting changes

4f52339

Add fast-path to Encoding

33dfe9d

Update Decode to match the strictness of Encode

2da4291

warpfork reviewed Oct 27, 2021

View reviewed changes

decentralgabe added the open mainnet label Nov 3, 2021

smrz2001 mentioned this pull request Nov 13, 2021

Support dag-jose codec by default ipfs/kubo#8364

Closed

3 tasks

smrz2001 closed this Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dagjose): add encoder and decoder methods #13

feat(dagjose): add encoder and decoder methods #13

smrz2001 commented Sep 22, 2021

alexjg commented Sep 22, 2021

smrz2001 commented Sep 22, 2021 •

edited

Loading

smrz2001 commented Oct 7, 2021

decentralgabe Oct 9, 2021

alexjg Oct 9, 2021

decentralgabe left a comment

Geo25rey left a comment

oed commented Oct 14, 2021

Geo25rey commented Oct 14, 2021

warpfork Oct 20, 2021 •

edited

Loading

smrz2001 Oct 20, 2021

smrz2001 Oct 25, 2021

warpfork Oct 20, 2021

smrz2001 Oct 25, 2021

warpfork Oct 27, 2021

smrz2001 commented Nov 13, 2021

feat(dagjose): add encoder and decoder methods #13

feat(dagjose): add encoder and decoder methods #13

Conversation

smrz2001 commented Sep 22, 2021

alexjg commented Sep 22, 2021

smrz2001 commented Sep 22, 2021 • edited Loading

smrz2001 commented Oct 7, 2021

decentralgabe Oct 9, 2021

Choose a reason for hiding this comment

alexjg Oct 9, 2021

Choose a reason for hiding this comment

decentralgabe left a comment

Choose a reason for hiding this comment

Geo25rey left a comment

Choose a reason for hiding this comment

oed commented Oct 14, 2021

Geo25rey commented Oct 14, 2021

warpfork Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

smrz2001 Oct 20, 2021

Choose a reason for hiding this comment

smrz2001 Oct 25, 2021

Choose a reason for hiding this comment

warpfork Oct 20, 2021

Choose a reason for hiding this comment

smrz2001 Oct 25, 2021

Choose a reason for hiding this comment

warpfork Oct 27, 2021

Choose a reason for hiding this comment

smrz2001 commented Nov 13, 2021

smrz2001 commented Sep 22, 2021 •

edited

Loading

warpfork Oct 20, 2021 •

edited

Loading