EXSC-712: perf(safe): prefetch the next network's Safe context during confirm-safe-tx - #2133
Conversation
…afe-tx confirm-safe-tx.ts prepared each network serially and inline: initialize the Safe client, reconcile in-flight Mongo rows, then augment and decode every tx. The operator waited through that setup between networks, on top of a fresh Safe client per network and RPC round-trips for signature state. - extract per-network preparation into confirm-safe-tx-prefetch.ts behind a ConfirmSafeTxPrefetchQueue: network N+1 is scheduled before N is handed to the interactive loop, so its setup overlaps the operator's review of N - pool Safe clients (getOrInitializeSafeClient / releaseAllPooledSafeClients) instead of building a fresh one per network and per deployer-Safe branch - derive signature state from the Mongo document (getSignatureCountFromDoc, isDocSignedBySigner, hasEnoughSignaturesInDoc) instead of re-querying RPC - add read-only-safe-client.ts for Safe reads that need no signer - warm the deployment cache in parallel with the bulk pending-tx fetch No change to what is signed, executed or persisted: the Ledger Flex filmstrip still binds verifyingContract to the client-derived safeAddress, and tx selection, nonce sequencing and Mongo writes are untouched.
|
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:
WalkthroughSafe confirmation now prepares per-network contexts in advance, reuses pooled Safe clients and transactions, evaluates actionability from read-only contract data and persisted signatures, parallelizes reconciliation, and caches deployment and selector lookups. ChangesSafe confirmation flow
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (5)
script/deploy/safe/safe-utils.ts (2)
1786-1842: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueActionability logic is equivalent, but the second branch is redundant.
Reaching Line 1833 already implies
getSignatureCountFromDoc(tx) < threshold(otherwisecanExecutewould have short-circuited), so the condition reduces to!hasSignedAlready. Worth simplifying so the intent — executable, or not yet signed by me — reads directly.The read-only migration itself preserves semantics: owners/threshold still come from the same
SAFE_SINGLETON_ABIreads, and the error path still surfaces throughPromise.allSettled.♻️ Collapse the branches
let hasActionableTx = false for (const tx of networkTxs) { - const hasSignedAlready = isDocSignedBySigner(tx, signerAddress) - const canExecute = hasEnoughSignaturesInDoc(tx, threshold) - - if (canExecute) { - hasActionableTx = true - break - } - - if (!hasSignedAlready && getSignatureCountFromDoc(tx) < threshold) { + // Actionable when the operator can execute it, or has not signed it yet + if ( + hasEnoughSignaturesInDoc(tx, threshold) || + !isDocSignedBySigner(tx, signerAddress) + ) { hasActionableTx = true break } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 1786 - 1842, In the transaction loop, simplify the second actionability condition after the canExecute check to test only !hasSignedAlready, removing the redundant getSignatureCountFromDoc(tx) < threshold check. Preserve the existing executable branch and return behavior so actionability remains “executable or not yet signed by me.”
1744-1751: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour dead parameters remain on an exported signature.
The signer/ledger params are no longer read now that actionability is derived read-only. Keeping them preserves the positional contract, but leaves a trap:
rpcUrlis the 8th positional arg, so any future removal silently shifts it. Consider dropping them and updating the single caller (confirm-safe-tx.tsaround line 856), or switching to an options object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 1744 - 1751, Remove the unused signer and ledger parameters from the exported function signature, including _privateKey, _useLedger, _ledgerOptions, and _account, then update the sole caller in confirm-safe-tx.ts to pass rpcUrl and subsequent arguments in the correct positions. Prefer an options object if needed to avoid future positional-argument shifts.script/deploy/safe/read-only-safe-client.ts (1)
14-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead-only clients skip the shared transport config (retries/timeouts).
SafeClient.initbuilds its transport viagetTransportConfigFromRpcUrl(provider)(retryCount, retryDelay, fetchOptions), butbuildReadOnlyClientpasses the raw URL tohttp(). Reconciliation and actionability checks now run all their reads through this client, so they silently lose the project's RPC retry/timeout tuning.Also note: when
rpcUrlis supplied for a Tron network, the/jsonrpcsuffix normalization applied bygetViemChainForNetworkNameis bypassed.♻️ Reuse the shared transport config
-import { createPublicClient, http, type PublicClient } from 'viem' - -import { getViemChainForNetworkName } from '../../utils/viemScriptHelpers' +import { createPublicClient, http, type PublicClient } from 'viem' + +import { getViemChainForNetworkName } from '../../utils/viemScriptHelpers' +import { getTransportConfigFromRpcUrl } from './safe-utils' /** Builds a read-only viem client for a network, honoring an optional RPC override. */ export function buildReadOnlyClient( network: string, rpcUrl?: string ): PublicClient { + const chain = getViemChainForNetworkName(network) + const url = rpcUrl ?? chain.rpcUrls.default.http[0] + const { fetchOptions, retryCount, retryDelay } = + getTransportConfigFromRpcUrl(url) return createPublicClient({ - chain: getViemChainForNetworkName(network), - transport: http(rpcUrl), + chain, + transport: http(url, { + ...(fetchOptions ? { fetchOptions } : {}), + ...(retryCount !== undefined ? { retryCount } : {}), + ...(retryDelay !== undefined ? { retryDelay } : {}), + }), }) as PublicClient }(Watch for an import cycle — if
getTransportConfigFromRpcUrllives insafe-utils.ts, move it to a shared module instead.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/read-only-safe-client.ts` around lines 14 - 17, Update buildReadOnlyClient to construct its HTTP transport through the shared getTransportConfigFromRpcUrl configuration used by SafeClient.init, preserving retryCount, retryDelay, and fetchOptions. Reuse the shared transport helper without introducing an import cycle, and ensure the supplied rpcUrl receives the same network-specific normalization, including Tron /jsonrpc handling, before creating the client.script/deploy/safe/confirm-safe-tx.ts (1)
958-961: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win"No actionable pending transactions" message can mask an underlying prefetch failure.
prepareConfirmSafeTxNetworkreturnsnullboth when there's genuinely nothing to sign and when an owner/threshold/nonce RPC read failed (alreadyconsola.warn'd internally). Following a warning with aconsola.success"No actionable..." message on the same network can make a real failure look like an expected no-op to the operator.Consider having
prepareConfirmSafeTxNetworkcommunicate failure vs. legitimate no-op distinctly (e.g. a discriminated result) so this branch can log accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/confirm-safe-tx.ts` around lines 958 - 961, Update prepareConfirmSafeTxNetwork and its caller to distinguish a genuine no-op from owner/threshold/nonce prefetch failure, using a discriminated result or equivalent explicit status. In the branch currently guarded by !prepared, retain the success message only for the no-op outcome and log the failure outcome without presenting it as an expected absence of actionable transactions.script/deploy/safe/reconcile.ts (1)
259-287: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReconcile failure logs lose the offending network's identity.
Promise.allSettledcorrectly isolates per-group failures, but the rejection branch only logsresult.reason's message — thenetwork/chainId/safeAddressof the failed group isn't included, making it hard to tell which network failed reconciliation when several run concurrently.🔧 Proposed fix — tag the error with its network before it's thrown
const covered = new Set<string>() const results = await Promise.allSettled( [...groups.entries()].map( async ([key, { network, chainId, safeAddress }]) => { - const client = clientFactory(network) - const onChainNonce = await nonceReader(client, safeAddress) - await reconcileSubmittedSafeTxs( - pendingTransactions, - client, - network, - chainId, - safeAddress, - onChainNonce, - options - ) - return key + try { + const client = clientFactory(network) + const onChainNonce = await nonceReader(client, safeAddress) + await reconcileSubmittedSafeTxs( + pendingTransactions, + client, + network, + chainId, + safeAddress, + onChainNonce, + options + ) + return key + } catch (error: unknown) { + const msg = error instanceof Error ? error.message : String(error) + throw new Error(`[${network}] ${msg}`) + } } ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/reconcile.ts` around lines 259 - 287, Update the per-group callback in the Promise.allSettled map to catch reconciliation errors and rethrow or wrap them with the group’s network, chainId, and safeAddress. Keep the existing rejection logging in the results loop, ensuring the resulting consola.warn message identifies the failed group along with the original error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/safe/confirm-safe-tx.ts`:
- Around line 925-963: The prefetchQueue scheduling for nextNetwork currently
starts Ledger initialization before the active network’s signing flow completes.
In the loop around prefetchQueue.take and processTxs, move the next-network
prefetch scheduling until after await processTxs returns, while preserving the
existing network and transaction checks and queue behavior.
In `@script/deploy/safe/safe-decode-utils.ts`:
- Around line 579-608: Update lookupDiamondFunctionName to build
diamondSelectorNameCache from the ABI returned by getDiamondAbi(), reusing its
existing cached loading and parsing behavior. Remove the direct diamond.json
path, file read, JSON parse, and related error handling, while preserving
selector-to-function-name mapping and lookup behavior.
- Around line 642-650: The fourByteSelectorCache hit path in
decodeTransactionData must preserve the full decoded function result, including
decodedData.args. Update the cache to store and retrieve the complete decoded
metadata for 4byte/Sourcify resolutions, and ensure cache hits return the same
decodedData shape as the initial resolution instead of reconstructing it from
only cachedName.
In `@script/deploy/safe/safe-utils.ts`:
- Around line 2183-2187: Update the error path in the selector-map loading
function around the catch block so transient read or parse exceptions do not
assign null to cachedDiamondSelectorMap. Continue logging the error and return
null for that invocation, while leaving null caching unchanged for legitimate
missing-file or invalid-shape paths so later calls can retry after transient
failures.
- Around line 1604-1614: Update safeClientPoolKey to derive the account
identifier from privateKey using the imported privateKeyToAccount helper, then
use the resulting lowercased address in accountPart. Remove the raw private-key
prefix fallback while preserving the existing account.address and ledger
behavior.
- Around line 1638-1652: Update the safe client initialization flow around
safeClientPool and initializeSafeClient so a rejected initialization promise
removes its matching entry from the pool before propagating the error. Only
evict the entry if it still references the failed promise, preserving newer
successful or in-flight replacements and allowing later calls to retry after
transient failures.
---
Nitpick comments:
In `@script/deploy/safe/confirm-safe-tx.ts`:
- Around line 958-961: Update prepareConfirmSafeTxNetwork and its caller to
distinguish a genuine no-op from owner/threshold/nonce prefetch failure, using a
discriminated result or equivalent explicit status. In the branch currently
guarded by !prepared, retain the success message only for the no-op outcome and
log the failure outcome without presenting it as an expected absence of
actionable transactions.
In `@script/deploy/safe/read-only-safe-client.ts`:
- Around line 14-17: Update buildReadOnlyClient to construct its HTTP transport
through the shared getTransportConfigFromRpcUrl configuration used by
SafeClient.init, preserving retryCount, retryDelay, and fetchOptions. Reuse the
shared transport helper without introducing an import cycle, and ensure the
supplied rpcUrl receives the same network-specific normalization, including Tron
/jsonrpc handling, before creating the client.
In `@script/deploy/safe/reconcile.ts`:
- Around line 259-287: Update the per-group callback in the Promise.allSettled
map to catch reconciliation errors and rethrow or wrap them with the group’s
network, chainId, and safeAddress. Keep the existing rejection logging in the
results loop, ensuring the resulting consola.warn message identifies the failed
group along with the original error message.
In `@script/deploy/safe/safe-utils.ts`:
- Around line 1786-1842: In the transaction loop, simplify the second
actionability condition after the canExecute check to test only
!hasSignedAlready, removing the redundant getSignatureCountFromDoc(tx) <
threshold check. Preserve the existing executable branch and return behavior so
actionability remains “executable or not yet signed by me.”
- Around line 1744-1751: Remove the unused signer and ledger parameters from the
exported function signature, including _privateKey, _useLedger, _ledgerOptions,
and _account, then update the sole caller in confirm-safe-tx.ts to pass rpcUrl
and subsequent arguments in the correct positions. Prefer an options object if
needed to avoid future positional-argument shifts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19e4e162-7497-466b-b38c-9a04a5874b79
📒 Files selected for processing (7)
script/deploy/safe/confirm-safe-tx-prefetch.tsscript/deploy/safe/confirm-safe-tx.tsscript/deploy/safe/facet-version-utils.tsscript/deploy/safe/read-only-safe-client.tsscript/deploy/safe/reconcile.tsscript/deploy/safe/safe-decode-utils.tsscript/deploy/safe/safe-utils.ts
0xDEnYO
changed the title
🔍 QA Review — EXSC-712 / PR #2133 (Post-Approval Re-Review — Run #29)Ticket: EXSC-712 — perf(safe): prefetch next network's Safe context in confirm-safe-tx
Post-Approval Commit Reviewed
Code Review1. Chain-ID fail-closed (
|
| # | Severity | Item |
|---|---|---|
| A1 | 🟢 Low | read-only-safe-client.ts — no direct unit tests; exercised indirectly through prefetch-queue integration tests |
| A2 | 🟢 Low | getSignatureCountFromDoc, isDocSignedBySigner, hasEnoughSignaturesInDoc doc helpers — pure functions, no direct unit tests |
| A3 | 🟢 Low | JSDoc tag density: getOrCreatePooledPromise and ConfirmSafeTxPrefetchQueue.schedule/take use prose-only documentation vs. the file's full @param/@returns style |
These are Low advisory items and do not block merge for a TypeScript-only performance improvement to an internal developer tool.
🛡️ Audit State
No src/ Solidity contracts were modified. AuditNotRequired is correct. ✅
✅ Verdict: Pass (Post-Approval Re-Review)
Commit f20147acb302 is a clean hardening of the prefetch infrastructure:
- Chain-ID mismatch now fails closed ← security improvement (advisory → fix)
- Prefetch re-validation is now semantics-preserving (failure kinds re-prepare inline at
take()) - Deployment-log refresh correctly restored to unconditional
- Two minor test improvements (temp file cleanup, NaN concurrency assertion)
All prior approvals remain valid. No new blocking issues introduced.
QA re-review by lifi-qa-agent[bot] — Run #29 · 2026-07-31
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟠 Medium | Test gap | script/deploy/safe/confirm-safe-tx-prefetch.test.ts (new) |
| 2 | 🟠 Medium | Test gap | script/deploy/safe/safe-utils.test.ts |
| 3 | 🟠 Medium | Code | Pool poisoning: rejected init promise never evicted from safeClientPool |
1. [Medium] Test gap — confirm-safe-tx-prefetch.test.ts (new)
- Missing:
ConfirmSafeTxPrefetchQueue.schedule()deduplication (second call for same key is a no-op) - Missing:
take()consumes inflight entry and returns result - Missing:
take()without priorschedule()falls back toprepareConfirmSafeTxNetworkdirectly - Missing:
schedule()/take()error swallowing (internal throw returnsnull, not unhandled)
This is an explicit AC requirement per the ticket: "Unit tests added forConfirmSafeTxPrefetchQueue".
2. [Medium] Test gap — safe-utils.test.ts
- Missing:
getOrInitializeSafeClientdeduplication (two calls with same key reuse the same promise) - Missing:
releaseAllPooledSafeClientsdrains pool and callscleanup()on each client - Missing: pool poisoning case — rejected init is currently never evicted (see item 3)
This is an explicit AC requirement per the ticket: "Unit tests added for the Safe client pool".
3. [Medium] Code — Pool poisoning in safe-utils.ts getOrInitializeSafeClient
safeClientPool.set(key, promise) is called before the promise settles. If initializeSafeClient rejects (transient RPC failure, DNS blip), the rejected promise remains in the pool forever. Every subsequent call for that (network, safeAddress, signer) triple immediately rethrows the original rejection — the operator cannot retry without restarting the process.
Fix: evict the pool entry on rejection:
const promise = initializeSafeClient(...).catch((err) => {
if (safeClientPool.get(key) === promise) safeClientPool.delete(key)
throw err
})
safeClientPool.set(key, promise)(The === promise guard prevents evicting a concurrent replacement that succeeded.)
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
…yte lookups (EXSC-712) Function-name resolution in the signing UI paid an HTTP round trip to 4byte.sourcify.dev for every selector outside diamond.json — including Timelock scheduleBatch on every run — and diamondCut display resolved unknown selectors one sequential request at a time, with only in-memory caching. - selector-registry.ts: merged local-first selector map from diamond.json, config/clearSigningProposal.json (98 diamond signatures), the config/whitelist.json signature harvest, and a well-known list (LI.FI admin fns, Timelock, AccessControl, ERC20, Safe management). Local hits never touch the network. - Remaining unknowns resolve through one batched, comma-separated 4byte request (openchain-compatible API) instead of per-selector calls, and positive results persist to .cache/selector-signatures.json so a selector hits the network at most once per machine. Misses are only remembered in-process, so a later 4byte DB addition is still picked up. - decodeTransactionData and decodeDiamondCut now consult the registry first; the single-selector lookup helper in safe-utils is superseded and removed. Display strings, dispatch (diamondCut / scheduleBatch / whitelist / role formatting) and what is signed are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p (EXSC-712) - Pool key derives the signer address via privateKeyToAccount instead of embedding a raw private-key fragment (heap-dump exposure + weak discriminator). - A rejected initializeSafeClient promise evicts itself from the pool so a transient RPC/Ledger failure no longer poisons every later call for that network. - createSelectorMap no longer memoizes null on thrown errors — transient IO faults must not permanently disable selector decoding for the run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…EXSC-712) Closes the QA review's AC-2/AC-3 gaps and remaining advisories: - confirm-safe-tx-prefetch.test.ts: schedule dedup (case-insensitive), take-consumes-inflight, direct-take fallback, error-to-null swallowing on both paths, null passthrough, parallel independence. The queue now takes an injectable prepare fn (defaults to the real one) to make this testable. - Pool coverage in safe-utils.test.ts: the pooling core is extracted into a generic getOrCreatePooledPromise (dedup, self-eviction on rejection, stale-rejection guard); safeClientPoolKey is exported and tested to contain the derived address and no key material; releaseAllPooledSafeClients takes an injectable pool and is tested for cleanup + clear + error tolerance. - confirm-safe-tx.ts: documented the invariant that prefetchParamsBase must carry the startup Ledger account so prefetch never opens a second Ledger transport during signing (QA finding 5). - reconcile.ts: startup-reconcile failure logs regained network identity lost in the parallel refactor (QA finding 7). - take() logs how long the caller waited (prefetched vs inline) so the upcoming real-world multi-network run captures AC-5 timing evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@lifi-qa-agent status update on the review findings (commits 8345a05 + 21fff34):
AC-4 / AC-5 — explicitly deferred with acceptance: real Safe + Ledger verification across multiple networks and before/after timing will be captured during the upcoming Symbiosis/Mayan shipping run. 🤖 Generated with Claude Code |
github-actions
Bot
added
QA AI Reviewing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
script/deploy/safe/safe-utils.ts (1)
1849-1863: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant actionability check.
getSignatureCountFromDoc(tx) < thresholdon Line 1859 is logically the same condition as!canExecutecomputed two lines above (both should express "does not yet have enough signatures"). Keeping two independently-computed expressions for the same fact risks silent divergence if either helper's semantics change later.♻️ Reuse the already-computed `canExecute` flag
- if (!hasSignedAlready && getSignatureCountFromDoc(tx) < threshold) { + if (!hasSignedAlready && !canExecute) { hasActionableTx = true break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 1849 - 1863, In the network transaction loop, update the second actionability condition to reuse the existing canExecute result instead of calling getSignatureCountFromDoc(tx) again. Preserve the hasSignedAlready check and the current behavior for executable and not-yet-signed transactions.script/deploy/safe/safe-decode-utils.ts (1)
601-637: 📐 Maintainability & Code Quality | 🔵 TrivialDefensive: normalize
selectorbefore the 4byte-fallback map lookup.
resolveSelectorsViaFourBytealways keys its returned map bynormalizeSelector(...)(lowercased,0x-prefixed), but Line 632 doesresolved.get(selector)with the rawdata.substring(0, 10)value. If the incomingdatais ever not already lowercase, the lookup would silently miss a value that was in fact resolved and persisted to disk. Low current risk sinceHexvalues here are conventionally lowercase, but a cheap safeguard avoids surprises if that assumption ever breaks.🛡️ Proposed defensive fix
- const resolved = await resolveSelectorsViaFourByte([selector]) - const signature = resolved.get(selector) + const resolved = await resolveSelectorsViaFourByte([selector]) + const signature = resolved.get(selector.toLowerCase())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-decode-utils.ts` around lines 601 - 637, Normalize selector before looking it up in the map returned by resolveSelectorsViaFourByte, ensuring the key is lowercase and 0x-prefixed consistently with that resolver. Update the resolved.get lookup in the fallback path while preserving the existing signature return behavior.script/deploy/safe/selector-registry.test.ts (1)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest couples to live
config/clearSigningProposal.jsoncontent.This test asserts a specific function name (
startBridgeTokensViaAcrossV4ERC20Packed) sourced from the realconfig/clearSigningProposal.jsonfile rather than a fixture. Unrelated future edits to that config could cause this test to fail with no code regression. SincegetLocalRegistry()/readJsonConfig()don't currently accept an injectable base path, hardening this would require adding a similar override hook toresolveSelectorsViaFourByte'scachePathoption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/selector-registry.test.ts` around lines 38 - 45, The test around getLocalSelectorInfo should use an isolated fixture rather than the live config/clearSigningProposal.json content. Add a cachePath/base-path override to resolveSelectorsViaFourByte and thread it through getLocalRegistry/readJsonConfig as needed, then point the test fixture at that override while preserving the existing selector assertion.script/deploy/safe/selector-registry.ts (3)
177-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
diamond.jsonread/parse vs.getDiamondAbi().
buildSelectorMapFromDiamondJsonindependently reads and parsesdiamond.json, duplicating the read already done bygetDiamondAbi()insafe-decode-utils.ts. The file's docstring explains this is intentional to keep the module dependency-light and avoid a cycle (sincesafe-decode-utils.tsnow imports this module). A small shared leaf module (purefs.readFileSync+JSON.parse, no other deps) that bothgetDiamondAbi()and this function import would remove the duplication without reintroducing a cycle.As per path instructions, "Prefer reusing existing helpers/utilities under
script/utils/(and existingscript/deploy/**patterns) instead of duplicating logic."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/selector-registry.ts` around lines 177 - 200, Extract the shared diamond.json read/parse logic used by buildSelectorMapFromDiamondJson and getDiamondAbi into a dependency-light leaf helper under script/utils/, containing only the necessary filesystem read and JSON.parse behavior. Update both functions to reuse this helper, preserving their existing empty/invalid-file handling and avoiding a dependency cycle.Source: Path instructions
354-373: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid unconditional disk-cache read when all selectors already resolve from
runCache.
readDiskCache(cachePath)runs on every call, even when everyselectorinnormalizedis already answered byrunCache(the common case for a hot decode path calling this once per unresolved selector, per transaction). Making the read lazy — only when a selector actually falls through to the disk-cache check — avoids repeated full-file reads/JSON parses on a path this PR specifically optimizes for low latency.⚡ Proposed lazy-read fix
const runCache = getRunCache(cachePath) - const diskCache = readDiskCache(cachePath) const toFetch: string[] = [] + let diskCache: Record<string, string> | undefined for (const selector of normalized) { const inRun = runCache.get(selector) if (typeof inRun === 'string') { resolved.set(selector, inRun) continue } if (inRun === null) continue // known miss this run - const onDisk = diskCache[selector] + const onDisk = (diskCache ??= readDiskCache(cachePath))[selector] if (onDisk) { resolved.set(selector, onDisk) runCache.set(selector, onDisk) continue } toFetch.push(selector) }Note:
diskCacheis also referenced later inwriteDiskCache(cachePath, { ...diskCache, ... }); guard that spread withdiskCache ?? {}if it staysundefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/selector-registry.ts` around lines 354 - 373, Make the disk-cache load in the selector resolution flow lazy: initialize diskCache only when a selector misses runCache and reaches the disk-cache lookup, while preserving existing cache behavior. Update the later writeDiskCache merge to safely use an empty object when diskCache was never loaded.
337-408: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNo in-flight dedup for concurrent 4byte lookups of the same selector.
resolveSelectorsViaFourByteonly checks completed cache entries (runCache/disk) before deciding what to fetch; there's no promise coalescing for calls that are already in-flight for the same selector. If this is invoked concurrently for the same unresolved selector — plausible given this PR's parallel prefetch/reconciliation architecture — both callers will independently hit the network, contradicting the stated "hits the network at most once per machine" design goal. Not a correctness bug (writes are idempotent), but worth confirming whether concurrent invocation is actually possible from the callers inconfirm-safe-tx.ts/facet-version-utils.ts/safe-utils.ts(not in this file set), and if so, adding an in-flightMap<string, Promise<...>>similar toConfirmSafeTxPrefetchQueue's pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/selector-registry.ts` around lines 337 - 408, Update resolveSelectorsViaFourByte to coalesce concurrent lookups for the same normalized selector by maintaining a module-level in-flight Map keyed by selector and reusing existing promises before issuing fetches. Ensure each selector’s promise is removed from the map after completion while preserving current cache updates, error handling, and returned Map behavior.script/deploy/safe/safe-decode-utils.test.ts (1)
115-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the diamond.json branch and 4byte-fallback branch of
decodeTransactionDatadirectly.This test only exercises the "well-known signature" branch. The
local.source === 'diamond.json'branch (bare-name +contractName: 'Diamond'shape) and the 4byte-fallback branch (mockedfetchreturning a signature) are exercised indirectly viaselector-registry.test.ts, but the actual shape/branching logic insidedecodeTransactionDataitself isn't directly verified in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-decode-utils.test.ts` around lines 115 - 133, Extend the decodeTransactionData tests to directly cover both remaining resolution paths: a diamond.json registry entry returning the bare function name with contractName "Diamond", and an unknown selector that uses mocked fetch to resolve a 4byte signature. Keep the existing network-disabled test for the well-known selector and assert each branch’s expected decoded result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/safe/safe-utils.test.ts`:
- Around line 546-592: Update the test cleanup in the finally block surrounding
decodeDiamondCut to delete the temporary selector-cache file created via
SELECTOR_SIGNATURE_CACHE_PATH. Preserve restoration of globalThis.fetch and the
environment variable, and ensure file removal runs even when the test fails.
---
Nitpick comments:
In `@script/deploy/safe/safe-decode-utils.test.ts`:
- Around line 115-133: Extend the decodeTransactionData tests to directly cover
both remaining resolution paths: a diamond.json registry entry returning the
bare function name with contractName "Diamond", and an unknown selector that
uses mocked fetch to resolve a 4byte signature. Keep the existing
network-disabled test for the well-known selector and assert each branch’s
expected decoded result.
In `@script/deploy/safe/safe-decode-utils.ts`:
- Around line 601-637: Normalize selector before looking it up in the map
returned by resolveSelectorsViaFourByte, ensuring the key is lowercase and
0x-prefixed consistently with that resolver. Update the resolved.get lookup in
the fallback path while preserving the existing signature return behavior.
In `@script/deploy/safe/safe-utils.ts`:
- Around line 1849-1863: In the network transaction loop, update the second
actionability condition to reuse the existing canExecute result instead of
calling getSignatureCountFromDoc(tx) again. Preserve the hasSignedAlready check
and the current behavior for executable and not-yet-signed transactions.
In `@script/deploy/safe/selector-registry.test.ts`:
- Around line 38-45: The test around getLocalSelectorInfo should use an isolated
fixture rather than the live config/clearSigningProposal.json content. Add a
cachePath/base-path override to resolveSelectorsViaFourByte and thread it
through getLocalRegistry/readJsonConfig as needed, then point the test fixture
at that override while preserving the existing selector assertion.
In `@script/deploy/safe/selector-registry.ts`:
- Around line 177-200: Extract the shared diamond.json read/parse logic used by
buildSelectorMapFromDiamondJson and getDiamondAbi into a dependency-light leaf
helper under script/utils/, containing only the necessary filesystem read and
JSON.parse behavior. Update both functions to reuse this helper, preserving
their existing empty/invalid-file handling and avoiding a dependency cycle.
- Around line 354-373: Make the disk-cache load in the selector resolution flow
lazy: initialize diskCache only when a selector misses runCache and reaches the
disk-cache lookup, while preserving existing cache behavior. Update the later
writeDiskCache merge to safely use an empty object when diskCache was never
loaded.
- Around line 337-408: Update resolveSelectorsViaFourByte to coalesce concurrent
lookups for the same normalized selector by maintaining a module-level in-flight
Map keyed by selector and reusing existing promises before issuing fetches.
Ensure each selector’s promise is removed from the map after completion while
preserving current cache updates, error handling, and returned Map behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4284620-fdad-4625-aa6e-66f52352e8c7
📒 Files selected for processing (10)
script/deploy/safe/confirm-safe-tx-prefetch.test.tsscript/deploy/safe/confirm-safe-tx-prefetch.tsscript/deploy/safe/confirm-safe-tx.tsscript/deploy/safe/reconcile.tsscript/deploy/safe/safe-decode-utils.test.tsscript/deploy/safe/safe-decode-utils.tsscript/deploy/safe/safe-utils.test.tsscript/deploy/safe/safe-utils.tsscript/deploy/safe/selector-registry.test.tsscript/deploy/safe/selector-registry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- script/deploy/safe/reconcile.ts
- script/deploy/safe/confirm-safe-tx-prefetch.ts
- script/deploy/safe/confirm-safe-tx.ts
…712) Follow-up to the prefetch/selector-resolution work, addressing review feedback: - Re-validate the Safe's on-chain nonce in ConfirmSafeTxPrefetchQueue.take(): a prefetched context computed while the operator reviewed the previous network is discarded and re-prepared inline if another signer executed in the meantime, so stale nonce/reconcile state can't drive a signing or execution decision. - Return a discriminated result from prepareConfirmSafeTxNetwork instead of a bare null: not-owner (with signer + owner list), owner-check-failed and read-failed are logged/aborted distinctly instead of all surfacing as a green "No actionable transactions" success. The threshold/nonce read failure keeps its abort semantics. - Verify harvested selector→signature pairs (whitelist.json, disk cache, 4byte responses) actually hash to the claimed selector before displaying them, so a hand-edited typo or poisoned cache degrades to "unknown selector". - facet-version-utils: stop memoizing null on a transient read/parse fault; only missing-file / invalid-shape outcomes are cached. - Drop the dead _privateKey/_useLedger/_ledgerOptions/_account params from getNetworksWithActionableTransactions and its caller. - Bound reconcileAllSubmittedSafeTxs parallelism (extracted a shared mapWithConcurrency util, reused in healthCheckAllNetworks). - Warn on RPC-vs-config chain-id mismatch when a global --rpc-url override is in play; bound pooled-client cleanup with a timeout so a hung init can't stall shutdown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/safe/confirm-safe-tx-prefetch.ts (1)
128-155: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFilter the post-reconcile refetch by the actual Safe address.
pendingTxsis rebuilt with{ network: networkKey, status: 'pending' }after reconciliation, but the later augmentation is scoped aroundtxSafeAddress. AddsafeAddressto that query so any stale or legacy pending rows for another Safe on the same network cannot be brought into this context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/confirm-safe-tx-prefetch.ts` around lines 128 - 155, The post-reconciliation pending transaction refetch is not scoped to the current Safe address. Update the find query assigned to pendingTxs in the reconcileSubmittedSafeTxs flow to include safeAddress alongside network and status filters, preserving the existing augmentation context.
🧹 Nitpick comments (4)
script/deploy/safe/facet-version-utils.test.ts (1)
248-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse suite hooks for the temporary fixture lifecycle.
Move temporary-directory setup into
beforeEachand cleanup intoafterEachso fixture management is consistent and reusable as more cases are added. The currenttry/finallycleanup is safe, so this is a maintainability refactor.As per coding guidelines, test fixtures should use
beforeEachandafterEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/facet-version-utils.test.ts` around lines 248 - 292, Refactor the temporary fixture lifecycle in the test suite by moving flakyRoot creation and cache-directory setup into a beforeEach hook, and moving recursive cleanup into an afterEach hook. Update the test around getDeployedFacetVersionFromLog to reuse the hook-provided fixture path and remove its local try/finally cleanup, preserving the existing assertions and file contents.Source: Coding guidelines
script/utils/mapWithConcurrency.test.ts (2)
10-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to TypeScript callbacks.
The new
describe,it, mapper, and Promise executor callbacks omit return annotations. Annotate them consistently, such as(): Promise<void>for async tests and(): voidfor synchronous callbacks.As per coding guidelines, TypeScript functions should use explicit return types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/utils/mapWithConcurrency.test.ts` around lines 10 - 72, Add explicit return types to every callback in the mapWithConcurrency tests, including describe and it callbacks, mapper functions, Promise executors, and setTimeout callbacks. Use Promise<void> for async test callbacks, void for synchronous callbacks, and the appropriate resolved-value Promise type for async mappers that return strings or numbers.Source: Coding guidelines
10-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for
undefineditems andNaNlimits.The suite currently covers dense arrays and a zero limit, but not
mapWithConcurrency([undefined], ...)ormapWithConcurrency(items, Number.NaN, ...). Add cases ensuring the mapper still runs and results remain populated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/utils/mapWithConcurrency.test.ts` around lines 10 - 72, Add regression tests in the mapWithConcurrency suite for an input containing undefined, verifying the mapper executes and the output is populated, and for a Number.NaN concurrency limit, verifying items are still processed and results are returned. Keep the assertions focused on mapper invocation and result contents, alongside the existing boundary-condition tests.script/deploy/safe/confirm-safe-tx-prefetch.ts (1)
157-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
.then()chain withawait.
Promise.all(...).then((augmented) => augmented.filter(...))mixes.then()with the surroundingasync/awaitstyle.♻️ Proposed refactor
- const txs = await Promise.all( + const augmentedTxs = await Promise.all( pendingTxs.map( async (tx: ISafeTxDocument): Promise<IAugmentedSafeTxDocument> => { const safeTransaction = await initializeSafeTransaction(tx, safe) const hasSignedAlready = isSignedByCurrentSigner( safeTransaction, signerAddress ) const canExecute = hasEnoughSignatures(safeTransaction, threshold) return { ...tx, safeTransaction, hasSignedAlready, canExecute, threshold, } } ) - ).then((augmented) => - augmented.filter((tx) => { - if (tx.canExecute) return true - if (tx.hasSignedAlready) return false - return tx.safeTransaction.signatures.size < tx.threshold - }) - ) + ) + const txs = augmentedTxs.filter((tx) => { + if (tx.canExecute) return true + if (tx.hasSignedAlready) return false + return tx.safeTransaction.signatures.size < tx.threshold + })As per coding guidelines, "Always use async/await for promises instead of .then() chains."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/confirm-safe-tx-prefetch.ts` around lines 157 - 182, Update the pending transaction processing around Promise.all to use await for the resolved augmented transactions, then apply the existing filter in a separate step. Preserve the current filtering behavior for canExecute, hasSignedAlready, and signature thresholds while removing the .then() chain.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/utils/mapWithConcurrency.ts`:
- Around line 31-33: Update the item-processing logic in mapWithConcurrency so
mapper is invoked for every index, including entries whose value is undefined.
Remove the item === undefined skip while preserving the existing indexed
assignment to results and mapper arguments.
- Around line 37-39: Normalize the concurrency limit before calculating the
worker count in mapWithConcurrency: treat a NaN limit as the documented minimum
of one, then preserve the existing Math.max/Math.min bounds so Array.from always
creates at least one worker and valid limits remain capped by items.length.
---
Outside diff comments:
In `@script/deploy/safe/confirm-safe-tx-prefetch.ts`:
- Around line 128-155: The post-reconciliation pending transaction refetch is
not scoped to the current Safe address. Update the find query assigned to
pendingTxs in the reconcileSubmittedSafeTxs flow to include safeAddress
alongside network and status filters, preserving the existing augmentation
context.
---
Nitpick comments:
In `@script/deploy/safe/confirm-safe-tx-prefetch.ts`:
- Around line 157-182: Update the pending transaction processing around
Promise.all to use await for the resolved augmented transactions, then apply the
existing filter in a separate step. Preserve the current filtering behavior for
canExecute, hasSignedAlready, and signature thresholds while removing the
.then() chain.
In `@script/deploy/safe/facet-version-utils.test.ts`:
- Around line 248-292: Refactor the temporary fixture lifecycle in the test
suite by moving flakyRoot creation and cache-directory setup into a beforeEach
hook, and moving recursive cleanup into an afterEach hook. Update the test
around getDeployedFacetVersionFromLog to reuse the hook-provided fixture path
and remove its local try/finally cleanup, preserving the existing assertions and
file contents.
In `@script/utils/mapWithConcurrency.test.ts`:
- Around line 10-72: Add explicit return types to every callback in the
mapWithConcurrency tests, including describe and it callbacks, mapper functions,
Promise executors, and setTimeout callbacks. Use Promise<void> for async test
callbacks, void for synchronous callbacks, and the appropriate resolved-value
Promise type for async mappers that return strings or numbers.
- Around line 10-72: Add regression tests in the mapWithConcurrency suite for an
input containing undefined, verifying the mapper executes and the output is
populated, and for a Number.NaN concurrency limit, verifying items are still
processed and results are returned. Keep the assertions focused on mapper
invocation and result contents, alongside the existing boundary-condition tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d183440-edd2-4d08-acfd-e090a4f677b6
📒 Files selected for processing (14)
script/deploy/healthCheckAllNetworks.tsscript/deploy/safe/confirm-safe-tx-prefetch.test.tsscript/deploy/safe/confirm-safe-tx-prefetch.tsscript/deploy/safe/confirm-safe-tx.tsscript/deploy/safe/facet-version-utils.test.tsscript/deploy/safe/facet-version-utils.tsscript/deploy/safe/reconcile.test.tsscript/deploy/safe/reconcile.tsscript/deploy/safe/safe-utils.test.tsscript/deploy/safe/safe-utils.tsscript/deploy/safe/selector-registry.test.tsscript/deploy/safe/selector-registry.tsscript/utils/mapWithConcurrency.test.tsscript/utils/mapWithConcurrency.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- script/deploy/safe/facet-version-utils.ts
- script/deploy/safe/safe-utils.test.ts
- script/deploy/safe/selector-registry.ts
- script/deploy/safe/safe-utils.ts
- script/deploy/safe/confirm-safe-tx.ts
…t (EXSC-712) Address CodeRabbit review on the extracted shared util: - Pass through undefined item values instead of skipping them (skipping left a hole in results and never invoked the mapper for that index). - Floor a NaN concurrency limit to one worker so results are always filled. Both were inherited from the original healthCheckAllNetworks copy, where items were always network-name strings, so neither path was reachable there. As a generic util they must be correct. Not reachable from current callers either (concurrency is a literal or an int-with-fallback), but covered by tests now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/utils/mapWithConcurrency.test.ts`:
- Around line 59-66: Strengthen the NaN case in the mapWithConcurrency test by
tracking the number of in-flight mapper calls, using an awaited delay to allow
overlap, and recording the peak concurrency. In the test “treats a NaN limit as
a single worker and still fills every result,” assert that the peak in-flight
count is exactly one while preserving the existing output assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0fff7ea-ae35-4bde-b224-0c1ab3d55ee3
📒 Files selected for processing (2)
script/utils/mapWithConcurrency.test.tsscript/utils/mapWithConcurrency.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- script/utils/mapWithConcurrency.ts
There was a problem hiding this comment.
QA Pass (Run #24, post-approval re-review) — 2 new commits: mapWithConcurrency hardening (undefined passthrough + NaN guard, with tests) and confirm-safe-tx prefetch improvements (discriminated union result type, bounded reconcile concurrency, Ledger transport separation, transient-failure memoization fix, selector hash-verification guard). No Solidity changes.
The deployment-cache warm-up depends only on MONGODB_URI, so kicking it off right after the Mongo connection lets it overlap the reconcile sweep and ownership filtering instead of only the pending-tx query. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0023e1f
The only RPC between the operator's Sign selection and the Ledger prompt was signTransaction's eth_chainId. With prefetch, the network's client does all its chain reads while the previous network is being reviewed, so that call now lands on a cold connection and delays the device display. Resolve the chain id from config at init (same pattern and cross-check as createChainExecutor) so the sign hot path makes no RPC call; fall back to the RPC read when no config id is known. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
script/deploy/safe/safe-utils.ts (2)
233-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
knownChainIdas readonly.This field is assigned only in the constructor and never reassigned. Declare it as
private readonly knownChainId?: numberto prevent accidental mutation.As per coding guidelines, use
readonlyfor properties that should not be modified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 233 - 252, Mark the Safe utility’s knownChainId property as readonly, changing its declaration from private knownChainId?: number to private readonly knownChainId?: number; leave the constructor assignment unchanged.Source: Coding guidelines
295-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
async/awaitfor the background RPC check.Replace the new
.then(...).catch(...)chain with a non-blocking async helper/IIFE usingawaitandtry/catch.As per coding guidelines, always use async/await for promises instead of
.then()chains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/deploy/safe/safe-utils.ts` around lines 295 - 304, Replace the promise chain in the configChainId RPC validation with a non-blocking async helper or IIFE that awaits publicClient.getChainId() inside try/catch. Preserve the existing mismatch warning and silently ignore RPC errors, while keeping the check conditional on configChainId being defined.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/deploy/safe/safe-utils.ts`:
- Around line 400-424: Update SafeClient.init’s configChainId resolution to
obtain the chain ID directly from network configuration without invoking
getViemChainForNetworkName, which also requires the RPC environment variable.
Preserve the resolved chain ID when an explicit provider or --rpc-url is
supplied, and distinguish missing optional configuration from an invalid network
instead of silently clearing configChainId.
- Around line 274-275: Update the chain-ID handling around configChainId and
publicClient.getChainId to synchronously validate the configured chain against
the live RPC chain before signing or execution. Cache or await the validation
result, and abort on mismatch rather than only logging a warning; ensure Tron
paths perform this validation before returning. Apply the same fail-closed
behavior to the related flows at the other chain-ID handling sites.
---
Nitpick comments:
In `@script/deploy/safe/safe-utils.ts`:
- Around line 233-252: Mark the Safe utility’s knownChainId property as
readonly, changing its declaration from private knownChainId?: number to private
readonly knownChainId?: number; leave the constructor assignment unchanged.
- Around line 295-304: Replace the promise chain in the configChainId RPC
validation with a non-blocking async helper or IIFE that awaits
publicClient.getChainId() inside try/catch. Preserve the existing mismatch
warning and silently ignore RPC errors, while keeping the check conditional on
configChainId being defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32fa77fa-3850-412c-8aa3-200d29bb32dd
📒 Files selected for processing (3)
script/deploy/safe/confirm-safe-tx.tsscript/deploy/safe/safe-utils.test.tsscript/deploy/safe/safe-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- script/deploy/safe/safe-utils.test.ts
- script/deploy/safe/confirm-safe-tx.ts
There was a problem hiding this comment.
✅ QA approved (Run #27, post-approval re-review). 2 new commits: deploy-log refresh parallelization (safe) + EIP-712 chain ID from config (valid optimization; mismatch is immediately visible at Safe execution). Advisory: async chain-ID verification log. (lifi-qa-agent[bot])
…n-id mismatch (EXSC-712) - retry stale prefetched failure results inline at take() instead of surfacing an error that may be minutes old; a failed nonce re-read now re-prepares instead of trusting the unvalidated context, and a nonce-advance re-prepare clears the startup reconcile coverage so the executed rows are back-filled - abort SafeClient init when the RPC's live chain id contradicts the config chain id (a wrong-chain Safe tx hash could otherwise be signed); unreachable RPC stays non-fatal - resolve the config chain id straight from networks.json so the sign-path optimization survives an explicit provider URL without the RPC env var - restore the unconditional deployment-log refresh (get() was TTL-gated and could hide a facet deployed minutes ago from the signing UI) - test hygiene: clean up the temp selector-cache file, assert single-worker peak for a NaN concurrency limit - docs: correct stale JSDoc/comments, document SELECTOR_SIGNATURE_CACHE_PATH in .env.example, note selector-registry.ts in rule 201 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Gate-review residual (overnight run, commit f20147a)Adversarial review + bot triage completed; safe findings were fixed in f20147a (see commit message). Items below need a human decision or were deliberately left: Behavior changes made per review findings — please sanity-check:
Known-and-accepted (not fixed):
Lower-confidence — human judgment:
|
0xDEnYO
left a comment
0xDEnYO
left a comment
There was a problem hiding this comment.
Re-triggering protect-critical-code — it only runs on pull_request_review and had not reported on the current head commit.
3 participants
Which Linear task belongs to this PR?
EXSC-712
Why did I implement it this way?
confirm-safe-tx.tsprepares each network serially and inline — Safe client init,Mongo reconcile, tx augment/decode — so the signer waits through that setup between
every network of a multi-chain session. Network N+1's prep doesn't depend on anything
the operator does on network N, so it can overlap their review instead of following it.
confirm-safe-tx-prefetch.ts+ConfirmSafeTxPrefetchQueue— per-network prep isextracted and scheduled one network ahead. It lives in its own module because the
old code interleaved preparation and prompting in a single function, which is what
made "prepare ahead" inexpressible.
getOrInitializeSafeClient/releaseAllPooledSafeClients)rather than rebuilt per network and per deployer-Safe branch.
nonce and threshold are still read from chain.
read-only-safe-client.tsgives signer-less Safe reads, so prefetch doesn't need anunlocked Ledger.
Unchanged: what is signed, executed or persisted. Tx selection, nonce sequencing,
threshold logic and Mongo writes are untouched, and the Ledger Flex filmstrip still
binds
verifyingContractto the client-derivedsafeAddress(not the document'stxSafeAddress— the two are distinct and only the former is what the device shows).Review focus: that
safeAddressbinding, and the pool's release on every exit pathincluding abort.
Update (2nd commit): local-first selector resolution. Function-name resolution
was the remaining slow spot: every selector outside diamond.json (e.g. Timelock
scheduleBatchon every run) paid an HTTP round trip to 4byte.sourcify.dev, anddiamondCut display resolved unknown selectors one sequential request at a time.
selector-registry.tsnow merges a local selector map from diamond.json,config/clearSigningProposal.json(98 diamond signatures), theconfig/whitelist.jsonsignature harvest, and a well-known list (LI.FI adminfns, Timelock, AccessControl, ERC20, Safe management) — local hits never touch
the network. Remaining unknowns go out as ONE batched comma-separated 4byte
request, and positive results persist to
.cache/selector-signatures.json(gitignored), so a selector hits the network at most once per machine. Display
strings, dispatch and what is signed are unchanged; covered by
selector-registry.test.tsplus new offline-decode tests.Checklist before requesting a review
bun test script/deploy/safe/→ 377 pass / 0 fail;tsc-filesandeslintclean.Tests now cover
ConfirmSafeTxPrefetchQueue(via injected prepare fn), the clientpool (
getOrCreatePooledPromisededup/eviction,safeClientPoolKey,releaseAllPooledSafeClients) and the selector registry (21 tests incl.offline-decode proofs). Outstanding, deliberately deferred: verification against a
real Safe + Ledger across multiple networks (AC-4) and before/after timing (AC-5) —
both will be captured during the upcoming Symbiosis/Mayan shipping run;
take()logs prefetched-vs-inline wait times to make that measurement automatic.Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)