Feature: unconsumed l1-to-l2 message migration - #57
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 (3)
WalkthroughAdds L1→L2 message migration to Mode B: new Noir proof-data types and builder methods, migration nullifier constant and verification, TypeScript proof builders and types, e2e tests and test-utils exports, CI/test script integration, and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (TS)
participant ProofBuilder as Proof Builder (TS)
participant AztecNode as AztecNode
participant MigrationBuilder as Mode B Builder (Noir)
participant NullifierTree as Nullifier Tree
Client->>ProofBuilder: buildFullL1ToL2MessageProof(node, blockRef, oldApp, message, secret)
par Inclusion branch
ProofBuilder->>AztecNode: getL1ToL2MessageMembershipWitness()
AztecNode-->>ProofBuilder: inclusion witness (sender, content, secret_hash, leaf_index, sibling_path)
ProofBuilder->>ProofBuilder: construct L1ToL2MessageProofData
and Non-inclusion branch
ProofBuilder->>ProofBuilder: compute message_hash from secret
ProofBuilder->>AztecNode: getLowNullifierMembershipWitness()
AztecNode-->>ProofBuilder: non-inclusion witness
ProofBuilder->>ProofBuilder: construct NonNullificationProofData
end
ProofBuilder-->>Client: FullL1ToL2MessageProofData
Client->>MigrationBuilder: .without_owner().with_l1_to_l2_message(proof)
MigrationBuilder->>MigrationBuilder: verify_message_inclusion(...)
MigrationBuilder->>NullifierTree: verify_message_nullifier_non_inclusion(...)
NullifierTree-->>MigrationBuilder: ✓ not consumed
MigrationBuilder->>MigrationBuilder: compute migration_nullifier using DOM_SEP__L1_TO_L2_MIGRATION_NULLIFIER
MigrationBuilder-->>Client: builder updated
Client->>MigrationBuilder: .finish_at_snapshot()
MigrationBuilder-->>Client: ✓ migration complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
DamianStraszak
approved these changes
Mar 22, 2026
JanKuczma
force-pushed
the
feature/mode-b-at-block
branch
from
March 22, 2026 19:47
384141a to
7e21692
Compare
JanKuczma
force-pushed
the
feature/unconsumed-l1-to-l2-msg-migration
branch
from
March 22, 2026 20:38
2293ba3 to
5fe0e63
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/spec/migration-spec.md (1)
428-441:⚠️ Potential issue | 🟡 MinorDocument the new L1→L2 proof path in the general spec too.
MigrationModeBnow exposes.with_l1_to_l2_message(...), but this file still only enumerates private-note and public-state proof semantics, and the earlier scope text still reads as if L1-bridged state is excluded. Please reconcile those sections so the general spec does not contradict the Mode B spec.As per coding guidelines, "Update
docs/spec/mode-a-spec.md,docs/spec/mode-b-spec.md, and/ordocs/architecture.mdwhen contract interfaces change (public functions, events, externally visible behavior)."Also applies to: 453-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/spec/migration-spec.md` around lines 428 - 441, The general migration spec contradicts the Mode B details because MigrationModeB now exposes .with_l1_to_l2_message(...) but the general-spec text and the listed proof semantics only describe private-note and public-state paths; update the general spec text to explicitly include the L1→L2 proof path and describe its semantics (address/key-note verification, block-hash enqueueing to MigrationArchiveRegistry, and any L1→L2 message inclusion rules) so it matches MigrationModeB, and also update docs/spec/mode-b-spec.md (and if relevant docs/spec/mode-a-spec.md or docs/architecture.md) to reflect the new public API and remove the earlier scope wording that excludes L1-bridged state.
🤖 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/spec/mode-b-spec.md`:
- Around line 84-90: Add the ```text fence around the ASCII diagram containing
"L1-to-L2 message", "l1_to_l2_message_tree_root", "BlockHeader",
"nullifier_tree_root", and "snapshot_block_hash" in docs/spec/mode-b-spec.md so
the code block is tagged as text (i.e., replace the opening triple backticks
with ```text and keep the closing ```), which satisfies the markdownlint MD040
rule.
In `@e2e-tests/migration-mode-b-l1-to-l2-message.test.ts`:
- Around line 261-293: Add a negative test that proves using the correct message
proof with a different secret fails: after building a proof with
buildFullL1ToL2MessageProof (e.g., messageProof/msgSecret or
consumedMessageProof/consumedSecret), create a differentSecret (not equal to
msgSecret/consumedSecret) and call
newAppUser.methods.migrate_l1_to_l2_message_mode_b(proof, differentSecret,
blockHeader).send({ from: newUserManager.address }) and wrap it in
expectRevert(...) asserting the contract's revert message for a
secret/secretHash mismatch (i.e., the same revert string used by the contract
when the provided secret doesn't match the proof).
- Around line 100-107: The test currently locates the MessageSent log via
msgReceipt.logs and extracts the leaf index by slicing raw log.data
(msgSentLog.data.slice(0,66)), which is brittle; instead decode the event using
the InboxAbi (or the findEvent helper) so you get the typed field. Replace the
raw slice usage for msgLeafIndex with a decoded event call (decodeEventLog({
abi: InboxAbi, data: msgSentLog.data, topics: msgSentLog.topics })) and read the
decoded index field (or call findEvent to obtain the MessageSent event and use
its index property); update both occurrences (the block using msgSentLog and the
second block at lines ~155-162) to use decoded.index rather than slicing data.
In `@noir/aztec-state-migration/src/mode_b/builder.nr`:
- Around line 214-228: The proof of message inclusion
(message_proof.verify_message_inclusion) currently returns only a message_hash
but does not bind the caller-supplied secret to the committed secret in the
proof, allowing secret substitution; fix by extracting or returning the
committed secret hash (e.g., secretHash) from L1ToL2MessageProofData (or make
verify_message_inclusion expose/verify it) and assert before calling
non_null_proof.verify_message_nullifier_non_inclusion that hash(secret) ==
committed secretHash for the included leaf, then pass the verified secret to
verify_message_nullifier_non_inclusion to ensure the nullifier check uses the
same secret bound in the inclusion proof.
In `@ts/aztec-state-migration/mode-b/types.ts`:
- Around line 2-3: Remove the unused import AztecAddress from the top of the
file: keep the existing import of EthAddress and the Fr type but delete the
import line that brings in AztecAddress (references: AztecAddress, EthAddress,
Fr, types.ts) so only used types remain imported.
---
Outside diff comments:
In `@docs/spec/migration-spec.md`:
- Around line 428-441: The general migration spec contradicts the Mode B details
because MigrationModeB now exposes .with_l1_to_l2_message(...) but the
general-spec text and the listed proof semantics only describe private-note and
public-state paths; update the general spec text to explicitly include the L1→L2
proof path and describe its semantics (address/key-note verification, block-hash
enqueueing to MigrationArchiveRegistry, and any L1→L2 message inclusion rules)
so it matches MigrationModeB, and also update docs/spec/mode-b-spec.md (and if
relevant docs/spec/mode-a-spec.md or docs/architecture.md) to reflect the new
public API and remove the earlier scope wording that excludes L1-bridged state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a22f917e-8de1-4589-9bbf-28a5c9978e6c
📒 Files selected for processing (21)
.github/workflows/ci.ymldocs/architecture.mddocs/integration-guide.mddocs/security.mddocs/spec/migration-spec.mddocs/spec/mode-b-spec.mde2e-tests/migration-mode-b-l1-to-l2-message.test.tse2e-tests/migration-mode-b.test.tse2e-tests/test-utils.tsnoir/aztec-state-migration/src/constants.nrnoir/aztec-state-migration/src/mode_b/builder.nrnoir/aztec-state-migration/src/mode_b/l1_to_l2_message_proof_data.nrnoir/aztec-state-migration/src/mode_b/mod.nrnoir/aztec-state-migration/src/mode_b/non_nullification_proof_data.nrnoir/test-contracts/example-app/v1/src/main.nrnoir/test-contracts/example-app/v2/src/main.nrnoir/test-contracts/token-migration-app/v2/src/main.nrpackage.jsonts/aztec-state-migration/mode-b/index.tsts/aztec-state-migration/mode-b/proofs.tsts/aztec-state-migration/mode-b/types.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
e2e-tests/migration-mode-b-l1-to-l2-message.test.ts (2)
100-107:⚠️ Potential issue | 🟠 MajorDecode
MessageSentvia ABI instead of slicing raw log data.Extracting the leaf index via
log.data.slice(0, 66)is brittle and can silently break if event layout changes. Please decode by event signature (InboxAbi) or use the sharedfindEventhelper.#!/bin/bash # Verify brittle raw-log slicing is present and ABI decoding is absent in this test. rg -n -C2 'data\.slice\(0,\s*66\)|MessageSent event' e2e-tests/migration-mode-b-l1-to-l2-message.test.ts rg -n -C2 'decodeEventLog|findEvent' e2e-tests/migration-mode-b-l1-to-l2-message.test.tsAlso applies to: 155-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/migration-mode-b-l1-to-l2-message.test.ts` around lines 100 - 107, Replace the brittle manual slice of the log data when extracting the MessageSent leaf index: locate where msgReceipt and msgSentLog are used (the const msgSentLog = ... and const msgLeafIndex = ... lines) and decode the event using the InboxAbi (or the shared findEvent helper) instead of log.data.slice(0,66); e.g., use an ethers Interface for InboxAbi to call decodeEventLog('MessageSent', msgSentLog.data, msgSentLog.topics) or call the project's findEvent(msgReceipt, 'MessageSent') helper, then read the leaf index from the decoded event result and assign it to msgLeafIndex.
261-289: 🧹 Nitpick | 🔵 TrivialAdd a mismatched-secret negative test for this security-critical path.
Please add a case where the same message/proof is attempted with a different secret and is expected to revert, to guard secret-to-
secretHashbinding regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/migration-mode-b-l1-to-l2-message.test.ts` around lines 261 - 289, Add a negative test that attempts to migrate the same L1→L2 message using a different secret to ensure secret-to-secretHash binding is enforced: create a new wrongSecret, call buildFullL1ToL2MessageProof(oldAztecNode, provenBlockNumber, oldApp.address, consumedL1ToL2Message, wrongSecret) to produce a mismatched secret proof, then call newAppUser.methods.migrate_l1_to_l2_message_mode_b(...) .send({ from: newUserManager.address }) and assert it reverts (use expectRevert) to verify the migration fails with a mismatched secret.docs/spec/mode-b-spec.md (1)
84-84:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced proof-chain block.
The opening fence should be
```textto satisfy MD040.📝 Proposed fix
-``` +```text L1-to-L2 message ──Merkle proof──> l1_to_l2_message_tree_root ──(embedded in)──> BlockHeader │ consumption nullifier ──low-leaf non-inclusion──> nullifier_tree_root │ │ BlockHeader.hash() ──(== block_hash)──> snapshot_block_hash</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/spec/mode-b-spec.mdat line 84, The fenced proof-chain code block
currently opens with(no language) which triggers MD040; update the opening fence for that block (the one containing "L1-to-L2 message ──Merkle proof──> l1_to_l2_message_tree_root ... snapshot_block_hash") to use a language tag by changing the opening fence totext so the block becomestext ....</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/spec/migration-spec.md:
- Around line 453-460: Update the MigrationModeB row in the Migration Library
Builders table to explicitly mention L1→L2 message migration: include
"unconsumed L1→L2 messages" (or similar phrasing) in the Description for
MigrationModeB and ensure the Key Methods listing keeps
.with_l1_to_l2_message(proof)so readers see that MigrationModeB verifies and
migrates unconsumed L1→L2 messages via the.with_l1_to_l2_messagemethod.In
@noir/aztec-state-migration/src/mode_b/builder.nr:
- Around line 206-235: The spec is inconsistent with the implementation:
builder.nr computes migration_nullifier using
poseidon2_hash_with_separator([message_hash, secret, self.old_app.to_field()],
DOM_SEP__L1_TO_L2_MIGRATION_NULLIFIER) while docs/spec/mode-b-spec.md currently
states poseidon2([message_hash, old_app], ...); update the spec to state the
migration nullifier formula includes the secret as the second input (i.e.,
poseidon2([message_hash, secret, old_app],
DOM_SEP__L1_TO_L2_MIGRATION_NULLIFIER)) and mention the relation to
verify_message_inclusion and verify_message_nullifier_non_inclusion to avoid
confusion about secret binding.In
@ts/aztec-state-migration/mode-b/proofs.ts:
- Around line 238-245: The returned proof object is using message.index for
leaf_index but must use the membership witness' index; replace the use of
message.index with the witness-provided index variable (the _leafIndex from
const [_leafIndex, siblingPath] = witness) when setting leaf_index
(convert/format to the expected type if necessary) so the proof uses the
canonical witness leaf index rather than the caller-supplied message.index.
Duplicate comments:
In@docs/spec/mode-b-spec.md:
- Line 84: The fenced proof-chain code block currently opens with
(no language) which triggers MD040; update the opening fence for that block (the one containing "L1-to-L2 message ──Merkle proof──> l1_to_l2_message_tree_root ... snapshot_block_hash") to use a language tag by changing the opening fence totext so the block becomestext ....In
@e2e-tests/migration-mode-b-l1-to-l2-message.test.ts:
- Around line 100-107: Replace the brittle manual slice of the log data when
extracting the MessageSent leaf index: locate where msgReceipt and msgSentLog
are used (the const msgSentLog = ... and const msgLeafIndex = ... lines) and
decode the event using the InboxAbi (or the shared findEvent helper) instead of
log.data.slice(0,66); e.g., use an ethers Interface for InboxAbi to call
decodeEventLog('MessageSent', msgSentLog.data, msgSentLog.topics) or call the
project's findEvent(msgReceipt, 'MessageSent') helper, then read the leaf index
from the decoded event result and assign it to msgLeafIndex.- Around line 261-289: Add a negative test that attempts to migrate the same
L1→L2 message using a different secret to ensure secret-to-secretHash binding is
enforced: create a new wrongSecret, call
buildFullL1ToL2MessageProof(oldAztecNode, provenBlockNumber, oldApp.address,
consumedL1ToL2Message, wrongSecret) to produce a mismatched secret proof, then
call newAppUser.methods.migrate_l1_to_l2_message_mode_b(...) .send({ from:
newUserManager.address }) and assert it reverts (use expectRevert) to verify the
migration fails with a mismatched secret.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `620b5dad-1b25-470d-9b7b-c1c8a3d0d812` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5fe0e63c562f8f2e1e374b757471ed82f0056328 and 15474b7fe951db39195130c1914330e5bb6c914c. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `README.md` * `docs/integration-guide.md` * `docs/security.md` * `docs/spec/migration-spec.md` * `docs/spec/mode-b-spec.md` * `e2e-tests/migration-mode-b-l1-to-l2-message.test.ts` * `noir/aztec-state-migration/src/mode_b/builder.nr` * `noir/aztec-state-migration/src/mode_b/l1_to_l2_message_proof_data.nr` * `noir/test-contracts/example-app/v2/src/main.nr` * `ts/aztec-state-migration/mode-b/proofs.ts` * `ts/aztec-state-migration/mode-b/types.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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
New Features
Documentation
Tests
Chores