fix(did,keys): emit a real Multikey for publicKeyMultibase - #166
Open
Dusk1e wants to merge 1 commit into
Open
Conversation
`createDidDocument` builds verification methods with `type: "Multikey"`, but `publicKeyMultibase` held a multibase encoding of the raw public key bytes. A Multikey is `multibase(base58-btc, varint(multicodec) ‖ key)`, so the value carried no algorithm identifier and a relying party could not tell one curve's key from another's. `createDidKeyUri` already built the prefixed form, so the same key came out two different ways depending on which function produced it. Encode the multicodec code for the curve and, for secp256k1 and secp256r1, the 33-byte compressed point that code identifies. The `hex` and `base58` inputs route through the same encoder, since they are converted to a Multikey verification method too. `publicKeyMultibase` now equals the `did:key` method-specific identifier for the same key, which is asserted directly against `createDidKeyUri`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. Walkthrough
ChangesMultikey public key encoding
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change corrects Multikey encoding and includes focused coverage for the affected key and DID paths; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #165.
createDidDocumentbuilds verification methods withtype: "Multikey", but thepublicKeyMultibaseon them was a multibase encoding of the raw public key bytes. A Multikey ismultibase(base58-btc, varint(multicodec code) ‖ key-bytes), so the value carried no algorithm identifier — a relying party reading it as a Multikey cannot tell an Ed25519 key from a P-256 one.createDidKeyUrialready built the prefixed form, so the same key came out two different ways depending on which function produced it.Using the two keypairs fixtured in
packages/did/src/methods/did-key.test.ts:createDidKeyUri)z8myPWEuZj3T3WzBQu281AeLzE3pmU9h7em1YEGiD6ickz6MknEES6VA14awWdV27ab5r1jtz3d6ct2wULmvU4YgE1wQ8zMi3oP598f15BeBYp3tdwtNEE3omWSogmoT8Q5ckb1N5AKKJj59KU61vMYYncCBDzGDaDkn7zib2ACobFPXB8TjGTzQ3shNCcRrVT3tm43o6JNjSjQaiBXvSb8kHtFhoNGR8eimFZsChange
publicKeyToMultikey(publicKey, curve)in@agentcommercekit/keysprepends the curve's multicodec code as a varint and multibase-encodes the result.encodePublicKeyMultibaseuses it, andconvertLegacyPublicKeyToMultibasein@agentcommercekit/didrouteshexandbase58through the same encoder rather than callingbytesToMultibasedirectly — those two are converted to aMultikeyverification method as well, so they carried the same defect.The codes live in a new
keyCurveMulticodecsmap inkeys, next tokeyCurves. That is the placement decision I flagged in the issue:keysalready owns curve metadata, key generation and every other encoding, and it is the package the encoder lives in.KEY_CONFIGindid/methods/did-key.tsis untouched, so nothing aboutdid:keychanges — instead the new test asserts the two agree, which is what keeps them from drifting apart again.secp256k1 and secp256r1 keys are compressed to the 33-byte form their codes identify, via a new
compressPublicKeyon each curve module (Point.fromBytes(...).toBytes(true), the same@noble/curvesAPIisValidPublicKeyalready uses).generateKeypairstores these uncompressed, which is why the secp256k1 row above is so long. Ed25519 has a single 32-byte encoding and is passed through.Behaviour changes
publicKeyMultibaseemitted bycreateDidDocument,createDidDocumentFromKeypair,createDidWebDocumentandencodePublicKey("multibase", ...)changes for themultibase,hexandbase58encodings. The defaultjwkencoding is unaffected. An already-published document keeps whatever it was published with; re-generating it produces the corrected value. Markedminoron both packages.encodePublicKey("multibase", bytes, curve)now throws forsecp256k1/secp256r1whenbytesis not a point on that curve, because compressing it means decoding it. This surfaced indid-resolvers/did-resolver.test.ts, which passed a 20-byte Ethereum address as a secp256k1 public key; that fixture is now a real key fromdid-key.test.ts. The test exercises the resolver cache, so the key material is incidental to it. Happy to make the compression lenient instead if you would rather this path keep accepting arbitrary bytes, though that would put non-keys back intoMultikeyvalues.Tests
keys: the multibase encoding decodes to the curve's multicodec code followed by a key of the expected length (32 for Ed25519, 33 for the EC curves).did: for every curve,publicKeyMultibaseequals thedid:keyidentifier for the same key, across themultibase,hexandbase58encodings — asserted againstcreateDidKeyUri, which is independent of the code being changed.create-did-document.test.ts,methods/did-web.test.ts) now build theirs withpublicKeyToMultikey.Both new tests fail on
main.keys(102),did(73),vc(109),ack-id(39) andack-pay(34) pass;oxlintandoxfmt --checkare clean on the changed files.One note on my local run:
packages/did/src/did-resolvers/pkh-did-resolver.test.tscannot run on Windows because thedid-pkhfixture filenames contain:, which is #145. Unrelated to this change and left alone.AI assistance disclosure
Per the repository AI policy: this contribution was AI-assisted using Claude Code (Claude Opus). AI assistance was used to find the inconsistency, write the change and the tests, and run verification locally. I reviewed the final diff, can explain what it does and where it changes behaviour, and take responsibility for what is submitted here.
Summary by CodeRabbit
did:keyidentifiers.