[HYPER-181] add cryptographic signature support to record lexicons (resubmission of #170)#219
Open
aspiers wants to merge 13 commits into
Open
[HYPER-181] add cryptographic signature support to record lexicons (resubmission of #170)#219aspiers wants to merge 13 commits into
aspiers wants to merge 13 commits into
Conversation
🦋 Changeset detectedLatest commit: 35493c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
More reviews will be available in 55 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds optional cryptographic signatures to all record lexicons in the lexicon repository. Two new signature lexicons define inline ECDSA signatures and remote attestation proofs; 21 existing records gain an optional ChangesCryptographic Signature Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ 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 |
aspiers
changed the title
Add optional cryptographic signature support to record lexicons, enabling platform attestation and verification that records were created through trusted services. Inspired by: - ATProtocol Attestation Specification by Nick Gerakines - The platformAttestation pattern from app.bumicerts.link.evm New lexicons: - app.certified.signature.inline: Inline signature with JOSE algorithm identifiers (ES256K, ES256, Ed25519) per ATProto convention - app.certified.signature.defs: Shared type definitions providing the `#list` array def (an open union of inline signatures and strongRefs to remote attestation proofs). Follows the atproto convention of putting reusable defs in a dedicated `*.defs` lexicon; see the lexicon spec section on "Lexicon Files" and the style guide's recommendation for `.defs` schemas. - app.certified.signature.proof: Remote attestation proof record holding the CID of attested content, hosted in the attestor's repository 19 record lexicons gain an optional `signatures` property (a flat array referencing app.certified.signature.defs#list) so callers access them as record.signatures[] with no wrapper nesting. This matches the ATProtocol Attestation Specification while keeping the array definition DRY via a single shared lexicon def. Lexicons updated: - org.hypercerts.claim.* (activity, contribution, contributorInformation, rights) - org.hypercerts.collection - org.hypercerts.context.* (acknowledgement, attachment, evaluation, measurement) - org.hypercerts.funding.receipt - org.hypercerts.workscope.tag - org.hyperboards.* (board, displayProfile) - app.certified.badge.* (award, definition, response) - app.certified.actor.* (profile, organization) - app.certified.location app.certified.link.evm is deliberately excluded: it already carries its own EIP-712 wallet-ownership proof in the `proof` field, which is the integrity mechanism for its semantic DID ↔ wallet claim. The EIP-712 signature is incompatible with the ATProto attestation spec (different preimage, hash function, signature format, and binding model), and adding a second signing mechanism on the same record would provide no additional trust while inviting confusion about which signature is authoritative. Includes validation tests for inline signatures, proof records, and signatures attached to records, plus regenerated SCHEMAS.md, ERD.puml, README.md, and a changeset.
…tion Tighten the signature lexicons added in this PR to conform strictly to the ATProtocol Attestation Specification, rather than carrying speculative extensions inherited from the original design draft. - Remove signatureType from app.certified.signature.inline. The spec has no such field; the signing curve is canonically derived from the multicodec prefix on the verification method's publicKeyMultibase. A separate tag was redundant in the happy path and a downgrade-attack vector in the adversarial path (could disagree with the multicodec). - Drop Ed25519 from the documented algorithm story. EdDSA isn't excluded by normative MUST/SHALL language in the spec but the spec's signing primitive is ECDSA + BIP-0062 low-S and the multicodec->curve table only covers 0xE701 (K-256) and 0x1200 (P-256). Ed25519-signed records would be schema-valid but unverifiable by spec-conforming verifiers. - Remove maxLength: 100 cap on app.certified.signature.defs#list. No rationale was ever recorded for the bound and the spec does not restrict array length. - Update lexicon descriptions to use spec terminology: 36-byte CIDv1, canonical DAG-CBOR, BIP-0062 low-S, \$sig repository binding. - Rewrite README signatures section to claim conformance explicitly, drop the now-stale JOSE algorithm-identifiers table, add a brief signing/verification procedure summary, and explain replay defense via CID + \$sig.repository. - Update changeset language from "inspired by" to "conforms to". - Drop signatureType usages from tests and remove the now-meaningless open-knownValues test case. - Regenerate SCHEMAS.md.
Align with the per-lexicon naming convention in AGENTS.md
("one file per lexicon, named validate-<lexicon-slug>.test.ts").
The tests cover the array def at app.certified.signature.defs#list
(open union of inline + strongRef, optional/empty/mixed array
semantics), so signature-defs is the lexicon under test. Activity
is just the representative carrier — all 19 record lexicons share
the same def.
Addresses CodeRabbit feedback on PR #170.
…g tests The "one file per lexicon" rule is a default, not a strict requirement. When a test exercises behavior that spans multiple lexicons or code files (shared *.defs lexicons, cross-cutting union semantics, etc.), pick the clearest name for what's under test rather than duplicating near-identical tests across every carrier. Cite validate-signature-defs.test.ts as an illustrative example. Surfaced by CodeRabbit feedback on PR #170.
…gnatures to all records Two coupled changes: 1. Move inline signature object from its own lexicon (app.certified.signature.inline) into signature.defs#inline. Aligns with the earlier signature.list -> signature.defs#list move: non-record helper defs belong in the *.defs lexicon, not as standalone lexicons. Matches ATProto convention (e.g. app.bsky.embed.defs, app.bsky.richtext.facet#mention). 2. Add the signatures property to graph.follow and link.evm, bringing all 21 record lexicons under the signature umbrella. On link.evm specifically: the original PR deliberately excluded this record on the rationale that its EIP-712 proof field already provides integrity. That conflated two orthogonal trust statements. The EIP-712 proof proves wallet consent (the EVM key holder agreed to be linked to the DID). The signatures array proves record provenance (e.g. that a platform UI minted the record, defending against replay of harvested EIP-712 signatures). Both are useful and they do not conflict, so signatures applies here too. Test changes: - Rename tests/validate-signature-inline.test.ts to validate-signature-defs-inline.test.ts (the lexicon under test is now signature.defs). - Switch to the generated validateInline() helper instead of the generic validate() + cast, removing an `as unknown` smell. - Update inline $type strings from app.certified.signature.inline to app.certified.signature.defs#inline throughout. Regenerate SCHEMAS.md.
The signatures property is uniform across all ~20 record lexicons, so drawing the relationships individually would just be a thicket of identical "signatures" edges without communicating any additional structure. Replace the partial activity-only depiction with a comment explaining the deliberate omission; signature lexicons remain documented in SCHEMAS.md and README.md.
Bring the agent-facing skill guide in line with the rest of the signature work on this PR. Adds: - Signatures subsection in Lexicon Overview covering signature.defs and signature.proof - Note in the EVM Link row that link.evm can additionally carry signatures[] for record provenance on top of EIP-712 wallet consent - Updated Relationship Map listing signature/defs and signature/proof, with a note explaining why the per-record signatures arrows aren't drawn - "Attaching Cryptographic Signatures" example showing mixed inline + remote-attestation usage, plus a short signing-procedure summary pointing at the ATProto Attestation Specification - "Creating a Remote Attestation Proof" example for the proof record
The Leaflet URL is a useful background blog post but not the normative specification document. Update all doc references so the primary link is the canonical spec at tangled.org and the blog post appears as a secondary "see also" link. Affects README, changeset, and the building-with-hypercerts-lexicons skill.
Previously the cryptographic signatures example presented a record with signatures already embedded, using a placeholder comment for the ECDSA bytes and explaining the signing procedure only afterward. That reads backwards: a developer wanting to attach a signature needs to see how the bytes are produced first. Rewrite both README and SKILL.md so the example walks step by step: 1. Build the inline signature object — compute the spec-defined CID over the record (insert temporary \$sig with repository DID, encode canonical DAG-CBOR, hash to 36-byte CIDv1) and ECDSA-sign with @atproto/crypto's Secp256k1Keypair.sign() which enforces low-S automatically. 2. Build a remote attestation strongRef (optional). 3. Attach either or both to the record via spread. Uses canonical ATProto + IPLD libraries (@atproto/crypto, @ipld/dag-cbor, multiformats) rather than placeholder pseudocode. multiformats is already a runtime dep of this package; the others are shown as consumer imports.
Defends against docs/test divergence by making the docs themselves the test source. The new test reads README.md and the building-with-hypercerts-lexicons skill, finds the "Cryptographic Signatures" section in each, extracts every fenced TypeScript code block under it, concatenates them into one ESM module (deduping imports and appending synthetic exports for the identifiers we want to inspect), writes the result to tests/extracted/ (gitignored), and dynamically imports. For each source file, asserts: - Extraction + execution succeeds without throwing - The produced CID is a 36-byte CIDv1 with the dag-cbor/SHA-256 prefix - Secp256k1Keypair.sign() returns 64 raw r,s bytes - The inline signature has the correct \$type and references the signature bytes - The remote-attestation strongRef has the correct \$type and at:// URI - Both signatures attach to the final record in order - The resulting record (with a syntactically valid CID substituted for the "bafy..." placeholder) passes lexicon validation - The produced signature verifies via @atproto/crypto verifySignature Adds @atproto/crypto and @ipld/dag-cbor to devDependencies (only used by this test; not exposed to package consumers).
Line 51 restated what line 13's #inline description already says about ECDSA + low-S + multicodec-derived curve. Addresses review comment on PR #170.
aspiers
force-pushed
the
feat/signature-support-v2
branch
from
426553f to
b40da9f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds opt-in cryptographic attestation support across the Hypercerts record lexicon surface area, plus documentation and tests to keep the signing procedure and schema shape from drifting.
Changes:
- Introduces new signature lexicons:
app.certified.signature.defs(shared#list/#inline) andapp.certified.signature.proof(remote proof record). - Extends all 21 record lexicons with an optional
signaturesfield referencingapp.certified.signature.defs#list. - Adds docs + tests, including an integration-style test that extracts the “Cryptographic Signatures” docs code blocks and executes them.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/validate-signing-example.test.ts | Extracts/executes the docs signing example and asserts CID/signature/validation behavior. |
| tests/validate-signature-proof.test.ts | Validates app.certified.signature.proof record shape and required fields. |
| tests/validate-signature-defs.test.ts | Validates app.certified.signature.defs#list union/array semantics via an activity carrier record. |
| tests/validate-signature-defs-inline.test.ts | Validates the app.certified.signature.defs#inline object def requirements. |
| README.md | Documents the new signature lexicons and adds a “Cryptographic Signatures” usage section. |
| SCHEMAS.md | Regenerated schema tables to include the new signatures property and new signature lexicons. |
| lexicons/app/certified/signature/defs.json | New shared defs lexicon for inline signatures and signature lists. |
| lexicons/app/certified/signature/proof.json | New proof record lexicon for remote attestations. |
| lexicons/org/hypercerts/claim/activity.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/claim/contribution.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/claim/contributorInformation.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/claim/rights.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/collection.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/context/acknowledgement.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/context/attachment.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/context/evaluation.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/context/measurement.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/funding/receipt.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hypercerts/workscope/tag.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hyperboards/board.json | Adds optional signatures ref to signature defs list. |
| lexicons/org/hyperboards/displayProfile.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/badge/award.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/badge/definition.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/badge/response.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/actor/profile.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/actor/organization.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/location.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/graph/follow.json | Adds optional signatures ref to signature defs list. |
| lexicons/app/certified/link/evm.json | Adds optional signatures ref to signature defs list (and clarifies its relationship to proof). |
| package.json | Adds dev deps for signing/docs test support and bumps viem. |
| package-lock.json | Locks new deps (@atproto/crypto, @ipld/dag-cbor, transitive updates). |
| .agents/skills/building-with-hypercerts-lexicons/SKILL.md | Adds signature lexicon docs and signing procedure example (used by extraction test). |
| .changeset/add-signature-support.md | Publishes a minor version changeset describing signature support across lexicons. |
| AGENTS.md | Updates testing guidance to allow cross-cutting tests that don’t map 1:1 to a lexicon file. |
| ERD.puml | Notes that signature relationships are intentionally omitted from the ERD to avoid clutter. |
| .gitignore | Ignores tests/extracted/ output directory created by the docs-extraction test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/validate-signature-defs-inline.test.ts (1)
1-41: ⚡ Quick winConsolidate shared defs tests into the canonical defs test file.
This standalone file adds shared
app.certified.signature.defscoverage that should live intests/validate-signature-defs.test.tsto keep one canonical defs test target and avoid drift between parallel suites.Based on learnings: For this repository’s tests, when validating a shared
defslexicon, keep a singlevalidate-<lexicon-slug>.test.tsfile with a representative carrier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/validate-signature-defs-inline.test.ts` around lines 1 - 41, This file duplicates shared tests for app.certified.signature.defs; remove these tests from tests/validate-signature-defs-inline.test.ts and merge them into the canonical tests/validate-signature-defs.test.ts: copy the three cases (valid inline object using validateInline and two validate(...) calls that assert missing required fields) into the canonical defs test file, keeping the same assertions and using validateInline and validate from generated imports, then delete the standalone validate-signature-defs-inline.test.ts to avoid parallel/duplicated coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ERD.puml`:
- Around line 254-262: The ERD's dataclass linkEvm is missing the optional
property "signatures" defined in lexicons/app/certified/link/evm.json; update
the dataclass linkEvm in ERD.puml to include signatures alongside address,
proof, and createdAt (ensure it follows the SHOW_FIELDS conditional like the
other fields and marks it as optional if your ERD convention supports that).
In `@lexicons/app/certified/signature/defs.json`:
- Around line 22-25: The "signature" property currently allows arbitrary-length
byte arrays; constrain it to the exact ECDSA raw (r,s) length by adding length
limits to the "signature" field in lexicons/app/certified/signature/defs.json
(the "signature" property): set minLength and maxLength to 64 (32-byte r +
32-byte s) so only 64-byte signatures pass schema validation.
---
Nitpick comments:
In `@tests/validate-signature-defs-inline.test.ts`:
- Around line 1-41: This file duplicates shared tests for
app.certified.signature.defs; remove these tests from
tests/validate-signature-defs-inline.test.ts and merge them into the canonical
tests/validate-signature-defs.test.ts: copy the three cases (valid inline object
using validateInline and two validate(...) calls that assert missing required
fields) into the canonical defs test file, keeping the same assertions and using
validateInline and validate from generated imports, then delete the standalone
validate-signature-defs-inline.test.ts to avoid parallel/duplicated coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d24334cf-05ff-4c03-b58c-b0e4d952183b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.agents/skills/building-with-hypercerts-lexicons/SKILL.md.changeset/add-signature-support.md.gitignoreAGENTS.mdERD.pumlREADME.mdSCHEMAS.mdlexicons/app/certified/actor/organization.jsonlexicons/app/certified/actor/profile.jsonlexicons/app/certified/badge/award.jsonlexicons/app/certified/badge/definition.jsonlexicons/app/certified/badge/response.jsonlexicons/app/certified/graph/follow.jsonlexicons/app/certified/link/evm.jsonlexicons/app/certified/location.jsonlexicons/app/certified/signature/defs.jsonlexicons/app/certified/signature/proof.jsonlexicons/org/hyperboards/board.jsonlexicons/org/hyperboards/displayProfile.jsonlexicons/org/hypercerts/claim/activity.jsonlexicons/org/hypercerts/claim/contribution.jsonlexicons/org/hypercerts/claim/contributorInformation.jsonlexicons/org/hypercerts/claim/rights.jsonlexicons/org/hypercerts/collection.jsonlexicons/org/hypercerts/context/acknowledgement.jsonlexicons/org/hypercerts/context/attachment.jsonlexicons/org/hypercerts/context/evaluation.jsonlexicons/org/hypercerts/context/measurement.jsonlexicons/org/hypercerts/funding/receipt.jsonlexicons/org/hypercerts/workscope/tag.jsonpackage.jsontests/validate-signature-defs-inline.test.tstests/validate-signature-defs.test.tstests/validate-signature-proof.test.tstests/validate-signing-example.test.ts
4 tasks
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
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.
Uh oh!
There was an error while loading. Please reload this page.