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

Shared codecs package #153

Merged
merged 9 commits into from
May 24, 2023
Merged

Shared codecs package #153

merged 9 commits into from
May 24, 2023

Conversation

PaulLeCam
Copy link
Contributor

@PaulLeCam PaulLeCam commented May 11, 2023

In line with starting to use codecs in js-ceramic, I think it would be useful to have a codecs package for the DIDs structures, that could also be used in the js-ceramic codecs package considering the overlaps for some types.

Opening as draft to start the discussion, but will need more tests and docs.

@ukstv it seems there is a conflict with using both type and namespace for the Cacao export, any idea how to address this please? If not, I think I'll rename the type to CacaoType to avoid the conflict, what do you think please?

@PaulLeCam PaulLeCam marked this pull request as ready for review May 12, 2023 15:54
@PaulLeCam
Copy link
Contributor Author

@zachferland if it looks OK to you do you mind if I publish the new package once the PR is merged please?
I'd like to use it in the new @ceramicnetwork/codecs package to avoid duplicating the codecs, cc @ukstv

Copy link
Contributor

@zachferland zachferland left a comment

Choose a reason for hiding this comment

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

is this going to change the return types for all these functions? making it a breaking change

overall really nice have shared/defined codecs/types

/**
* Passthrough codeco codec for CID.
*/
export const cid = new Type<CID, CID, unknown>(
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to have the ipld ones here? didnt see where they were used

Copy link
Contributor

Choose a reason for hiding this comment

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

oh saw what you said before, since js-ceramic depends on dids dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -40,13 +40,13 @@ ___

### cacao

• `get` **cacao**(): `Cacao`
• `get` **cacao**(): `MapIn`<`RequiredProps`<{ `h`: `ExactCodec`<`TypeCodec`<{ `t`: `KeyOfCodec`<{ `caip122`: ``null`` ; `eip4361`: ``null`` }\> }\>\> ; `p`: `SparseCodec`<{ `aud`: `TrivialCodec`<`string`\> ; `domain`: `TrivialCodec`<`string`\> ; `exp`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `iat`: `TrivialCodec`<`string`\> ; `iss`: `TrivialCodec`<`string`\> ; `nbf`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `nonce`: `TrivialCodec`<`string`\> ; `requestId`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `resources`: `OptionalCodec`<`Codec`<`string`[], `string`[], `unknown`\> & { `item`: `TrivialCodec`<`string`\> }\> ; `statement`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `version`: `TrivialCodec`<`string`\> }\> ; `s`: `OptionalCodec`<`ExactCodec`<`TypeCodec`<{ `s`: `TrivialCodec`<`string`\> ; `t`: `KeyOfCodec`<{ `eip1271`: ``null`` ; `eip191`: ``null`` ; `solana:ed25519`: ``null`` ; `stacks:secp256k1`: ``null`` ; `tezos:ed25519`: ``null`` }\> }\>\>\> }\>, `$TypeOf`\> & `MapIn`<`OptionalProps`<{ `h`: `ExactCodec`<`TypeCodec`<{ `t`: `KeyOfCodec`<{ `caip122`: ``null`` ; `eip4361`: ``null`` }\> }\>\> ; `p`: `SparseCodec`<{ `aud`: `TrivialCodec`<`string`\> ; `domain`: `TrivialCodec`<`string`\> ; `exp`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `iat`: `TrivialCodec`<`string`\> ; `iss`: `TrivialCodec`<`string`\> ; `nbf`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `nonce`: `TrivialCodec`<`string`\> ; `requestId`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `resources`: `OptionalCodec`<`Codec`<`string`[], `string`[], `unknown`\> & { `item`: `TrivialCodec`<`string`\> }\> ; `statement`: `OptionalCodec`<`TrivialCodec`<`string`\>\> ; `version`: `TrivialCodec`<`string`\> }\> ; `s`: `OptionalCodec`<`ExactCodec`<`TypeCodec`<{ `s`: `TrivialCodec`<`string`\> ; `t`: `KeyOfCodec`<{ `eip1271`: ``null`` ; `eip191`: ``null`` ; `solana:ed25519`: ``null`` ; `stacks:secp256k1`: ``null`` ; `tezos:ed25519`: ``null`` }\> }\>\>\> }\>, `$TypeOf`\>
Copy link
Contributor

Choose a reason for hiding this comment

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

is there no nicer way to show the types in the docs? seems like it decreases readability across a lot of the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree it makes the generated type definitions ugly, @ukstv any idea if that could be worked around when using codeco's TypeOf please? Maybe using something like type-fest's Simplify?

import { refinement, string } from 'codeco'
import type { Opaque } from 'ts-essentials'

export type DIDString = Opaque<string, 'DIDString'>
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like later we could add stricter codecs for each did:key/pkh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can always add stricter codecs as needed, would be nice indeed to check for actually supported DID methods.

@PaulLeCam
Copy link
Contributor Author

Good question about the return types, I'm not sure what the behavior will be so should be safer to use major versions increases.

@zachferland
Copy link
Contributor

thanks for adding changesets! want to update the release section here? https://github.com/ceramicnetwork/js-did/blob/main/DEVELOPMENT.md

@PaulLeCam
Copy link
Contributor Author

thanks for adding changesets! want to update the release section here? https://github.com/ceramicnetwork/js-did/blob/main/DEVELOPMENT.md

Yes good call, I will do it on Monday!

# Conflicts:
#	.changeset/config.json
#	packages/cacao/src/siwx/siwx.ts
#	pnpm-lock.yaml
@PaulLeCam
Copy link
Contributor Author

@zachferland I updated the instructions, can you have a look please?

Copy link
Contributor

@zachferland zachferland left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@PaulLeCam PaulLeCam merged commit a40c824 into main May 24, 2023
13 checks passed
@PaulLeCam PaulLeCam deleted the feat/codecs branch May 24, 2023 07:32
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