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

Update multiformats, deps, examples, docs #16

Merged
merged 7 commits into from Jun 3, 2021

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented May 15, 2021

First .. this is very heavy-handed of me so please tell me off where I'm stepping over bounds of style etc.! I'm happy to scale back, or have you take my commits and scale them back, if you have strong opinions about some of the things I've adjusted in the process of this PR.

I spent a bit of time familiarising myself with the workflow here and how it's all supposed to work. In the process I fleshed out the examples a bit more and got the signing example from the README fully working (it seemed to be partial). The signing work also prompted ceramicnetwork/js-dag-jose-utils#1 to get some minor utilities in place. I'm not an expert in JOSE so you may have to correct me if I'm getting the workflow wrong!

Examples are now included as separate files, separate ones for encryption with and without IPFS - so demonstrating best practice now for bridging into the js-IPFS world from the multiformats world, but also demonstrating stand-alone IPLD usage without needing the js-IPFS pieces. A separate example for signing seemed appropriate since it's such a different workflow.

Dependencies are trimmed down quite a bit thanks to the affordances of the new stack.

I've copied the current multiformats codec pattern: https://github.com/multiformats/js-multiformats/blob/1b133a87f8c85bb8f1f088ca78821d87a3a0988a/src/codecs/interface.ts#L21, where we've removed default exports for codecs (we've basically ditched default exports across the stack, it's too hard to support across different bundlers and resolvers). So it exports {name, code, encode, decode} now but also exports a prepare function. This same pattern is used in https://github.com/ipld/js-dag-pb where a similar thing is happening—input objects are being transformed such that a round-trip leaves you with a different object, and the forms are very strict on input, which is all unlike dag-cbor and dag-json which are flexible. So instead, prepare() can optionally get you to the same form of object that you would have if you were to round-trip through an encode and decode (optionally because I didn't want to break the pattern of letting encode() still accept the string form). Also in support of this round-trippishness I added link onto the prepare()d form of DagJWS, so it should just always be there.

Custom Jest resolver is pending fix being tracked @ jestjs/jest#9771

Jest "testEnvironment": "node" is set pending jsdom getting proper TextEncoder and TextDecoder support like the rest of the JS world, tracked @ jestjs/jest#9983

@oed oed self-requested a review May 15, 2021 18:34
Copy link
Member

@oed oed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really awesome! Thanks for doing a deep dive and creating all of these examples 🙏
Left a few comments, but nothing major.

You may also be interesting in having a look at js-did to get a sense of how we use this codec together with the DID standard.

Ping @ukstv, you might be curious about this code base :)

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example-ipfs.mjs Outdated Show resolved Hide resolved
ES256KSigner,
createJWS,
verifyJWS
} from 'did-jwt'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been discussing renaming this library to did-jose since it does much more than just JWTs :)

src/encryption.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
test/index.test.ts Outdated Show resolved Hide resolved
@rvagg
Copy link
Contributor Author

rvagg commented May 17, 2021

started having a look at the comments but have run out of time, will get back to this asap

@rvagg
Copy link
Contributor Author

rvagg commented May 26, 2021

@oed I've renamed prepare() to toGeneral(), commented it out in the example but documented it there. It's really useful to have a way to ensure you have the same form of object that you'd get through a round-trip so I think this is a useful export. Your call though. I was thinking about just doing a toCompact() to provide the two-way option and then maybe toJWSStrings() in dag-jose-utils could be removed, but I'm not sure at all how to handle the multi-sig option. That doesn't seem to be handled here at all. fromSplit() only handles a single signature.

So I'm wondering if:

  1. toGeneral() should also accept a string[] with fromSplit() - in this case I'm not sure whether encryption.ts/fromSplit() has an array form for DagJWE (if so, what does it do?)
  2. add toCompact() which is essentially the same as toJWSStrings() .. and also handles the DagJWE case (again, not sure if there's an array form of this, although I see .recipients is an array so maybe that's the multiple form?).

Thoughts?

@oed
Copy link
Member

oed commented May 31, 2021

I was thinking about just doing a toCompact() to provide the two-way option

The compact encoding only supports a single signer/recipient, so it would be weird to return multiple compact encodings. Ideally the did-jwt library should support the general encoding.

  1. No, as I think having an array of strings doesn't make much sense from the perspective of the JOSE spec.
  2. I would not add a toCompact() function!

I think most of these issues stem from the did-jwt library not yet supporting the general encoding. If it did we wouldn't need any of these additional conversions. Rather fix things there than to add more confusing conversion functions here.

@rvagg
Copy link
Contributor Author

rvagg commented Jun 3, 2021

The compact encoding only supports a single signer/recipient

ok, that simplifies things then. So this has a toGeneral() which I guess covers the basics. IMO this is good to go, I've just updated dependencies again so we're up to date.

Copy link
Member

@oed oed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting all of this together @rvagg!

@oed oed merged commit 51750b4 into ceramicnetwork:master Jun 3, 2021
@oed
Copy link
Member

oed commented Jun 3, 2021

Published as dag-jose@1.0.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

2 participants