ceramicnetwork / js-dag-jose

Other
46 stars 9 forks source link

Update multiformats, deps, examples, docs #16

Closed rvagg closed 3 years ago

rvagg commented 3 years ago

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 https://github.com/ceramicnetwork/js-dag-jose-utils/pull/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 @ https://github.com/facebook/jest/issues/9771

Jest "testEnvironment": "node" is set pending jsdom getting proper TextEncoder and TextDecoder support like the rest of the JS world, tracked @ https://github.com/facebook/jest/issues/9983

rvagg commented 3 years ago

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

rvagg commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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.

oed commented 3 years ago

Published as dag-jose@1.0.0 🎉