Feature: App-specific block snapshot verification in Mode B - #56
Merged
Conversation
WalkthroughThis pull request introduces a systematic renaming of the migration verification API in MigrationArchiveRegistry, changing Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App as Test App<br/>(v2)
participant MigrationBuilder as MigrationModeB<br/>Builder
participant Registry as MigrationArchiveRegistry
rect rgba(0, 100, 200, 0.5)
Note over User,Registry: Snapshot-Based Mode B Migration
User->>App: migrate_mode_b_at_snapshot(proof_data)
App->>MigrationBuilder: builder.finish_at_snapshot(recipient, signature)
MigrationBuilder->>MigrationBuilder: _verify_signature(recipient, signature)
MigrationBuilder->>MigrationBuilder: _finish_at_snapshot()
MigrationBuilder->>Registry: verify_migration_at_snapshot(block_hash)
Registry-->>MigrationBuilder: verification passed
MigrationBuilder-->>App: migration complete
end
rect rgba(200, 100, 0, 0.5)
Note over User,Registry: Block-Number-Based Mode B Migration
User->>App: migrate_mode_b_at_block(proof_data, block_header)
App->>MigrationBuilder: builder.finish_at_block(recipient, signature, block_number)
MigrationBuilder->>MigrationBuilder: _verify_signature(recipient, signature)
MigrationBuilder->>MigrationBuilder: _finish_at_block(block_number)
MigrationBuilder->>Registry: verify_migration_at_block(block_number, block_hash)
Registry-->>MigrationBuilder: verification passed
MigrationBuilder-->>App: migration complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 Generate unit tests (beta)
Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
DamianStraszak
approved these changes
Mar 22, 2026
DamianStraszak
left a comment
DamianStraszak
left a comment
Contributor
There was a problem hiding this comment.
Very nice generalization. Looks solid.
| /// Claim a Mode B (app specific block number) migration on the new rollup. | ||
| /// | ||
| /// Verifies note inclusion and non-nullification at the specified block number via | ||
| /// `migrate_notes_mode_b` and mints the equivalent tokens to the caller. |
Contributor
There was a problem hiding this comment.
This comment is copied from the other function, but still, it's a bit too specific. Instead of "mints tokens" should be "creates note".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/architecture.md (1)
19-27:⚠️ Potential issue | 🟡 MinorUpdate the topology diagram to the new Mode B app entrypoints.
This section renames the registry verification methods, but the same diagram still shows the app exposing
migrate_mode_b(). After this PR, the externally visible Mode B paths aremigrate_mode_b_at_snapshot()/migrate_mode_b_at_block(), so the topology view is still stale.Based on learnings: Applies to **/*.nr : 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).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` around lines 19 - 27, The topology diagram in docs/architecture.md is stale: it still shows the app exposing migrate_mode_b() while the contract now exposes migrate_mode_b_at_snapshot() and migrate_mode_b_at_block(); update the diagram and any labels referencing migrate_mode_b() to the new entrypoints migrate_mode_b_at_snapshot() and migrate_mode_b_at_block(), and similarly ensure MigrationKeyRegistry references the renamed verification methods (verify_migration_at_snapshot(), verify_migration_at_block()) and that docs/spec/mode-a-spec.md and docs/spec/mode-b-spec.md are updated to match these public function names so the diagrams and text reflect the new external contract interfaces.e2e-tests/migration-mode-b.test.ts (2)
198-203:⚠️ Potential issue | 🟡 MinorAssert that a second active note exists before indexing it.
The setup only rejects the zero-note case, but Line 283 assumes
balanceNotesActive[1]exists. If the app or wallet collapses balances into a single active note, this test fails before it exercises the new at-block path.Suggested guard
- if (balanceNotesActive.length === 0) { - throw new Error("No active balance notes found"); + if (balanceNotesActive.length < 2) { + throw new Error( + "Expected at least two active balance notes to cover snapshot and at-block migration paths", + ); }Also applies to: 283-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/migration-mode-b.test.ts` around lines 198 - 203, The test currently only checks balanceNotesActive.length !== 0 but later accesses balanceNotesActive[1]; update the guard to assert that balanceNotesActive has at least two active notes (e.g., if (balanceNotesActive.length < 2) throw new Error("Expected at least two active balance notes for indexing at-block path")), or alternatively create/ensure a second active note before using balanceNotesActive[1]; adjust the code paths around the indexing that reference balanceNotesActive[1] so they only run when balanceNotesActive.length >= 2 (symbols: balanceNotesActive, balanceNote and the subsequent indexing logic that uses balanceNotesActive[1]).
143-156:⚠️ Potential issue | 🟠 MajorUse a different registered block for the at-block scenario.
Step 10.1 reuses the same
provenBlockNumberthat was already stored as the global snapshot height. With both paths anchored to the same header/hash, this still passes ifmigrate_mode_b_at_blockaccidentally routes through snapshot verification. Register another block and rebuild the second proof/header against that block so this test actually distinguishes the new behavior.Also applies to: 279-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/migration-mode-b.test.ts` around lines 143 - 156, The test currently reuses provenBlockNumber and archiveProof/archive_sibling_path when calling newArchiveRegistry.methods.set_snapshot_height(...) and later for the at-block scenario, which masks incorrect routing through snapshot verification; instead register a second block (create a new block number variable, a new header and proof) and use that new block/header/proof for the migrate_mode_b_at_block scenario so the two paths are anchored to different headers/hashes; update the calls around set_snapshot_height/get_snapshot_height and the at-block test (also change the duplicated logic in the later section around lines 279-321) to build and pass the distinct proof/header to the at-block invocation.noir/test-contracts/example-app/v2/src/main.nr (1)
193-236:⚠️ Potential issue | 🟠 MajorUpdate specs/architecture docs for the new Mode B interface.
This file adds/renames externally visible contract functions (
migrate_mode_b_at_snapshot,migrate_mode_b_at_block,set_migration_block_number). Please update the relevant docs in this PR.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: 385-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@noir/test-contracts/example-app/v2/src/main.nr` around lines 193 - 236, The PR introduces/renames externally visible contract functions (migrate_mode_b_at_snapshot, migrate_mode_b_at_block, set_migration_block_number) but the specs/architecture docs were not updated; update docs/spec/mode-a-spec.md, docs/spec/mode-b-spec.md, and docs/architecture.md to reflect the new Mode B interface: add/modify entries for these functions (signatures, visibility [external/private], purpose, parameters, side-effects like minting to balances and proof verification via MigrationModeB::finish_at_snapshot/finish_at_block), note the storage slot requirement (balances_expected_storage_slot from STORAGE_LAYOUT_ExampleMigrationAppV1), and any changes to externally visible behavior or migration flow so the docs and contract interface remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@noir/test-contracts/example-app/v2/src/main.nr`:
- Line 56: The field migration_block_number is currently caller-writable via
set_migration_block_number which allows anyone to hijack Mode B-at-block
verification; change the storage to a restricted variable and add an ownership
check in the setter: make migration_block_number private/mutable (or use an
Ownable/owner field already present) and update set_migration_block_number to
require Context::caller (or the contract owner identifier used in this codebase)
matches the authorized owner before setting; apply the same restriction to the
other setter(s) referenced around lines ~385-388 and ensure callers cannot
bypass the check.
---
Outside diff comments:
In `@docs/architecture.md`:
- Around line 19-27: The topology diagram in docs/architecture.md is stale: it
still shows the app exposing migrate_mode_b() while the contract now exposes
migrate_mode_b_at_snapshot() and migrate_mode_b_at_block(); update the diagram
and any labels referencing migrate_mode_b() to the new entrypoints
migrate_mode_b_at_snapshot() and migrate_mode_b_at_block(), and similarly ensure
MigrationKeyRegistry references the renamed verification methods
(verify_migration_at_snapshot(), verify_migration_at_block()) and that
docs/spec/mode-a-spec.md and docs/spec/mode-b-spec.md are updated to match these
public function names so the diagrams and text reflect the new external contract
interfaces.
In `@e2e-tests/migration-mode-b.test.ts`:
- Around line 198-203: The test currently only checks balanceNotesActive.length
!== 0 but later accesses balanceNotesActive[1]; update the guard to assert that
balanceNotesActive has at least two active notes (e.g., if
(balanceNotesActive.length < 2) throw new Error("Expected at least two active
balance notes for indexing at-block path")), or alternatively create/ensure a
second active note before using balanceNotesActive[1]; adjust the code paths
around the indexing that reference balanceNotesActive[1] so they only run when
balanceNotesActive.length >= 2 (symbols: balanceNotesActive, balanceNote and the
subsequent indexing logic that uses balanceNotesActive[1]).
- Around line 143-156: The test currently reuses provenBlockNumber and
archiveProof/archive_sibling_path when calling
newArchiveRegistry.methods.set_snapshot_height(...) and later for the at-block
scenario, which masks incorrect routing through snapshot verification; instead
register a second block (create a new block number variable, a new header and
proof) and use that new block/header/proof for the migrate_mode_b_at_block
scenario so the two paths are anchored to different headers/hashes; update the
calls around set_snapshot_height/get_snapshot_height and the at-block test (also
change the duplicated logic in the later section around lines 279-321) to build
and pass the distinct proof/header to the at-block invocation.
In `@noir/test-contracts/example-app/v2/src/main.nr`:
- Around line 193-236: The PR introduces/renames externally visible contract
functions (migrate_mode_b_at_snapshot, migrate_mode_b_at_block,
set_migration_block_number) but the specs/architecture docs were not updated;
update docs/spec/mode-a-spec.md, docs/spec/mode-b-spec.md, and
docs/architecture.md to reflect the new Mode B interface: add/modify entries for
these functions (signatures, visibility [external/private], purpose, parameters,
side-effects like minting to balances and proof verification via
MigrationModeB::finish_at_snapshot/finish_at_block), note the storage slot
requirement (balances_expected_storage_slot from
STORAGE_LAYOUT_ExampleMigrationAppV1), and any changes to externally visible
behavior or migration flow so the docs and contract interface remain in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23577703-94e3-4614-b6b0-70d869ff61f1
⛔ Files ignored due to path filters (2)
solidity/soldeer.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
.devcontainer/development/Dockerfile.devcontainer/mac-arm/Dockerfile.devcontainer/testing/Dockerfile.github/workflows/ci.yml.github/workflows/publish.yml.mcp.jsonCLAUDE.mdREADME.mddocs/architecture.mddocs/integration-guide.mddocs/security.mddocs/spec/migration-spec.mddocs/spec/mode-a-spec.mddocs/spec/mode-b-spec.mde2e-tests/migration-key-registry.test.tse2e-tests/migration-mode-a.test.tse2e-tests/migration-mode-b.test.tse2e-tests/migration-public-mode-b.test.tse2e-tests/nft-migration-mode-a.test.tse2e-tests/nft-migration-mode-b.test.tse2e-tests/package.jsone2e-tests/test-utils.tse2e-tests/token-migration-mode-a.test.tse2e-tests/token-migration-mode-b.test.tse2e-tests/token-migration-public-mode-b.test.tsnoir/aztec-state-migration/Nargo.tomlnoir/aztec-state-migration/src/mode_a/builder.nrnoir/aztec-state-migration/src/mode_b/builder.nrnoir/contracts/migration-archive-registry/Nargo.tomlnoir/contracts/migration-archive-registry/src/main.nrnoir/contracts/migration-key-registry/Nargo.tomlnoir/test-contracts/example-app/v1/Nargo.tomlnoir/test-contracts/example-app/v2/Nargo.tomlnoir/test-contracts/example-app/v2/src/main.nrnoir/test-contracts/minimal-benchmark/Nargo.tomlnoir/test-contracts/minimal-benchmark/src/main.nrnoir/test-contracts/nft-migration-app/v1/Nargo.tomlnoir/test-contracts/nft-migration-app/v2/Nargo.tomlnoir/test-contracts/nft-migration-app/v2/src/main.nrnoir/test-contracts/token-migration-app/v1/Nargo.tomlnoir/test-contracts/token-migration-app/v2/Nargo.tomlnoir/test-contracts/token-migration-app/v2/src/main.nrnoir/tests/Nargo.tomlsolidity/contracts/Poseidon2Deploy.solsolidity/contracts/RegisterNewRollupVersionPayload.solsolidity/foundry.tomlsolidity/remappings.txtts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.tsts/aztec-state-migration/package.jsonts/aztec-state-migration/proofs.ts
💤 Files with no reviewable changes (1)
- solidity/contracts/RegisterNewRollupVersionPayload.sol
JanKuczma
force-pushed
the
feature/mode-b-at-block
branch
from
March 22, 2026 19:47
384141a to
7e21692
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/migration-spec.md`:
- Around line 276-279: Update the Mode B builder example and surrounding text to
mention the new block-number alternative: note that in addition to
.finish_at_snapshot(recipient, signature) callers can use
.finish_at_block(recipient, signature, block_number) to verify against a
specific block height; reference the builder and Mode B in the wording and add a
short clarifying sentence that .finish_at_block takes an extra block_number
parameter and performs the same migration-signature check at that block.
In `@e2e-tests/migration-mode-b.test.ts`:
- Line 283: The test assumes a second active note but only checked for
zero-length earlier; before using balanceNotesActive[1] (assigned to
balanceNote2) add a guard or assertion that balanceNotesActive.length >= 2, or
change the logic to handle the single-note case (e.g., use balanceNotesActive[0]
or branch accordingly). Locate the access to balanceNotesActive and the
assignment to balanceNote2 and either assert/expect length >= 2 or conditionally
select the correct index so the test cannot throw an out-of-bounds error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: baace820-6cc3-4639-83d1-a1698cfd441c
📒 Files selected for processing (15)
docs/architecture.mddocs/integration-guide.mddocs/security.mddocs/spec/migration-spec.mddocs/spec/mode-a-spec.mddocs/spec/mode-b-spec.mde2e-tests/migration-mode-b.test.tsnoir/aztec-state-migration/src/mode_a/builder.nrnoir/aztec-state-migration/src/mode_b/builder.nrnoir/contracts/migration-archive-registry/src/main.nrnoir/test-contracts/example-app/v2/src/main.nrnoir/test-contracts/minimal-benchmark/src/main.nrnoir/test-contracts/nft-migration-app/v2/src/main.nrnoir/test-contracts/token-migration-app/v2/src/main.nrts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.ts
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
Release Notes
Refactor
verify_migration_mode_a/b→verify_migration_at_block/at_snapshotfinish_at_snapshot()andfinish_at_block()methods with explicit block-based verification optionDocumentation