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

Use CAIP-10 in did:pkh #286

Merged
merged 2 commits into from Sep 23, 2021
Merged

Use CAIP-10 in did:pkh #286

merged 2 commits into from Sep 23, 2021

Conversation

clehner
Copy link
Contributor

@clehner clehner commented Sep 9, 2021

This implements CAIP-10 blockchain account identifiers as method-specific ids in did-pkh, as proposed in #279 (#238).

Support for the existing non-CAIP-10 prefixes is kept, although test vectors are removed for some of them.

Fortunately, there do not appear to be any conflicts between the namespace of CAIP-2 chain ids registered in CAIPs and the non-CAIP-10 did:pkh: prefixes used here.

Non-CAIP-10 did:pkh usages are marked as deprecated in some comments, but no warnings are yet produced when resolving or otherwise using them.

DID document test vector files used in this did:pkh implementation are referenced in the did:pkh specification draft in this repo. These test vectors are updated to use the new CAIP-10 did:pkh format, for consistency with #279.

Existing deployments of did:pkh:eth and did:pkh:tz are mentioned in #279 (comment).
To ensure compatibility with existing deployments, for now, some of the DID document test vectors for non-CAIP-10 did:pkh DIDs are kept in this repo and under test, but under new file names (since the URLs correponding to their previous/existing names are being updated instead, and remain referencable in #279). There are also other tests in place for all the existing non-CAIP-10 did:pkh prefixes, including issued verifiable credentials, which have not yet been generated.

@clehner clehner linked an issue Sep 9, 2021 that may be closed by this pull request
@clehner clehner marked this pull request as ready for review September 9, 2021 21:05
@clehner clehner requested review from sbihel and wyc September 10, 2021 15:40
@clehner
Copy link
Contributor Author

clehner commented Sep 10, 2021

This will need updates since #283 was merged.

@clehner
Copy link
Contributor Author

clehner commented Sep 14, 2021

Fixed merge conflicts with #282 and #283

@clehner clehner marked this pull request as draft September 16, 2021 11:02
@clehner clehner marked this pull request as ready for review September 16, 2021 12:11
@clehner clehner requested a review from sbihel September 16, 2021 12:11
@clehner
Copy link
Contributor Author

clehner commented Sep 22, 2021

This PR is now updated to incorporate the specification changes from #279 and #303. This is for consistent updating of the specification, test vectors, and implementation. Test vector file names are updated to follow #303. All the old non-CAIP-10 DID documents are now kept in testing.

Comparison with the commit previously reviewed by @sbihel (26cd7c5), after merging with main, is here: https://github.com/spruceid/ssi/compare/1f32503507ccd446d0caa517bbcbc1efc20292b0..110732299262a09f614b1fcf349ffb6f89012ee8

cc @bumblefudge @oed @wyc

|`eth`|`eip155:1`|EcdsaSecp256k1RecoveryMethod2020|https://identity.foundation/EcdsaSecp256k1RecoverySignature2020#EcdsaSecp256k1RecoveryMethod2020|
|`celo`|`eip155:42220`|EcdsaSecp256k1RecoveryMethod2020|https://identity.foundation/EcdsaSecp256k1RecoverySignature2020#EcdsaSecp256k1RecoveryMethod2020|
|`poly`|`eip155:137`|EcdsaSecp256k1RecoveryMethod2020|https://identity.foundation/EcdsaSecp256k1RecoverySignature2020#EcdsaSecp256k1RecoveryMethod2020|
|`sol`|`solana`|Ed25519VerificationKey2018|https://w3id.org/security#Ed25519VerificationKey2018|
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say
solana:4sGjMW1sUnHzSxGspuhpqLDx6wiyjNtZ
for consistency? 🔍

(and good catch, btw, I didn't think to scour the CAIP repo for the mainnet chain_id for solana)

Copy link
Contributor Author

@clehner clehner Sep 23, 2021

Choose a reason for hiding this comment

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

clehner and others added 2 commits September 23, 2021 09:27
- Use CAIP-10 strings as method-specific id.
- Allow previous non-CAIP-10 strings as legacy behavior.
- Update description of default chain behavior.
- Update test vectors, and rename the previous ones.

Co-authored-by: Bumblefudge <37127325+bumblefudge@users.noreply.github.com>
Co-authored-by: Joel Thorstensson <oed@3box.io>
Conform to updated specification draft.
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.

Ethereum PKH verification method id Suggestion: Make did:pkh completely based on new caip10 spec
4 participants