Mode B signature over notes and public data#53
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR unifies Mode B signing into a single signMigrationModeB API that accepts an options object with optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/token-migration-mode-b.test.ts (1)
220-244: 🧹 Nitpick | 🔵 TrivialAdd one explicit multi-note signing case to preserve order-sensitive coverage.
This test now validates only the single-note path. Since Mode B hashing is order-sensitive over note hashes, add one case signing
notes: [n1, n2]to guard against ordering regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/token-migration-mode-b.test.ts` around lines 220 - 244, The test currently only covers single-note signing for Mode B (using signMigrationModeB with notes: [balanceNote]), but Mode B is order-sensitive; add an explicit multi-note signing case that signs two notes in a specific order to guard against ordering regressions. Modify the test to pull a second note from balanceNotesActive (e.g. balanceNotesActive[1]) and perform the same preparation (buildFullNoteProof / buildKeyNoteProofData / getMigrationSignerFromAddress) and then call signMigrationModeB with notes: [n1, n2] (preserving the intended order), and assert the signature/behavior matches expectations; keep the original single-note case intact for coverage of both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/integration-guide.md`:
- Around line 389-405: The examples reuse the same const name "sig" three times
causing redeclaration errors; update the three signMigrationModeB examples to
use distinct variable names (e.g., sigPrivate, sigPublic, sigMixed) so each call
to wallet.signMigrationModeB (the function referenced) produces a uniquely named
result and the snippets can be copied and compiled without conflict.
In `@docs/spec/mode-b-spec.md`:
- Around line 234-235: The spec text for signMigrationModeB() currently
contradicts the hash-order in the formula; update the formula so its
concatenation order matches the description and Noir builder (packed public data
fields first, then note hashes). Edit the formula/notation used in the section
that defines the hash input to explicitly show concat(packed_public_fields ||
note_hashes) or equivalent symbol names to match signMigrationModeB(), and
harmonize any adjacent examples or explanatory text that reference the old order
so all references use the public-first, notes-second ordering.
In `@ts/aztec-state-migration/mode-b/signature.ts`:
- Around line 14-30: Update docs/security.md to describe the new unified Mode B
signing model implemented in signature.ts: document that the signer (e.g.,
MigrationAccount.migrationKeySigner) now signs a combined payload using
Poseidon2Hasher with inputs in the specific order (packed public data fields
first, then note hashes) and that the final signed payload is
poseidon2_hash([DOM_SEP__CLAIM_B, oldVersion, newVersion, finalHash, recipient,
newApp]). Explain the trust/assumption changes when private notes and public
state entries are packed together, note the ordering requirement (matches the
Noir builder), and call out the related symbols/functions (DOM_SEP__CLAIM_B,
finalHash, MigrationSignature, and the Mode B signing flow in signature.ts) so
reviewers can trace the security impact.
- Around line 43-60: The function signMigrationModeB currently proceeds to hash
and sign even when both options.publicData and options.notes are missing or
empty; add a guard at the start of signMigrationModeB that checks
(options.publicData is undefined or length===0) AND (options.notes is undefined
or length===0) and fail fast (throw an Error or return a rejected promise) with
a clear message like "empty Mode B payload" before any encoding/poseidon2Hash
calls so invalid signing requests are rejected immediately.
---
Outside diff comments:
In `@e2e-tests/token-migration-mode-b.test.ts`:
- Around line 220-244: The test currently only covers single-note signing for
Mode B (using signMigrationModeB with notes: [balanceNote]), but Mode B is
order-sensitive; add an explicit multi-note signing case that signs two notes in
a specific order to guard against ordering regressions. Modify the test to pull
a second note from balanceNotesActive (e.g. balanceNotesActive[1]) and perform
the same preparation (buildFullNoteProof / buildKeyNoteProofData /
getMigrationSignerFromAddress) and then call signMigrationModeB with notes: [n1,
n2] (preserving the intended order), and assert the signature/behavior matches
expectations; keep the original single-note case intact for coverage of both
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73a5b5a9-baf6-470e-83c6-404bbb8f69b2
📒 Files selected for processing (12)
docs/integration-guide.mddocs/spec/mode-b-spec.mde2e-tests/migration-mode-b.test.tse2e-tests/migration-public-mode-b.test.tse2e-tests/nft-migration-mode-b.test.tse2e-tests/nft-migration-public-mode-b.test.tse2e-tests/token-migration-mode-a.test.tse2e-tests/token-migration-mode-b.test.tse2e-tests/token-migration-public-mode-b.test.tsts/aztec-state-migration/mode-b/index.tsts/aztec-state-migration/mode-b/signature.tsts/aztec-state-migration/wallet/migration-base-wallet.ts
DamianStraszak
approved these changes
Mar 6, 2026
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
2 participants
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.
Summary by CodeRabbit
Documentation
New Features
Breaking Changes