Restructure TypeScript SDK as publishable npm package#43
Merged
Conversation
AdamGagol
reviewed
Feb 27, 2026
| let notesAndData: MigrationNoteAndData<unknown>[] = []; | ||
|
|
||
| for (const [txHash, notes] of noteByTxHash) { | ||
| if (abiTypes.length !== notes.length) { |
Contributor
There was a problem hiding this comment.
This will fail for user that partially migrated already, because we're fetching all historically created migration notes. Proposed fix: do not throw an error here, just continue. On new rollup we're filtering out migrated notes, so I think it's safe to do so.
Contributor
Author
There was a problem hiding this comment.
I'm not sure how it would fail. We're fetching all historically created migration notes along with all emitted events (so per tx, the number of notes must match the number of events must match the number of abi types). Then we filter out (using knowledge about nullifiers from the new rollup). It is something to test though.
Contributor
There was a problem hiding this comment.
I think the problem would arise in the following scenario:
- Monday: user is locking a single note via Tx A (1 note, 1 event)
- Tuesday: user is locking two additional notes via Tx B (2 notes, 2 events)
- Wednesday: user tries to claim Tx B notes through this interface. Frontend calls this functions, decodes Tx B, and gets abiTypes of length 2. But noteByTxHash has length 3, as it is based on all historically locked notes.
Regarding filtering - as I understand, it is done after this check.
Contributor
Author
There was a problem hiding this comment.
Are the data types in Tx B (in events) different? If not then the app/wallet should use getMigrationNotesAndData. But when an App has two entrypoints for locking notes with different MigrationData structure, and both are used in separate Txs, then neither method is suitable 🤔. Maybe getMigrationNotesAndData should "ignore" events which cannot be decoded given some abi. This way the abi would specify which MigrationNotes are fetched (and which ignored).
DamianStraszak
approved these changes
Mar 1, 2026
* Move l1 archive migration to test utils * Publishable SDK (#46) * reorg scopes * npm publish ready * publish workflow * Update e2e-tests/test-utils.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update ts/aztec-state-migration/noir-contracts/MigrationKeyRegistry.lazy.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update ts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.lazy.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * add build step in readme --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
WalkthroughRestructures the repo into a Yarn workspace, publishes a new ts/aztec-state-migration package with typed Noir contract wrappers and lazy loaders, consolidates batch proof APIs into single-item/blockReference variants, removes bridge/polling modules (moved into e2e test utils), updates tests to package imports, and adds CI publish/build workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant E2E as E2E Test
participant TU as TestUtils (bridgeBlock)
participant L1W as L1 Wallet Client
participant Migr as L1 Migrator Contract
participant Inbox as L1 Inbox Contract
participant L1RPC as L1 Node/RPC
participant L2 as Aztec Node
E2E->>TU: bridgeBlock(l1Wallet, l2Node, blockReference)
TU->>L1W: send migrateArchiveRoot() tx
L1W->>Migr: invoke L1Migrator.migrateArchiveRoot
Migr->>L1RPC: broadcast tx
L1RPC-->>TU: tx receipt with logs
TU->>Inbox: decode MessageSent / ArchiveRootMigrated (extract messageHash, archiveRoot, leaf/index)
TU->>L2: waitForL1ToL2Message(messageHash) / poll proven block
L2-->>TU: provenBlockNumber & inclusion status
TU-->>E2E: {provenBlockNumber, provenArchiveRoot, archiveProof, blockHeader}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ts/aztec-state-migration/mode-b/signature.ts (1)
3-7: 🧹 Nitpick | 🔵 TrivialRemove unused import
DOM_SEP__CLAIM_A.
DOM_SEP__CLAIM_Ais imported but no longer used in this file sincesignMigrationModeAwas moved tomode-a/signature.ts. OnlyDOM_SEP__CLAIM_BandDOM_SEP__MSK_M_GENare used here.🧹 Proposed fix
import { - DOM_SEP__CLAIM_A, DOM_SEP__CLAIM_B, DOM_SEP__MSK_M_GEN, } from "../constants.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/aztec-state-migration/mode-b/signature.ts` around lines 3 - 7, The file imports an unused constant DOM_SEP__CLAIM_A; remove DOM_SEP__CLAIM_A from the import list at the top of signature.ts so only DOM_SEP__CLAIM_B and DOM_SEP__MSK_M_GEN are imported, keeping the rest of the import statement and symbols unchanged.
♻️ Duplicate comments (1)
ts/aztec-state-migration/wallet/migration-base-wallet.ts (1)
231-235:⚠️ Potential issue | 🟠 MajorAvoid global ABI-length hard failure across historical txs.
Line 231 enforces the same
abiTypes.lengthfor every tx and throws on mismatch. This still breaks mixed historical flows where txs emitted different note/event counts. Prefer per-tx ABI mapping (or tolerant filtering) instead of aborting the whole retrieval.Also applies to: 240-244
🤖 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 231 - 235, The current strict check in migration-base-wallet.ts that throws when abiTypes.length !== notes.length (and the similar check at 240-244) must be replaced with tolerant per-transaction handling: instead of aborting, map or filter notes to ABI entries on a per-tx basis (using txHash to log mismatches) and proceed with only the matching pairs; update the logic in the function that compares abiTypes and notes (referencing the abiTypes array, notes array, and txHash) to either (a) align by index up to min(abiTypes.length, notes.length) and process those, or (b) use a per-tx ABI mapping to match events to types, and emit a warning/log via the existing logger when mismatches occur rather than throwing an Error so historical mixed-format transactions do not halt retrieval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 54-57: The "Set version" step currently interpolates
inputs.version directly into run, creating a command-injection risk; change the
step to place inputs.version into an environment variable (e.g., env: VERSION:
${{ inputs.version }}), validate that $VERSION matches an allowed pattern (for
example a strict semver regex like ^[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.-]+)?$)
and fail early if it doesn't, and then call npm version using the safe, quoted
env var (npm version "$VERSION" --no-git-tag-version); keep the step name "Set
version" so it's easy to locate and ensure validation occurs before executing
npm.
- Around line 60-67: The workflow conditional uses dot notation on a hyphenated
input key (if: inputs.dry-run and if: ${{ !inputs.dry-run }}) which parses as a
subtraction; change both conditions to bracket notation (e.g., inputs['dry-run']
and !inputs['dry-run']) so the workflow correctly accesses the "dry-run" input;
update the two occurrences surrounding the npm publish step and the subsequent
Publish step to use bracket access.
In `@e2e-tests/package.json`:
- Around line 1-25: The new workspace package "@aztec-state-migration/e2e-tests"
was added in e2e-tests/package.json but docs/architecture.md wasn't updated;
edit docs/architecture.md to add this e2e-tests component to the component
catalog/topology (including name "@aztec-state-migration/e2e-tests", purpose
(end-to-end tests/state-migration validation), and its placement in the
workspace/deployment topology), ensuring the change satisfies the repo guideline
for files matching **/*.{ts,tsx,json,toml,md}.
In `@e2e-tests/test-utils.ts`:
- Around line 399-401: The check `if (!messageBlock)` incorrectly treats a valid
falsy block value (e.g., 0) as missing; change it to an explicit nullish check
when handling the result of aztecNode.getL1ToL2MessageBlock(messageHash) (for
example use `messageBlock == null` or `messageBlock === null || messageBlock ===
undefined`) before returning undefined, keeping the rest of the logic (including
the subsequent call to aztecNode.getProvenBlockNumber()) unchanged.
In `@ts/aztec-state-migration/key.ts`:
- Around line 1-13: Document the new master migration key derivation in
docs/security.md: add an entry describing deriveMasterMigrationSecretKey (the
function) and the use of sha512ToGrumpkinScalar with the domain separator
DOM_SEP__MSK_M_GEN, state the intended trust assumptions, domain-separation
rationale, attacker/usage model (what this key is for: migration
signing/encryption), and any migration/compatibility notes; ensure the wording
matches the repository's security-doc style and references the exact symbols
deriveMasterMigrationSecretKey and DOM_SEP__MSK_M_GEN so reviewers can correlate
the code change to the documented rationale.
In `@ts/aztec-state-migration/mode-b/proofs.ts`:
- Around line 27-30: The JSDoc for buildNullifierProof is out of date: update
the parameter name from blockNumber to blockReference in the function's comment
block so it matches the actual signature (buildNullifierProof(node: AztecNode,
blockReference: BlockNumber | BlockHash, noteDao: NoteDao)); also adjust any
descriptive text referencing a numeric block to reflect that blockReference may
be a BlockNumber or BlockHash and ensure param name matches the JSDoc `@param` tag
and any examples in the same comment.
In `@ts/aztec-state-migration/proofs.ts`:
- Around line 35-37: Update the thrown Error in the witness lookup failure to
include the lookup key uniqueHash along with noteDao.noteHash so the error
message contains both identifiers; locate the throw in proofs.ts (the block that
currently throws `Could not get note hash membership witness for note
${noteDao.noteHash.toString()}`) and append or interpolate the uniqueHash value
(the variable used for the lookup) into the message to provide both noteHash and
uniqueHash for easier triage.
In `@ts/aztec-state-migration/wallet/entrypoints/browser.ts`:
- Line 10: The import should mark EmbeddedWalletOptions as a type-only import to
match project conventions: split the current import of EmbeddedWalletOptions and
WalletDB into a type import for EmbeddedWalletOptions and a regular import for
WalletDB, i.e. keep WalletDB as a value import and change EmbeddedWalletOptions
to a type import so symbols EmbeddedWalletOptions and WalletDB are imported
appropriately.
In `@ts/aztec-state-migration/wallet/migration-base-wallet.ts`:
- Around line 329-333: Update the stale JSDoc that still refers to "batch" or
"notes" to reflect the new single-note API: change descriptions to singular
language for the note parameter and any mention of processing multiple notes,
and ensure param tags for blockReference, note, and noteMapper and the `@returns`
description describe a single decoded note (storage slot, randomness, nonce,
sibling path). Apply the same singular wording fixes to the other affected JSDoc
blocks (the ones around the comment groups that include lines referencing notes
at the later locations).
- Around line 190-198: The loop that calls this.getPrivateEvents inside "for
(const [txHash, notes] of noteByTxHash)" is performing network requests
serially; change both occurrences (the loop around getPrivateEvents and the
similar loop at the later block) to build an array of promises (e.g., map over
noteByTxHash entries calling getPrivateEvents with eventFilter(txHash) and the
same eventSelector/abiType) and await them with Promise.all (or
Promise.allSettled if per-tx failure isolation is needed), then re-associate
results to their txHash to preserve logic; ensure error handling is adjusted
accordingly so a single failed fetch doesn’t block all other txs.
- Around line 278-281: The code uses allNotes.map(...) solely for side effects
which triggers the lint rule; change the iterator to a side-effect construct
such as allNotes.forEach(...) (or a for...of loop) so the callback's return
value isn't ignored; update the block that references allNotes and noteByTxHash
(the anonymous callback that gets currentNotes via noteByTxHash.get and then
sets noteByTxHash.set) to use forEach/for...of instead of map to satisfy
lint/suspicious/useIterableCallbackReturn.
---
Outside diff comments:
In `@ts/aztec-state-migration/mode-b/signature.ts`:
- Around line 3-7: The file imports an unused constant DOM_SEP__CLAIM_A; remove
DOM_SEP__CLAIM_A from the import list at the top of signature.ts so only
DOM_SEP__CLAIM_B and DOM_SEP__MSK_M_GEN are imported, keeping the rest of the
import statement and symbols unchanged.
---
Duplicate comments:
In `@ts/aztec-state-migration/wallet/migration-base-wallet.ts`:
- Around line 231-235: The current strict check in migration-base-wallet.ts that
throws when abiTypes.length !== notes.length (and the similar check at 240-244)
must be replaced with tolerant per-transaction handling: instead of aborting,
map or filter notes to ABI entries on a per-tx basis (using txHash to log
mismatches) and proceed with only the matching pairs; update the logic in the
function that compares abiTypes and notes (referencing the abiTypes array, notes
array, and txHash) to either (a) align by index up to min(abiTypes.length,
notes.length) and process those, or (b) use a per-tx ABI mapping to match events
to types, and emit a warning/log via the existing logger when mismatches occur
rather than throwing an Error so historical mixed-format transactions do not
halt retrieval.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/publish.yml.gitignoreREADME.mde2e-tests/deploy-types.tse2e-tests/deploy.tse2e-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/package.jsone2e-tests/test-utils.tspackage.jsonts/aztec-state-migration/bridge.tsts/aztec-state-migration/index.tsts/aztec-state-migration/key.tsts/aztec-state-migration/mode-a/index.tsts/aztec-state-migration/mode-a/proofs.tsts/aztec-state-migration/mode-a/signature.tsts/aztec-state-migration/mode-b/index.tsts/aztec-state-migration/mode-b/proofs.tsts/aztec-state-migration/mode-b/signature.tsts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.lazy.tsts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.tsts/aztec-state-migration/noir-contracts/MigrationKeyRegistry.lazy.tsts/aztec-state-migration/noir-contracts/MigrationKeyRegistry.tsts/aztec-state-migration/noir-contracts/index.tsts/aztec-state-migration/noir-contracts/lazy.tsts/aztec-state-migration/package.jsonts/aztec-state-migration/polling.tsts/aztec-state-migration/proofs.tsts/aztec-state-migration/tsconfig.jsonts/aztec-state-migration/types.tsts/aztec-state-migration/wallet/entrypoints/browser.tsts/aztec-state-migration/wallet/index.tsts/aztec-state-migration/wallet/migration-account.tsts/aztec-state-migration/wallet/migration-base-wallet.tsts/aztec-state-migration/wallet/migration-embedded-wallet.tstsconfig.json
💤 Files with no reviewable changes (5)
- ts/aztec-state-migration/polling.ts
- .gitignore
- ts/aztec-state-migration/wallet/index.ts
- ts/aztec-state-migration/bridge.ts
- ts/aztec-state-migration/types.ts
JanKuczma
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
64-72:⚠️ Potential issue | 🟡 MinorDuplicate
yarn ts:buildin Formatting section.Line 69 includes
yarn ts:buildunder the Formatting section, but this is a compilation command, not a formatting command. It's already documented in the Compilation section (line 60).Suggested fix
## Formatting ```sh yarn fmt # Format all code (Noir, Solidity, TypeScript) yarn fmt:check # Check formatting without modifying files (CI use) -yarn ts:build # Compile TypeScript</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 64 - 72, Remove the duplicate compilation command
from the Formatting section: locate the "## Formatting" heading and the list
item containing "yarn ts:build" and delete that line so only formatting-related
commands remain (keep "yarn fmt" and "yarn fmt:check"); compilation commands
such as "yarn ts:build" should remain only in the Compilation section where it's
already documented.</details> </blockquote></details> </blockquote></details>♻️ Duplicate comments (1)
e2e-tests/test-utils.ts (1)
574-577:⚠️ Potential issue | 🟡 MinorUse an explicit nullish check for
messageBlock.
getL1ToL2MessageBlockmay return0for messages included in the genesis block. The current!messageBlockcheck would incorrectly treat block0as absent.Suggested fix
- if (!messageBlock) return undefined; + if (messageBlock == null) return undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/test-utils.ts` around lines 574 - 577, The nullity check for messageBlock incorrectly treats block 0 as absent; change the condition in the code that calls getL1ToL2MessageBlock (variable messageBlock) to use an explicit nullish check (e.g., messageBlock === undefined or messageBlock == null) instead of !messageBlock, then keep the existing logic comparing against getProvenBlockNumber() (proven) and returning messageBlock or undefined accordingly; update the check around getL1ToL2MessageBlock / messageBlock / getProvenBlockNumber to ensure genesis block (0) is handled as a valid value.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@e2e-tests/test-utils.ts`: - Around line 362-369: L1MigratorAbi and InboxAbi are duplicated in e2e-tests/test-utils.ts and e2e-tests/deploy.ts; consolidate by moving the ABI definitions into a single shared export (e.g., export const L1MigratorAbi / InboxAbi from a common module) and update both test-utils and deploy to import those symbols instead of redefining them; ensure parseAbi usage and exact string signatures for L1MigratorAbi and InboxAbi are preserved when relocating. In `@ts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.lazy.ts`: - Around line 23-38: The function getMigrationArchiveRegistryContractArtifact currently uses value-only caching (cachedArtifact) which allows two concurrent first callers to both import and parse the JSON; fix this by caching the in-flight Promise instead: introduce a module-level variable (e.g., cachedArtifactPromise or reuse cachedArtifact but typed as Promise<ContractArtifact> | ContractArtifact | undefined), and in getMigrationArchiveRegistryContractArtifact if the promise is unset create and assign a Promise from import(...).then(json => loadContractArtifact(json as NoirCompiledContract)) and return that promise; optionally, when the promise resolves store the resolved ContractArtifact into cachedArtifact for fast synchronous reads in future calls. Ensure you reference the existing symbols getMigrationArchiveRegistryContractArtifact, cachedArtifact (or new cachedArtifactPromise), and loadContractArtifact when implementing this change. In `@ts/aztec-state-migration/noir-contracts/MigrationKeyRegistry.lazy.ts`: - Around line 115-120: Narrow deployWithOpts so its variadic args only accept the contract constructor signature: stop using a free generic M over MigrationKeyRegistryContract["methods"]; instead either constrain M to the constructor key or remove M and type args as Parameters<MigrationKeyRegistryContract["methods"]["constructor"]> (and similarly update the overload at the other occurrence on lines 133-134), keeping opts the same; update the declaration of deployWithOpts to use that constructor parameter type to prevent non-constructor method signatures from being passed. --- Outside diff comments: In `@README.md`: - Around line 64-72: Remove the duplicate compilation command from the Formatting section: locate the "## Formatting" heading and the list item containing "yarn ts:build" and delete that line so only formatting-related commands remain (keep "yarn fmt" and "yarn fmt:check"); compilation commands such as "yarn ts:build" should remain only in the Compilation section where it's already documented. --- Duplicate comments: In `@e2e-tests/test-utils.ts`: - Around line 574-577: The nullity check for messageBlock incorrectly treats block 0 as absent; change the condition in the code that calls getL1ToL2MessageBlock (variable messageBlock) to use an explicit nullish check (e.g., messageBlock === undefined or messageBlock == null) instead of !messageBlock, then keep the existing logic comparing against getProvenBlockNumber() (proven) and returning messageBlock or undefined accordingly; update the check around getL1ToL2MessageBlock / messageBlock / getProvenBlockNumber to ensure genesis block (0) is handled as a valid value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.gitignoreREADME.mde2e-tests/deploy.tse2e-tests/migration-mode-a.test.tse2e-tests/migration-mode-b.test.tse2e-tests/nft-migration-mode-a.test.tse2e-tests/nft-migration-mode-b.test.tse2e-tests/test-utils.tspackage.jsonts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.lazy.tsts/aztec-state-migration/noir-contracts/MigrationKeyRegistry.lazy.tsts/aztec-state-migration/package.json💤 Files with no reviewable changes (1)
- .gitignore
ts/aztec-state-migration/noir-contracts/MigrationArchiveRegistry.lazy.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ts/aztec-state-migration/wallet/migration-base-wallet.ts (1)
190-198: 🧹 Nitpick | 🔵 TrivialPer-tx event fetches are still serialized; consider parallelizing to reduce latency.
Both loops await PXE calls one tx at a time, which scales poorly with many migration txs.
⚡ Minimal refactor direction
- for (const [txHash, notes] of noteByTxHash) { - const events = await ... - ... - } + const perTx = await Promise.all( + [...noteByTxHash.entries()].map(async ([txHash, notes]) => { + const events = await ... + ... + return ... + }), + ); + return perTx.flat();Also applies to: 230-239
🤖 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 190 - 198, The loop over noteByTxHash is making sequential awaits to this.getPrivateEvents for each tx; refactor to run fetches in parallel by mapping noteByTxHash entries to promises and awaiting them with Promise.all (or Promise.allSettled) so multiple calls to getPrivateEvents (using eventSelector, abiType and eventFilter(txHash)) execute concurrently; ensure you preserve the association between txHash and its events when collecting results and handle errors per-promise if needed.e2e-tests/test-utils.ts (1)
571-586:⚠️ Potential issue | 🟡 MinorUse an explicit nullish check for
messageBlock.Line 583 currently treats
0as absent. This can incorrectly skip a valid block number.Suggested fix
- if (!messageBlock) return undefined; + if (messageBlock === undefined || messageBlock === null) return undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/test-utils.ts` around lines 571 - 586, In waitForL1ToL2Message, the current truthy check "if (!messageBlock)" treats a valid block number 0 as absent; change this to an explicit nullish check (e.g. "if (messageBlock == null)") when inspecting the result of aztecNode.getL1ToL2MessageBlock so that a returned 0 is handled as a valid block number before comparing with aztecNode.getProvenBlockNumber().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 24-29: Add a concurrency guard to the publish job so
manual/parallel runs are serialized: under the jobs.publish block (the job with
name "Build & Publish"), add a concurrency stanza such as concurrency: group:
publish-${{ github.ref }} cancel-in-progress: false so runs for the same ref
wait for the prior publish to finish instead of racing; place it at the same
indentation level as runs-on/steps within the publish job.
- Around line 30-37: Replace the three external action tag refs with immutable
commit SHAs to prevent tag retargeting: locate the uses entries for
actions/checkout@v4, actions/setup-node@v4, and noir-lang/noirup@v0.1.4 in the
publish workflow and update each to the corresponding full commit SHA (e.g.,
actions/checkout@<full-sha>, actions/setup-node@<full-sha>,
noir-lang/noirup@<full-sha>), keeping the same inputs (node-version,
registry-url) intact; verify the SHAs against the upstream repos/tags and commit
the updated workflow.
---
Duplicate comments:
In `@e2e-tests/test-utils.ts`:
- Around line 571-586: In waitForL1ToL2Message, the current truthy check "if
(!messageBlock)" treats a valid block number 0 as absent; change this to an
explicit nullish check (e.g. "if (messageBlock == null)") when inspecting the
result of aztecNode.getL1ToL2MessageBlock so that a returned 0 is handled as a
valid block number before comparing with aztecNode.getProvenBlockNumber().
In `@ts/aztec-state-migration/wallet/migration-base-wallet.ts`:
- Around line 190-198: The loop over noteByTxHash is making sequential awaits to
this.getPrivateEvents for each tx; refactor to run fetches in parallel by
mapping noteByTxHash entries to promises and awaiting them with Promise.all (or
Promise.allSettled) so multiple calls to getPrivateEvents (using eventSelector,
abiType and eventFilter(txHash)) execute concurrently; ensure you preserve the
association between txHash and its events when collecting results and handle
errors per-promise if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/publish.ymle2e-tests/deploy.tse2e-tests/test-utils.tsts/aztec-state-migration/mode-b/proofs.tsts/aztec-state-migration/wallet/entrypoints/browser.tsts/aztec-state-migration/wallet/migration-base-wallet.ts
JanKuczma
changed the title
This was referenced Mar 4, 2026
Merged
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
3 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
Refactor
Breaking Changes