fix(did,keys): emit a real Multikey for publicKeyMultibase - #166
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: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesMultikey public key encoding
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR corrects DID verification-method identifiers so Multikey values include the curve identifier and remain consistent with did:key output; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files. (1 skipped: 1 unsupported.) ✨ 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 |
| * @param curve - The curve the key belongs to | ||
| * @returns The Multikey string | ||
| */ | ||
| export function publicKeyToMultikey( |
There was a problem hiding this comment.
It looks like Ed25519 input is not validated in this method but EC input is. We could likely call isValidPublicKey(publicKey, curve) at the top w/ errors thrown for each curve
There was a problem hiding this comment.
Good catch, fixed sir
…ikey The secp256k1 and secp256r1 paths rejected bad input only as a side effect of decoding the point to compress it, and Ed25519 has no compression step, so any 32 bytes — and in fact any length — took the `0xed` prefix and came back as a well-formed Multikey. Check `isValidPublicKey(publicKey, curve)` up front instead, so every curve fails the same way and for the stated reason.
2 participants
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.