feat(premigration): improve security, performance, and reliability#259
feat(premigration): improve security, performance, and reliability#259
Conversation
hiddentao
commented
Mar 31, 2026
hiddentao
commented
- Support private key via PREMIGRATION_PRIVATE_KEY env var to avoid exposing secrets in process listings and shell history
- Replace sequential RPC calls with viem multicall batching, reducing 2*N round-trips per batch to just 2
- Add binary-search batch fallback to isolate failing names in ~log2(N) transactions instead of N individual ones
- Add pre-flight gas estimation with dynamic batch splitting when estimated gas exceeds 80% of block gas limit
- Add comprehensive e2e integration tests covering env var auth, multicall verification, batch sizing, checkpoint tracking, and edge cases (34 tests, all passing)
- Fix pre-existing TS errors in migration.test.ts (abitype import, hex string types)
- Support private key via PREMIGRATION_PRIVATE_KEY env var to avoid exposing secrets in process listings and shell history - Replace sequential RPC calls with viem multicall batching, reducing 2*N round-trips per batch to just 2 - Add binary-search batch fallback to isolate failing names in ~log2(N) transactions instead of N individual ones - Add pre-flight gas estimation with dynamic batch splitting when estimated gas exceeds 80% of block gas limit - Add comprehensive e2e integration tests covering env var auth, multicall verification, batch sizing, checkpoint tracking, and edge cases (34 tests, all passing) - Fix pre-existing TS errors in migration.test.ts (abitype import, hex string types)
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
Snapshot totalProcessed before the verification loop so that in-loop increments for skipped/failed items no longer double-advance the displayed progress index.
…rsing Increase premigration e2e test timeout from 30s to 60s to account for slow gas estimation on CI runners. Also fix --min-expiry-days 0 being incorrectly treated as 7 due to falsy zero in `parseInt() || 7`.
Add isValidLabel() to reject labels exceeding the 255-byte on-chain
limit and encoded labelhash representations ([hex{64}]) whose plaintext
is unknown. Both the CSV batch filter and verifyNameOnV1() now use this
shared validation function.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dfa64cfd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mid = Math.ceil(labels.length / 2); | ||
| const leftResult = await estimateAndSplitBatch( | ||
| batchRegistrar, | ||
| client, | ||
| resolver, |
There was a problem hiding this comment.
When estimatedGas is above maxGas for a batch of exactly one label, mid becomes 1 and the left recursive call receives labels.slice(0, 1), i.e. the same input again. Because there is no labels.length <= 1 base case in this over-limit path, the function can recurse indefinitely and eventually hang/overflow instead of reporting the label as failed; this is reachable on chains with lower block gas limits or unusually expensive single-name registrations.
Useful? React with 👍 / 👎.
| const [v2Results, v1Results] = await Promise.all([ | ||
| client.multicall({ contracts: v2Contracts }), | ||
| mainnetClient.multicall({ contracts: v1Contracts }), |
There was a problem hiding this comment.
These multicalls are awaited as a single Promise.all with no local recovery, so any transport/provider-level error (e.g., timeout, rate limiting, or oversized eth_call response) rejects the whole verification step before per-name error accounting runs. Since the caller rethrows batch errors, one transient RPC failure now aborts the entire migration run instead of isolating the affected names and preserving forward progress.
Useful? React with 👍 / 👎.
3 participants
