Fix: Migration Data Id in MigrationDataEvent#48
Conversation
WalkthroughMigration events now include an explicit, auto-incremented data_id emitted by MigrationLock; Noir structs and MigrationLock were updated to track and emit data_id. TypeScript types and wallet decoding were updated to surface and use dataId for selecting ABI and decoding event payloads. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MigrationLock as MigrationLock (Noir)
participant PXE as PXE
participant Wallet as MigrationBaseWallet (TS)
Client->>MigrationLock: lock_state(mut self, migration_data)
MigrationLock->>MigrationLock: build event { data_id = next_data_id, migration_data }
MigrationLock->>PXE: emit MigrationDataEvent(event)
MigrationLock->>MigrationLock: next_data_id = next_data_id + 1
MigrationLock-->>Client: return updated MigrationLock
Client->>Wallet: getMigrationNotesAndData(contractAddress, owner, abiType)
Wallet->>PXE: getPrivateEvents(contractAddress, owner)
PXE-->>Wallet: events[]
Wallet->>Wallet: for each event: extract dataId, select abiType, decode payload
Wallet-->>Client: return [{ note, dataId, data }, ...]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/aztec-state-migration/wallet/migration-base-wallet.ts`:
- Around line 244-249: The code currently silently skips unknown abiType for a
dataId (variables: abiType, dataId, txHash, notes[i].noteHash) which hides
mapping errors; in the method inside the MigrationBaseWallet class replace the
`continue` path with a fail-fast behavior by logging the full context and
throwing an Error (include dataId, txHash and noteHash in the message) so the
migration halts and surfaces the broken ABI mapping; remove the `continue`
branch and ensure callers can handle/propagate the thrown error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
noir/aztec-state-migration/src/mode_a/migration_data_event.nrnoir/aztec-state-migration/src/mode_a/migration_lock.nrts/aztec-state-migration/mode-a/types.tsts/aztec-state-migration/wallet/migration-base-wallet.ts
| if (!abiType) { | ||
| this.log.warn( | ||
| `Unknown migration dataId ${dataId} in tx ${txHash}, skipping note ${notes[i].noteHash}.`, | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Do not silently skip unknown dataId entries.
Continuing on unknown IDs returns partial note/data sets and can hide broken ABI mappings, leading to missed migration claims. This should fail fast.
Suggested fix
- if (!abiType) {
- this.log.warn(
- `Unknown migration dataId ${dataId} in tx ${txHash}, skipping note ${notes[i].noteHash}.`,
- );
- continue;
- }
+ if (!abiType) {
+ throw new Error(
+ `Unknown migration dataId ${dataId} in tx ${txHash}; missing AbiType mapping.`,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!abiType) { | |
| this.log.warn( | |
| `Unknown migration dataId ${dataId} in tx ${txHash}, skipping note ${notes[i].noteHash}.`, | |
| ); | |
| continue; | |
| } | |
| if (!abiType) { | |
| throw new Error( | |
| `Unknown migration dataId ${dataId} in tx ${txHash}; missing AbiType mapping.`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ts/aztec-state-migration/wallet/migration-base-wallet.ts` around lines 244 -
249, The code currently silently skips unknown abiType for a dataId (variables:
abiType, dataId, txHash, notes[i].noteHash) which hides mapping errors; in the
method inside the MigrationBaseWallet class replace the `continue` path with a
fail-fast behavior by logging the full context and throwing an Error (include
dataId, txHash and noteHash in the message) so the migration halts and surfaces
the broken ABI mapping; remove the `continue` branch and ensure callers can
handle/propagate the thrown error.
There was a problem hiding this comment.
We skip the events with unknown IDs on purpose. It's a way of migration notes filtration. The "warn" is to notify the dev about skipped note.
There was a problem hiding this comment.
@JanKuczma, thank you for the clarification! I understand now that the filtering by unknown dataId is intentional behavior rather than an error condition. The warn log makes sense as a way to keep developers informed about which notes are being filtered out during the migration process.
The current implementation is appropriate for this use case where you want graceful degradation and selective processing based on the provided ABI type mappings.
✏️ Learnings added
Learnt from: JanKuczma
Repo: Cardinal-Cryptography/aztec-state-migration PR: 48
File: ts/aztec-state-migration/wallet/migration-base-wallet.ts:244-249
Timestamp: 2026-03-02T17:19:54.901Z
Learning: In `ts/aztec-state-migration/wallet/migration-base-wallet.ts`, the `getMixedMigrationNotesAndData` method intentionally skips events with unknown `dataId` values as a filtering mechanism for migration notes. The warning log is meant to notify developers about skipped notes, not to indicate an error. This is by design for graceful degradation and selective processing based on provided ABI type mappings.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: Cardinal-Cryptography/aztec-state-migration PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T09:52:32.972Z
Learning: Applies to **/*.{nr,sol} : Update docs/spec/mode-a-spec.md and docs/spec/mode-b-spec.md when contract public functions, events, or externally visible behavior changes
Learnt from: CR
Repo: Cardinal-Cryptography/aztec-state-migration PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T09:52:32.972Z
Learning: Applies to **/*.sol : Document Solidity-facing interfaces in docs/architecture.md (Migrator section) when external interfaces change
Learnt from: CR
Repo: Cardinal-Cryptography/aztec-state-migration PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T09:52:32.972Z
Learning: Applies to **/*.{nr,ts,tsx} : When changing migration logic affecting old/new rollup flows, archive root verification, L1↔L2 messages, or note hashing, run yarn check:full E2E test
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
noir/aztec-state-migration/src/mode_a/migration_lock.nr (1)
98-133:⚠️ Potential issue | 🟠 MajorUpdate Mode A/Mode B spec docs for this API/event change.
Line 98 introduces a new public constructor and Line 129 changes event payload shape (
data_idadded), which is externally visible behavior. Please include corresponding updates indocs/spec/mode-a-spec.mdanddocs/spec/mode-b-spec.mdin this PR.As per coding guidelines
**/*.{nr,sol}: "Update docs/spec/mode-a-spec.md and docs/spec/mode-b-spec.md when contract public functions, events, or externally visible behavior changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@noir/aztec-state-migration/src/mode_a/migration_lock.nr` around lines 98 - 133, The PR adds a new public constructor new_with_offset and changes the MigrationDataEvent payload (data_id added) emitted by lock_state (which now uses and increments next_data_id), so update the Mode A and Mode B spec documents to reflect these externally visible API/event changes: document the new_with_offset signature and behavior (how next_data_id is initialized from data_id_offset), describe that lock_state emits MigrationDataEvent with the added data_id field and that next_data_id is incremented after emission, and note any recipient reconstruction/claiming implications of the new data_id behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@noir/aztec-state-migration/src/mode_a/migration_lock.nr`:
- Around line 98-133: The PR adds a new public constructor new_with_offset and
changes the MigrationDataEvent payload (data_id added) emitted by lock_state
(which now uses and increments next_data_id), so update the Mode A and Mode B
spec documents to reflect these externally visible API/event changes: document
the new_with_offset signature and behavior (how next_data_id is initialized from
data_id_offset), describe that lock_state emits MigrationDataEvent with the
added data_id field and that next_data_id is incremented after emission, and
note any recipient reconstruction/claiming implications of the new data_id
behavior.
JanKuczma
changed the title
2 participants
Summary by CodeRabbit
New Features
Documentation