Add #968 upgrade-in-place deploy scripts for the five L1 depositor proxies#979
Open
lrsaturnino wants to merge 17 commits into
Open
Add #968 upgrade-in-place deploy scripts for the five L1 depositor proxies#979lrsaturnino wants to merge 17 commits into
lrsaturnino wants to merge 17 commits into
Conversation
Sui consumed the published @keep-network/tbtc-v2 BTCDepositorWormhole copy via hardhat-dependency-compiler, which predates the best-effort tx-max-fee reimbursement implementation. Add a copyBTCDepositorWormholeArtifact helper that stages the locally built BTCDepositorWormhole artifact (seeding an empty validation log so the OZ validation entry merges into a package that compiles no local upgradeable source), wire it into the Sui hardhat config via a TASK_COMPILE override mirroring the Arbitrum rail, drop the dependencyCompiler npm path, and repoint the deploy contractName onto the staged local artifact. This makes the Sui rail resolve the best-effort source so the upgrade script targets the correct implementation. The Arbitrum/Base staging path is unchanged (append-only).
… test Author the StarkNet upgrade-in-place script (03_upgrade_starknet_btc_ depositor_to_968.ts) that prepares a fresh #968 StarkNetBitcoinDepositor implementation for the existing proxy, emits (never broadcasts) the ProxyAdmin.upgrade calldata, Etherscan-verifies the new impl, and refreshes the committed artifact ABI. The implementation compiles from the vendored tree, which already carries #968 (best-effort tx-max-fee reimbursement). Add a new mainnet-fork regression test asserting, across the simulated upgrade, that the post-slot-200 starkGateBridge field is byte-unchanged (read via the public getter to avoid the slot-206 packing footgun with the #968 reimburse flag), that the implementation slot advances to a new address, and that the upgraded impl exhibits the #968 best-effort skip path. Register TestERC20 in the dependency compiler so the stubbed skip-path case can back SafeERC20 plumbing with a real mintable token. No CI workflow exists for the StarkNet rail, so the fork test must be run locally with FORKING_URL set; this is noted for the PR.
Adds the upgrade-in-place deploy script for the Native gasless BTCDepositor proxy to ship the best-effort tx-max-fee reimbursement change. The script emits the ProxyAdmin.upgrade(proxy, newImpl) calldata for the Council Safe to execute and verifies the new implementation on Etherscan; it does not broadcast the upgrade. Native has no committed deployment artifact and its proxy is absent from the OpenZeppelin upgrades manifest, so the script takes the proxy address explicitly and the regression test forceImports it before prepareUpgrade. The mainnet-fork test runs against an anvil fork (Hardhat built-in forking cannot initialize against the available RPC, which omits totalDifficulty) and asserts reimburseTxMaxFee survives the implementation swap byte-for-byte and that the new implementation differs from the current one.
The shared Wormhole V2 artifact staging skipped the OpenZeppelin upgrade-validation merge whenever a same-named entry was already present in the target cache, and skipped entirely when the target cache was absent. Both cases left prepareUpgrade unable to resolve a freshly staged implementation on a re-upgrade (same contract, new build). Seed the target validation log when missing and merge every source entry describing the contract, deduped by serialized identity. The OZ plugin selects the matching entry by bytecode hash, so carrying older entries alongside the new one is harmless.
Adds the upgrade-in-place deploy script for the Arbitrum L1 depositor proxy to ship the best-effort tx-max-fee reimbursement change. It emits the ProxyAdmin.upgrade calldata for the 24h timelock owner to execute and verifies the new implementation; it does not broadcast the upgrade. Adds a mainnet-fork regression test run against an anvil fork via a new system_tests network (Hardhat built-in forking cannot initialize against the available RPC, which omits totalDifficulty). It asserts the l2ChainId and l2BitcoinDepositor getter values are preserved across the simulated upgrade and that the new implementation differs from the current one. prepareUpgrade allows custom types because the unchanged DepositState enum cannot be auto-compared against the recorded baseline; storage preservation is verified empirically by the fork assertions.
Adds the upgrade-in-place deploy script for the Base L1 depositor proxy to ship the best-effort tx-max-fee reimbursement change. It emits the ProxyAdmin.upgrade calldata for the 24h timelock owner (shared with Arbitrum) to execute and verifies the new implementation; it does not broadcast the upgrade. The Base variant carries a +1 storage-slot offset (tbtcToken at slot 201, not 200) and is compiled from L1BTCDepositorWormholeV2Base, never the Arbitrum variant. Adds a mainnet-fork regression test run against an anvil fork via a system_tests network. It asserts the packed l2ChainId/l2WormholeGateway slot (205) and l2BitcoinDepositor slot (206) are byte-identical across the simulated upgrade, the implementation advances to a new distinct address, and the upgraded implementation ABI exposes DepositTxMaxFeeReimbursementSkipped (the behavioral skip path is covered by the package unit tests). prepareUpgrade allows custom types because the unchanged DepositState enum cannot be auto-compared against the recorded baseline.
…kages Packages that consume a staged upgradeable artifact but compile no local upgradeable source (Sui) never have the OpenZeppelin upgrades plugin write a versioned validations.json. The staged-artifact merge seeded an empty log with no top-level version field, which readValidations rejects as outdated, so prepareUpgrade could not resolve the staged implementation on a fork. Backfill the version the consuming plugin expects (3.4 for the upgrades-core releases these packages pin) whenever it is absent; the merged 3.3-sourced entries are accepted under a 3.4 header, and packages that already carry a version (Arbitrum/Base) are untouched.
Add the Sui L1 BTCDepositorWormhole upgrade-in-place deploy script. It deploys a fresh best-effort-reimbursement implementation, emits — without broadcasting — the governance ProxyAdmin.upgrade(proxy, newImpl) calldata for the owning EOA to execute, updates the deployment artifact, and Etherscan-verifies the new implementation. Sui resolves the shared BTCDepositorWormhole from the local solidity build staged into the package, not the pre-best-effort published npm copy. Add a mainnet-fork regression test that, across a simulated ProxyAdmin.upgrade, asserts the post-gap storage (destinationChainId / destinationChainWormholeGateway) is byte-identical and that the upgraded implementation ships the DepositTxMaxFeeReimbursementSkipped change. The test runs against an external anvil fork node, since Hardhat's built-in forking cannot initialize against an RPC that omits totalDifficulty.
…Upgrade
The StarkNet depositor links an external library, and its unchanged DepositState enum cannot be auto-compared against the recorded baseline ("insufficient data to compare enums"). Pass unsafeAllow: ["external-library-linking"] and unsafeAllowCustomTypes to prepareUpgrade in both the deploy script and the fork regression test so the storage-safety check runs; the starkGateBridge raw-slot assertion remains the storage-integrity guard.
|
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:
📝 WalkthroughWalkthroughAdds prepareUpgrade deploy scripts for Arbitrum, Base, StarkNet, Sui, and Native rails; improves shared artifact staging/validations and Sui compile staging; patches ethers v5 receipt formatting for empty ChangesCross-Chain Depositor Upgrade Rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ESLint's no-extra-semi rejected the leading-semicolon idiom used to defensively guard the conditional describe gate under the repo's no-semicolon style, which failed the cross-chain lint check. Assign the fork-gated describe to a const instead: this removes the flagged empty statement while preserving the skip-when-not-forking behavior (verified — the fork block reports as pending without FORKING_URL). Applies to the Arbitrum, Base, StarkNet, and Sui upgrade fork tests.
The Native #968 upgrade fork regression ran unconditionally and its before-hook threw when the proxy had no code, so CI — which runs without a mainnet fork — failed the suite instead of skipping it. Gate the live-fork suite on FORKING_URL, matching the cross-chain upgrade tests, so it skips cleanly without a fork and runs only when one is configured.
lrsaturnino
requested review from
Shadowfiend,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cross-chain/arbitrum/hardhat.config.ts (1)
15-18:⚠️ Potential issue | 🟠 MajorCI:
contracts-slitherneeds to build the Solidity package before running Slither for Arbitrum
cross-chain/arbitrum/hardhat.config.tsextendsTASK_COMPILEto callcopyWormholeV2Artifact, which requires Solidity build outputs (e.g.,L1BTCDepositorWormholeV2Arbitrum). In.github/workflows/cross-chain-arbitrum.yml, thecontracts-slitherjob runsslither --hardhat-artifacts-directory build .but does not run the./solidity“Build solidity package” step that exists incontracts-build-and-test/contracts-format, causing the missing-artifact failure. Add the Solidity build step (or job dependency/artifact handoff) tocontracts-slither.🤖 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 `@cross-chain/arbitrum/hardhat.config.ts` around lines 15 - 18, The CI failure happens because the contracts-slither job runs Slither against hardhat artifacts but never builds the Solidity package needed by copyWormholeV2Artifact (which is invoked from the TASK_COMPILE override in cross-chain/arbitrum/hardhat.config.ts and expects artifacts like L1BTCDepositorWormholeV2Arbitrum); update the cross-chain-arbitrum.yml workflow to run the same "Build solidity package" step (or add a dependency on the contracts-build-and-test job or pass built artifacts) before the contracts-slither job so that hardhat build outputs exist for Slither to consume.Source: Pipeline failures
🧹 Nitpick comments (2)
cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts (1)
11-19: 💤 Low valueGlobal prototype mutation for ethers Formatter persists beyond this script.
The patch to
providers.Formatter.prototype.transactionResponsemodifies a global prototype that remains in effect for the entire process. While this is intentional to work around RPC provider quirks, it could affect other scripts running in the same process. The workaround is justified by the comment, but worth noting this isn't scoped to just this operation.🤖 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 `@cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts` around lines 11 - 19, The global mutation of providers.Formatter.prototype.transactionResponse (stored as originalFormat) should be scoped and reverted after the upgrade to avoid affecting other scripts; wrap the code that needs the patched behavior (the prepareUpgrade/deploy flow that expects tx.to === "" handling) in a try/finally: set providers.Formatter.prototype.transactionResponse to the patched function before the operation, perform the prepareUpgrade/deploy calls, and in the finally block restore providers.Formatter.prototype.transactionResponse = originalFormat so the global prototype is restored even on errors.cross-chain/common/copyWormholeV2Artifact.ts (1)
104-108: 💤 Low valuePartial-match risk with
key.includes(contractName)substring check.The check
key.includes(contractName)could match unintended entries if contract names are substrings of each other (e.g.,"BTCDepositorWormhole"is a substring of"L1BTCDepositorWormholeV2Arbitrum"). Currently safe because Sui and Arbitrum/Base use separate packages with isolatedvalidations.jsonfiles, but this could cause silent over-inclusion if the package structure changes.Consider using a stricter boundary match if this becomes a concern:
const describesContract = Object.keys(entry).some( (key) => key.includes(`:${contractName}`) || key.endsWith(`/${contractName}`) )🤖 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 `@cross-chain/common/copyWormholeV2Artifact.ts` around lines 104 - 108, The current substring check in the loop that builds describesContract (using key.includes(contractName)) can over-match when contract names are substrings of one another; update the test on Object.keys(entry) so it only matches true boundaries — for example treat keys as namespaced strings and require either a prefix/symbol before the contract name (e.g., a colon) or that the key ends with a path segment equal to contractName — replace the includes(contractName) usage in the describesContract computation with this stricter boundary-aware check so only exact namespaced or path-segment matches are accepted.
🤖 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 `@cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts`:
- Around line 15-19: The script patches
providers.Formatter.prototype.transactionResponse by assigning originalFormat
then never restoring it; wrap the deployment logic that relies on this patch in
a try/finally so you always restore
providers.Formatter.prototype.transactionResponse = originalFormat in the
finally block; locate the assignment to
providers.Formatter.prototype.transactionResponse and the saved originalFormat
and surround the subsequent deploy/execute steps with try { ... } finally {
providers.Formatter.prototype.transactionResponse = originalFormat } so the
global prototype is reverted even on errors.
In `@cross-chain/base/hardhat.config.ts`:
- Around line 46-49: The current Hardhat forking config assigns blockNumber
directly from parseInt(process.env.FORKING_BLOCK, 10) which can yield NaN and
break initialization; update the blockNumber logic where the config is built
(the blockNumber assignment using process.env.FORKING_BLOCK) to explicitly
validate that process.env.FORKING_BLOCK, when provided, parses to a finite
non-negative integer (use Number.isFinite/Number.isInteger checks on the parsed
value and ensure >= 0) and throw a clear Error if validation fails; only pass
the validated numeric value to blockNumber or undefined when the env var is
absent.
In `@cross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.ts`:
- Around line 252-258: The test dereferences verifyCalls[0] before checking its
length; move the assertion assert.equal(verifyCalls.length, 1) to before the
assignment to firstCall (or assert verifyCalls.length > 0) so you never index an
empty array, then assign firstCall from verifyCalls[0] and continue to assert
firstCall.taskName === "verify"; update references to verifyCalls and firstCall
accordingly.
In `@cross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.ts`:
- Around line 15-19: The current monkey-patch of
providers.Formatter.prototype.transactionResponse (using originalFormat and
overriding transactionResponse) mutates a global prototype and can stack if the
task runs multiple times; instead, limit the change to your provider instance by
wrapping or decorating the provider method that returns transactions (e.g., the
provider’s getTransaction/getTransactionReceipt or sendTransaction wrapper) and
sanitize the returned tx (inspect tx.to and replace "" with null) before
returning, or apply the originalFormat override only temporarily and restore it
after use; update code to stop modifying providers.Formatter.prototype globally
and target the provider instance or call-site that produces the tx.
In
`@cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts`:
- Around line 64-67: The test currently hardcodes CURRENT_IMPLEMENTATION and
PROXY_ADMIN_OWNER; instead read the live pre-upgrade implementation from
EIP1967_IMPLEMENTATION_SLOT (e.g., readSlot on the proxy address) before calling
prepareUpgrade and use proxyAdminInstance.owner() to fetch the current owner to
impersonate; replace usages of CURRENT_IMPLEMENTATION and PROXY_ADMIN_OWNER in
the assertions and impersonation code (and the other occurrences around the
second and third blocks noted) so the test derives the implementation and admin
dynamically at runtime rather than asserting against fixed addresses.
---
Outside diff comments:
In `@cross-chain/arbitrum/hardhat.config.ts`:
- Around line 15-18: The CI failure happens because the contracts-slither job
runs Slither against hardhat artifacts but never builds the Solidity package
needed by copyWormholeV2Artifact (which is invoked from the TASK_COMPILE
override in cross-chain/arbitrum/hardhat.config.ts and expects artifacts like
L1BTCDepositorWormholeV2Arbitrum); update the cross-chain-arbitrum.yml workflow
to run the same "Build solidity package" step (or add a dependency on the
contracts-build-and-test job or pass built artifacts) before the
contracts-slither job so that hardhat build outputs exist for Slither to
consume.
---
Nitpick comments:
In `@cross-chain/common/copyWormholeV2Artifact.ts`:
- Around line 104-108: The current substring check in the loop that builds
describesContract (using key.includes(contractName)) can over-match when
contract names are substrings of one another; update the test on
Object.keys(entry) so it only matches true boundaries — for example treat keys
as namespaced strings and require either a prefix/symbol before the contract
name (e.g., a colon) or that the key ends with a path segment equal to
contractName — replace the includes(contractName) usage in the describesContract
computation with this stricter boundary-aware check so only exact namespaced or
path-segment matches are accepted.
In `@cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts`:
- Around line 11-19: The global mutation of
providers.Formatter.prototype.transactionResponse (stored as originalFormat)
should be scoped and reverted after the upgrade to avoid affecting other
scripts; wrap the code that needs the patched behavior (the
prepareUpgrade/deploy flow that expects tx.to === "" handling) in a try/finally:
set providers.Formatter.prototype.transactionResponse to the patched function
before the operation, perform the prepareUpgrade/deploy calls, and in the
finally block restore providers.Formatter.prototype.transactionResponse =
originalFormat so the global prototype is restored even on errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfa9ae0c-e5c8-495a-bbea-d7b70f9976d3
📒 Files selected for processing (18)
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.tscross-chain/arbitrum/hardhat.config.tscross-chain/arbitrum/test/UpgradeArbitrumL1BitcoinDepositorTo968.verify.test.tscross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.tscross-chain/base/hardhat.config.tscross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.tscross-chain/common/copyWormholeV2Artifact.tscross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.tscross-chain/starknet/hardhat.config.tscross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.tscross-chain/sui/deploy_l1/00_deploy_sui_btc_depositor_wormhole.tscross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.tscross-chain/sui/hardhat.config.tscross-chain/sui/test/CopyWormholeV2Artifact.sui.test.tscross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.tssolidity/deploy/44_upgrade_native_btc_depositor_to_968.tssolidity/test/depositor/UpgradeNativeBTCDepositorTo968.test.tssolidity/test/depositor/UpgradeNativeBTCDepositorTo968.verify.test.ts
Comment on lines
+15
to
+19
| const originalFormat = providers.Formatter.prototype.transactionResponse | ||
| providers.Formatter.prototype.transactionResponse = function (tx: any): any { | ||
| const patched = tx.to === "" ? { ...tx, to: null } : tx | ||
| return originalFormat.call(this, patched) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant portion of the file
FILE="cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (lines 1-220) ----"
nl -ba "$FILE" | sed -n '1,220p'
echo "---- end ----"
# Extract any other occurrences of the patched symbol in the repo (to see if restoration exists elsewhere)
rg -n "Formatter\.prototype\.transactionResponse" cross-chain/base/deploy_l1 "$FILE" || true
rg -n "transactionResponse\s*=" cross-chain/base/deploy_l1 "$FILE" || trueRepository: threshold-network/tbtc-v2
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (relevant section) ----"
# Print the first ~120 lines with line numbers (cat -n is more widely available than nl)
cat -n "$FILE" | sed -n '1,140p'
echo "---- occurrences of transactionResponse patch in deploy_l1 ----"
rg -n "Formatter\.prototype\.transactionResponse" cross-chain/base/deploy_l1 "$FILE" || true
echo "---- occurrences of restoration patterns (assignment back to originalFormat / delete / finally) ----"
rg -n "originalFormat\s*$|transactionResponse\s*=\s*originalFormat|finally\s*[\{]|transactionResponse\s*=\s*original|delete\s+providers\.Formatter\.prototype\.transactionResponse" cross-chain/base/deploy_l1 "$FILE" || true
echo "---- repo-wide uses of Formatter.prototype.transactionResponse ----"
rg -n "Formatter\.prototype\.transactionResponse" . || trueRepository: threshold-network/tbtc-v2
Length of output: 8825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
cross-chain/base/deploy_l1/02_upgrade_base_l1_bitcoin_depositor_to_v2.ts \
cross-chain/base/deploy_l1/02_upgrade_base_l1_bitcoin_depositor_to_v2.ts \
cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts \
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts \
cross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.ts \
solidity/deploy/44_upgrade_native_btc_depositor_to_968.ts \
solidity/deploy/86_deploy_tip109_hotfix.ts
do
if [ -f "$f" ]; then
echo "==== $f ===="
# Print around the patch area only
line=$(rg -n "providers\.Formatter\.prototype\.transactionResponse = function" "$f" | cut -d: -f1 | head -n1 || true)
if [ -n "${line:-}" ]; then
start=$((line-10))
end=$((line+30))
sed -n "${start},${end}p" "$f" | cat -n
else
echo "No patch assignment found."
fi
fi
doneRepository: threshold-network/tbtc-v2
Length of output: 14794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo-wide: assignment back to originalFormat =="
rg -n "transactionResponse\s*=\s*originalFormat|finally[\s\S]{0,200}?transactionResponse\s*=\s*originalFormat" . || true
echo "== repo-wide: delete patch pattern =="
rg -n "delete\s+providers\.Formatter\.prototype\.transactionResponse" . || true
echo "== repo-wide: originalFormat variable used for restoration =="
rg -n "const\s+originalFormat\s*=\s*providers\.Formatter\.prototype\.transactionResponse" . || trueRepository: threshold-network/tbtc-v2
Length of output: 1723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts
cross-chain/base/deploy_l1/02_upgrade_base_l1_bitcoin_depositor_to_v2.ts
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts
cross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.ts
solidity/deploy/44_upgrade_native_btc_depositor_to_968.ts
solidity/deploy/86_deploy_tip109_hotfix.ts
cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts
)
echo "== Any try/finally in these files? =="
for f in "${files[@]}"; do
echo "--- $f ---"
rg -n "finally\s*\{" "$f" || true
done
echo "== Any reassignment of transactionResponse after patch? =="
rg -n "providers\.Formatter\.prototype\.transactionResponse\s*=" "${files[@]}" || true
echo "== Any deletion of transactionResponse patch? =="
rg -n "delete\s+providers\.Formatter\.prototype\.transactionResponse" . || trueRepository: threshold-network/tbtc-v2
Length of output: 1793
Restore the global ethers Formatter patch
cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts overwrites providers.Formatter.prototype.transactionResponse (lines 15-19) and never restores it (no try/finally/reassignment back to originalFormat). Because this mutates a global prototype, the rest of the Hardhat-deploy process can keep the altered behavior if the script continues after patching or errors later. Restore providers.Formatter.prototype.transactionResponse = originalFormat in a finally block around the deploy 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 `@cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts`
around lines 15 - 19, The script patches
providers.Formatter.prototype.transactionResponse by assigning originalFormat
then never restoring it; wrap the deployment logic that relies on this patch in
a try/finally so you always restore
providers.Formatter.prototype.transactionResponse = originalFormat in the
finally block; locate the assignment to
providers.Formatter.prototype.transactionResponse and the saved originalFormat
and surround the subsequent deploy/execute steps with try { ... } finally {
providers.Formatter.prototype.transactionResponse = originalFormat } so the
global prototype is reverted even on errors.
Comment on lines
+46
to
+49
| blockNumber: | ||
| process.env.FORKING_BLOCK !== undefined | ||
| ? parseInt(process.env.FORKING_BLOCK, 10) | ||
| : undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show the relevant section with line numbers
FILE="cross-chain/base/hardhat.config.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: threshold-network/tbtc-v2
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cross-chain/base/hardhat.config.ts"
[ -f "$FILE" ] || { echo "Missing file: $FILE" >&2; exit 1; }
echo "=== File head (with line numbers) ==="
cat -n "$FILE" | sed -n '1,120p'
echo "=== Focus around FORKING_BLOCK ==="
cat -n "$FILE" | rg -n "FORKING_BLOCK|forking|blockNumber" -n || true
echo "=== Exact lines 35-80 ==="
cat -n "$FILE" | sed -n '35,80p'Repository: threshold-network/tbtc-v2
Length of output: 6560
🌐 Web query:
Hardhat forking blockNumber expects number behavior when NaN parseInt invalid
💡 Result:
In Hardhat, the forking.blockNumber configuration field expects a number or bigint (as of recent updates to ensure type consistency) [1][2]. Providing an invalid value, such as NaN (which results from parseInt() on an invalid string), will trigger a validation error during Hardhat's configuration initialization or when the hardhat_reset JSON-RPC method is called [1][3][4]. Key behavioral details include: 1. Type Validation: Hardhat uses Zod schemas to validate configuration inputs [1][4]. If blockNumber is set to NaN or another invalid type, the internal validation logic will reject the configuration, typically throwing an error that identifies the type mismatch [1][4]. 2. hardhat_reset Method: When using hardhat_reset to dynamically change the fork block, passing an invalid value (like NaN) through the JSON-RPC interface will cause the network to fail to reset, often returning a JSON-RPC error [5][3]. 3. Normalization: Recent updates have moved toward normalizing blockNumber to bigint internally across both network and Solidity test configurations to handle numeric inputs consistently [1]. 4. Best Practices: Always ensure that any block number passed to the configuration or the hardhat_reset method is a valid, positive integer. If parsing a block number from an environment variable or external source, verify it with a check such as Number.isSafeInteger() before passing it to Hardhat to avoid NaN errors [3]. If you encounter unexpected behavior where blockNumber is ignored or causes issues, ensure you are interacting with the correct network (e.g., using --network localhost instead of the default hardhat network when resetting) [6].
Citations:
- 1: Use consistent types for forking.blockNumber in config NomicFoundation/hardhat#8114
- 2: https://hardhat.org/docs/reference/configuration.md
- 3: hardhat_reset blockNumber bug NomicFoundation/hardhat#2409
- 4: https://cdn.jsdelivr.net/npm/hardhat@3.0.9/src/internal/builtin-plugins/network-manager/type-validation.ts
- 5: hardhat_reset behavior in edr@0.12.0 NomicFoundation/hardhat#7834
- 6: hardhat_reset does not reset forked chain NomicFoundation/hardhat#2354
Validate FORKING_BLOCK before assigning Hardhat forking blockNumber (cross-chain/base/hardhat.config.ts:46-49)
parseInt(process.env.FORKING_BLOCK, 10) can produce NaN for malformed input; that invalid value gets passed to Hardhat’s fork config and causes a config/init failure. Add an explicit check that FORKING_BLOCK parses to a non-negative integer (and throw a clear error if not).
🤖 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 `@cross-chain/base/hardhat.config.ts` around lines 46 - 49, The current Hardhat
forking config assigns blockNumber directly from
parseInt(process.env.FORKING_BLOCK, 10) which can yield NaN and break
initialization; update the blockNumber logic where the config is built (the
blockNumber assignment using process.env.FORKING_BLOCK) to explicitly validate
that process.env.FORKING_BLOCK, when provided, parses to a finite non-negative
integer (use Number.isFinite/Number.isInteger checks on the parsed value and
ensure >= 0) and throw a clear Error if validation fails; only pass the
validated numeric value to blockNumber or undefined when the env var is absent.
Comment on lines
+252
to
+258
| const firstCall = verifyCalls[0] as { | ||
| taskName: string | ||
| args: { address: string; constructorArgsParams: unknown[] } | ||
| } | ||
|
|
||
| assert.equal(verifyCalls.length, 1) | ||
| assert.equal(firstCall.taskName, "verify") |
There was a problem hiding this comment.
Assert call count before dereferencing verifyCalls[0]
At Line 252, verifyCalls[0] is accessed before validating length. If no call happens, this fails with a type error instead of a clean assertion failure.
Proposed fix
- const firstCall = verifyCalls[0] as {
- taskName: string
- args: { address: string; constructorArgsParams: unknown[] }
- }
-
assert.equal(verifyCalls.length, 1)
+ const firstCall = verifyCalls[0] as {
+ taskName: string
+ args: { address: string; constructorArgsParams: unknown[] }
+ }
assert.equal(firstCall.taskName, "verify")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const firstCall = verifyCalls[0] as { | |
| taskName: string | |
| args: { address: string; constructorArgsParams: unknown[] } | |
| } | |
| assert.equal(verifyCalls.length, 1) | |
| assert.equal(firstCall.taskName, "verify") | |
| assert.equal(verifyCalls.length, 1) | |
| const firstCall = verifyCalls[0] as { | |
| taskName: string | |
| args: { address: string; constructorArgsParams: unknown[] } | |
| } | |
| assert.equal(firstCall.taskName, "verify") |
🤖 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 `@cross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.ts`
around lines 252 - 258, The test dereferences verifyCalls[0] before checking its
length; move the assertion assert.equal(verifyCalls.length, 1) to before the
assignment to firstCall (or assert verifyCalls.length > 0) so you never index an
empty array, then assign firstCall from verifyCalls[0] and continue to assert
firstCall.taskName === "verify"; update references to verifyCalls and firstCall
accordingly.
Comment on lines
+15
to
+19
| const originalFormat = providers.Formatter.prototype.transactionResponse | ||
| providers.Formatter.prototype.transactionResponse = function (tx: any): any { | ||
| const patched = tx.to === "" ? { ...tx, to: null } : tx | ||
| return originalFormat.call(this, patched) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ethers.js v5.5.3, is monkey-patching providers.Formatter.prototype.transactionResponse considered a stable approach, and is there a safer supported pattern for normalizing empty-string tx.to values from RPC responses?
💡 Result:
In ethers.js v5.5.3, monkey-patching providers.Formatter.prototype.transactionResponse is not considered a stable or recommended approach. While the Formatter class is exported and technically accessible, it is internal machinery designed to validate and normalize JSON-RPC responses, and modifying its prototype can lead to unpredictable behavior, maintenance difficulties, and breakage if internal implementation details change [1][2]. There is no native, "supported" hook specifically for normalizing empty-string tx.to values via the Formatter in ethers v5. Instead, safer and more standard patterns include: 1. Post-processing the data: Rather than attempting to patch the provider's internal formatting logic, allow the provider to return the transaction object as-is and perform the normalization (e.g., converting an empty string "" to null or a consistent sentinel value) in your application layer after receiving the response from the provider methods like getTransaction [3]. 2. Wrapping the provider: You can create a wrapper or a subclass of JsonRpcProvider that overrides the relevant methods (such as send or getTransaction) [4]. This allows you to intercept the raw response, apply your normalization logic, and return the modified object to the rest of your application, keeping your modifications isolated from ethers' internal prototype structures [4]. 3. Proxying: Utilizing a JavaScript Proxy to wrap the provider instance allows you to intercept property access and method calls. This is a powerful, non-invasive way to modify returned values without subclassing or modifying prototypes, providing a cleaner way to normalize return values like transaction objects [4]. If you find yourself frequently needing to adjust how raw RPC responses are handled, creating a utility function to sanitize your transaction objects is the most maintainable and least error-prone approach [2]. Overriding internal classes like Formatter is strongly discouraged as ethers v6 has moved many of these functions into internal, non-exported modules to discourage such dependencies [2].
Citations:
- 1: Export Formatter class from @ethersproject/providers ethers-io/ethers.js#562
- 2: Upgrading to ethers v6: JSON-RPC formatter ethers-io/ethers.js#4324
- 3: Type for getTransaction misses
| nullethers-io/ethers.js#988 - 4: How to override a provider's behavior? ethers-io/ethers.js#4545
Scope the ethers Formatter.prototype monkey-patch to avoid cumulative global mutation
- This code mutates
providers.Formatter.prototype.transactionResponseglobally; if the task runs multiple times in the same process, wrappers can stack and leak behavior into unrelated calls. - ethers maintainers discourage patching internal
Formatterlogic; prefer sanitizing the returned transaction in your app layer (or wrapping the provider methods that return transactions) instead of modifyingFormatter.prototype.
♻️ Proposed adjustment
- const originalFormat = providers.Formatter.prototype.transactionResponse
- providers.Formatter.prototype.transactionResponse = function (tx: any): any {
- const patched = tx.to === "" ? { ...tx, to: null } : tx
- return originalFormat.call(this, patched)
- }
+ const formatterProto = providers.Formatter.prototype as providers.Formatter & {
+ __emptyToNullPatched?: boolean
+ }
+ if (!formatterProto.__emptyToNullPatched) {
+ const originalFormat = formatterProto.transactionResponse
+ formatterProto.transactionResponse = function (tx: any): any {
+ const patched = tx?.to === "" ? { ...tx, to: null } : tx
+ return originalFormat.call(this, patched)
+ }
+ formatterProto.__emptyToNullPatched = true
+ }🤖 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 `@cross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.ts`
around lines 15 - 19, The current monkey-patch of
providers.Formatter.prototype.transactionResponse (using originalFormat and
overriding transactionResponse) mutates a global prototype and can stack if the
task runs multiple times; instead, limit the change to your provider instance by
wrapping or decorating the provider method that returns transactions (e.g., the
provider’s getTransaction/getTransactionReceipt or sendTransaction wrapper) and
sanitize the returned tx (inspect tx.to and replace "" with null) before
returning, or apply the originalFormat override only temporarily and restore it
after use; update code to stop modifying providers.Formatter.prototype globally
and target the provider instance or call-site that produces the tx.
Comment on lines
+64
to
+67
| const CURRENT_IMPLEMENTATION = "0xD3585922B7F6B30953Fc81726F48046826B8B2CA" | ||
| // The ProxyAdmin owner is a single-key EOA on this rail (no timelock). | ||
| const PROXY_ADMIN_OWNER = "0x353C5c3DE81EDb53FFB398f6416f962b90ae8611" | ||
|
|
There was a problem hiding this comment.
Avoid hardcoding live admin/implementation addresses in fork regression assertions.
Line 64 and Line 66 lock this test to one historical mainnet state. Once ownership rotates or the prior upgrade lands, this fails for the wrong reason. Read the pre-upgrade implementation from EIP1967_IMPLEMENTATION_SLOT before prepareUpgrade, and derive the owner from proxyAdminInstance.owner() before impersonation.
Also applies to: 144-149, 191-193
🤖 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
`@cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts`
around lines 64 - 67, The test currently hardcodes CURRENT_IMPLEMENTATION and
PROXY_ADMIN_OWNER; instead read the live pre-upgrade implementation from
EIP1967_IMPLEMENTATION_SLOT (e.g., readSlot on the proxy address) before calling
prepareUpgrade and use proxyAdminInstance.owner() to fetch the current owner to
impersonate; replace usages of CURRENT_IMPLEMENTATION and PROXY_ADMIN_OWNER in
the assertions and impersonation code (and the other occurrences around the
second and third blocks noted) so the test derives the implementation and admin
dynamically at runtime rather than asserting against fixed addresses.
… rails
Each upgrade script now guards on two axes via the same expression:
func.skip = async (hre) =>
hre.network.name !== "mainnet" || ALREADY_EXECUTED
The mainnet-name guard prevents the script from auto-executing against
hardhat/sepolia during a full `yarn deploy` (load-bearing on StarkNet,
whose hardhat.config wires `deploy_l1` for every network). The
`ALREADY_EXECUTED` constant defaults to `false` and must be flipped to
`true` after governance broadcasts the upgrade, preventing a second
mainnet run from deploying a fresh implementation and overwriting the
deployment artifact with an address that no longer points to on-chain
code.
Before: Native used the network guard only (re-run on mainnet would
re-deploy), Arbitrum/Base/Sui used the sentinel only (re-run on sepolia
would attempt an upgrade), StarkNet had neither.
Each `.verify.test.ts` gains a skip-guard describe block asserting:
both non-mainnet branches skip, and the mainnet branch runs while
ALREADY_EXECUTED is false. The mainnet assertion is intentionally
brittle: when the operator flips the sentinel after execution, the test
must be updated in the same commit, forcing both surfaces to move
together.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts (1)
265-273:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert call count before dereferencing
verifyCalls[0]Same issue as flagged in the Base test —
verifyCalls[0]is accessed on line 265 before the length assertion on line 270.Proposed fix
- const firstCall = verifyCalls[0] as { - taskName: string - args: { address: string; constructorArgsParams: unknown[] } - } - assert.equal(verifyCalls.length, 1) + const firstCall = verifyCalls[0] as { + taskName: string + args: { address: string; constructorArgsParams: unknown[] } + } assert.equal(firstCall.taskName, "verify")🤖 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 `@cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts` around lines 265 - 273, Accessing verifyCalls[0] before checking verifyCalls.length can cause out-of-bounds errors; move the length assertion for verifyCalls (assert.equal(verifyCalls.length, 1)) to come before dereferencing verifyCalls[0], then assign firstCall from verifyCalls[0] and perform the remaining assertions on firstCall.taskName and firstCall.args.constructorArgsParams to keep the same checks but ensure safety (symbols: verifyCalls, firstCall, assert.equal).cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts (1)
274-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert call count before dereferencing
verifyCalls[0]Same pattern as the Base and StarkNet tests —
verifyCalls[0]is accessed on line 274 before the length assertion on line 279. If no verify call occurs, this produces a confusing type error rather than a clear assertion failure.Proposed fix
- const firstCall = verifyCalls[0] as { - taskName: string - args: { address: string; constructorArgsParams: unknown[] } - } - assert.equal(verifyCalls.length, 1) + const firstCall = verifyCalls[0] as { + taskName: string + args: { address: string; constructorArgsParams: unknown[] } + } assert.equal(firstCall.taskName, "verify")🤖 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 `@cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts` around lines 274 - 282, Move the assertion that verifyCalls has one entry before accessing verifyCalls[0]; specifically, assert.equal(verifyCalls.length, 1) should precede the creation of firstCall and any dereference of verifyCalls[0] (the variable firstCall and subsequent checks of firstCall.taskName, firstCall.args.address, and firstCall.args.constructorArgsParams). This ensures the test fails with a clear assertion if no verify calls occurred rather than throwing a runtime/type error when evaluating verifyCalls[0].
🤖 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.
Outside diff comments:
In
`@cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts`:
- Around line 265-273: Accessing verifyCalls[0] before checking
verifyCalls.length can cause out-of-bounds errors; move the length assertion for
verifyCalls (assert.equal(verifyCalls.length, 1)) to come before dereferencing
verifyCalls[0], then assign firstCall from verifyCalls[0] and perform the
remaining assertions on firstCall.taskName and
firstCall.args.constructorArgsParams to keep the same checks but ensure safety
(symbols: verifyCalls, firstCall, assert.equal).
In `@cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts`:
- Around line 274-282: Move the assertion that verifyCalls has one entry before
accessing verifyCalls[0]; specifically, assert.equal(verifyCalls.length, 1)
should precede the creation of firstCall and any dereference of verifyCalls[0]
(the variable firstCall and subsequent checks of firstCall.taskName,
firstCall.args.address, and firstCall.args.constructorArgsParams). This ensures
the test fails with a clear assertion if no verify calls occurred rather than
throwing a runtime/type error when evaluating verifyCalls[0].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdf1e961-298c-44bd-8192-d636a3d13a04
📒 Files selected for processing (10)
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.tscross-chain/arbitrum/test/UpgradeArbitrumL1BitcoinDepositorTo968.verify.test.tscross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.tscross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.tscross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.tscross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.tscross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.tscross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.tssolidity/deploy/44_upgrade_native_btc_depositor_to_968.tssolidity/test/depositor/UpgradeNativeBTCDepositorTo968.verify.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts
- cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts
- cross-chain/arbitrum/test/UpgradeArbitrumL1BitcoinDepositorTo968.verify.test.ts
Each upgrade script now writes the proxy-admin upgrade calldata to a
tracked JSON file alongside the existing deployments.log line:
<package_root>/governance-calldata/968-<DEPLOYMENT_NAME>.json
The log line is ephemeral (stdout-scraped) and copy-paste-prone; the
JSON file is the durable hand-off artifact the proposer (Council Safe /
24h timelock / EOA) can verify against, and the file diff doubles as a
code-review surface for the upgrade proposal PR. Written outside
deployments/ because hardhat-deploy treats that directory as its own
deployment-artifact namespace (scans it for *.json on every run, plus
several rails .gitignore subpaths under it).
Payload shape (uniform across all five rails):
{ network, contract, proxy, newImpl, proxyAdmin, proxyAdminOwner,
upgradeTx: { from, to, data } }
The existing calldata-shape verify tests now inject
hre.config.paths.root pointing at an os.tmpdir() subdir, assert the
file is written, and deep-equal the JSON against the expected shape.
Any silent change to the payload contract (added/removed field,
renamed nesting) fails the assertion loudly.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts (1)
25-29:⚠️ Potential issue | 🟠 MajorRestore the global ethers formatter patch in a
finallyblock.This script patches
providers.Formatter.prototype.transactionResponseat the top but never restores it, so ifprepareUpgrade/deployments.save/verifythrows, the mutated formatter can leak into the rest of the Hardhat process.Suggested fix
const originalFormat = providers.Formatter.prototype.transactionResponse providers.Formatter.prototype.transactionResponse = function (tx: any): any { const patched = tx.to === "" ? { ...tx, to: null } : tx return originalFormat.call(this, patched) } + try { const { deployer } = await helpers.signers.getNamedSigners() @@ await run("verify", { address: newImplementationAddress, constructorArgsParams: [], }) + } finally { + providers.Formatter.prototype.transactionResponse = originalFormat + } }🤖 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 `@cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts` around lines 25 - 29, The global patch to providers.Formatter.prototype.transactionResponse is left in place and can leak if an exception occurs; wrap the mutation and the calls that rely on it (e.g., prepareUpgrade, deployments.save, verify) in a try/finally: save the original formatter (originalFormat), set the patched function before calling prepareUpgrade/deployments.save/verify, and in a finally block restore providers.Formatter.prototype.transactionResponse = originalFormat so the global state is always reverted even on errors.cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts (1)
75-120:⚠️ Potential issue | 🟠 MajorEnsure
simulateUpgradealways cleans up patched ethers formatter and Hardhat impersonation
patchEthersFormatter()overwritesethers.providers.Formatter.prototype.transactionResponseand is never restored ifdeploymentsGet,upgrades.prepareUpgrade, orproxyAdmin.upgradethrows.hardhat_stopImpersonatingAccountonly runs on the success path; if execution fails afterhardhat_impersonateAccount, the impersonation persists for subsequent tests.Suggested fix
async function simulateUpgrade(): Promise<string> { - patchEthersFormatter() - - const proxyDeployment = await deploymentsGet() - - const factory = await ethers.getContractFactory(CONTRACT_NAME) - const newImplementation = (await upgrades.prepareUpgrade( - proxyDeployment, - factory, - { - kind: "transparent", - unsafeAllowCustomTypes: true, - } - )) as string - - const proxyAdmin = await upgrades.admin.getInstance() - const proxyAdminOwner: string = await proxyAdmin.owner() - - await network.provider.request({ - method: "hardhat_impersonateAccount", - params: [proxyAdminOwner], - }) - await network.provider.request({ - method: "hardhat_setBalance", - params: [proxyAdminOwner, "0x21E19E0C9BAB2400000"], - }) - - const ownerSigner = await ethers.getSigner(proxyAdminOwner) - await proxyAdmin - .connect(ownerSigner) - .upgrade(PROXY_ADDRESS, newImplementation) - - await network.provider.request({ - method: "hardhat_stopImpersonatingAccount", - params: [proxyAdminOwner], - }) - - return newImplementation + const { providers } = ethers + const originalFormat = providers.Formatter.prototype.transactionResponse + patchEthersFormatter() + + let proxyAdminOwner: string | undefined + try { + const proxyDeployment = await deploymentsGet() + const factory = await ethers.getContractFactory(CONTRACT_NAME) + const newImplementation = (await upgrades.prepareUpgrade( + proxyDeployment, + factory, + { + kind: "transparent", + unsafeAllowCustomTypes: true, + } + )) as string + + const proxyAdmin = await upgrades.admin.getInstance() + proxyAdminOwner = await proxyAdmin.owner() + + await network.provider.request({ + method: "hardhat_impersonateAccount", + params: [proxyAdminOwner], + }) + await network.provider.request({ + method: "hardhat_setBalance", + params: [proxyAdminOwner, "0x21E19E0C9BAB2400000"], + }) + + const ownerSigner = await ethers.getSigner(proxyAdminOwner) + await proxyAdmin.connect(ownerSigner).upgrade(PROXY_ADDRESS, newImplementation) + + return newImplementation + } finally { + providers.Formatter.prototype.transactionResponse = originalFormat + if (proxyAdminOwner) { + await network.provider.request({ + method: "hardhat_stopImpersonatingAccount", + params: [proxyAdminOwner], + }) + } + } }🤖 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 `@cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts` around lines 75 - 120, simulateUpgrade currently calls patchEthersFormatter() and impersonates the ProxyAdmin owner but does not restore the original ethers formatter or guarantee hardhat_stopImpersonatingAccount runs on error; wrap the main body of simulateUpgrade in try/finally (or equivalent) so you always restore ethers.providers.Formatter.prototype.transactionResponse to its original reference saved before calling patchEthersFormatter(), and always call hardhat_stopImpersonatingAccount for proxyAdminOwner if impersonation succeeded (ensure you track a boolean or check state) even when deploymentsGet, upgrades.prepareUpgrade, or proxyAdmin.upgrade throws; reference simulateUpgrade, patchEthersFormatter, proxyAdmin.owner()/upgrade, and the hardhat_impersonateAccount/hardhat_stopImpersonatingAccount calls when making the changes.
♻️ Duplicate comments (1)
cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts (1)
25-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe formatter patch still needs to be restored.
This is the same unresolved issue from the earlier review: the script overwrites
providers.Formatter.prototype.transactionResponseand never restores it, so later deploy steps can inherit the patched behavior. Please wrap the rest offuncintry/finallyand reassignoriginalFormatin thefinally.🤖 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 `@cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts` around lines 25 - 29, The patch to providers.Formatter.prototype.transactionResponse currently replaces the formatter and never restores it; update the surrounding code (the function containing this patch, e.g., func) to wrap the remaining logic in a try/finally block so that originalFormat is re-assigned back to providers.Formatter.prototype.transactionResponse in the finally clause; keep using the patched behavior inside the try (creating patched from tx.to === "" -> { ...tx, to: null } as you have) and ensure originalFormat (the saved reference) is used to call the original formatter while the patch is active, then always restore providers.Formatter.prototype.transactionResponse = originalFormat in finally.
🤖 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.
Outside diff comments:
In
`@cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.ts`:
- Around line 25-29: The global patch to
providers.Formatter.prototype.transactionResponse is left in place and can leak
if an exception occurs; wrap the mutation and the calls that rely on it (e.g.,
prepareUpgrade, deployments.save, verify) in a try/finally: save the original
formatter (originalFormat), set the patched function before calling
prepareUpgrade/deployments.save/verify, and in a finally block restore
providers.Formatter.prototype.transactionResponse = originalFormat so the global
state is always reverted even on errors.
In `@cross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.ts`:
- Around line 75-120: simulateUpgrade currently calls patchEthersFormatter() and
impersonates the ProxyAdmin owner but does not restore the original ethers
formatter or guarantee hardhat_stopImpersonatingAccount runs on error; wrap the
main body of simulateUpgrade in try/finally (or equivalent) so you always
restore ethers.providers.Formatter.prototype.transactionResponse to its original
reference saved before calling patchEthersFormatter(), and always call
hardhat_stopImpersonatingAccount for proxyAdminOwner if impersonation succeeded
(ensure you track a boolean or check state) even when deploymentsGet,
upgrades.prepareUpgrade, or proxyAdmin.upgrade throws; reference
simulateUpgrade, patchEthersFormatter, proxyAdmin.owner()/upgrade, and the
hardhat_impersonateAccount/hardhat_stopImpersonatingAccount calls when making
the changes.
---
Duplicate comments:
In `@cross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.ts`:
- Around line 25-29: The patch to
providers.Formatter.prototype.transactionResponse currently replaces the
formatter and never restores it; update the surrounding code (the function
containing this patch, e.g., func) to wrap the remaining logic in a try/finally
block so that originalFormat is re-assigned back to
providers.Formatter.prototype.transactionResponse in the finally clause; keep
using the patched behavior inside the try (creating patched from tx.to === "" ->
{ ...tx, to: null } as you have) and ensure originalFormat (the saved reference)
is used to call the original formatter while the patch is active, then always
restore providers.Formatter.prototype.transactionResponse = originalFormat in
finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9031d5a7-0c2f-49cd-b69a-e89aed0fa7a0
📒 Files selected for processing (10)
cross-chain/arbitrum/deploy_l1/02_upgrade_arbitrum_l1_bitcoin_depositor_to_968.tscross-chain/arbitrum/test/UpgradeArbitrumL1BitcoinDepositorTo968.verify.test.tscross-chain/base/deploy_l1/04_upgrade_base_l1_bitcoin_depositor_to_968.tscross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.tscross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.tscross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.tscross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.tscross-chain/sui/test/UpgradeSuiBTCDepositorWormholeTo968.verify.test.tssolidity/deploy/44_upgrade_native_btc_depositor_to_968.tssolidity/test/depositor/UpgradeNativeBTCDepositorTo968.verify.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- cross-chain/sui/deploy_l1/02_upgrade_sui_btc_depositor_to_968.ts
- cross-chain/starknet/deploy_l1/03_upgrade_starknet_btc_depositor_to_968.ts
- solidity/deploy/44_upgrade_native_btc_depositor_to_968.ts
- cross-chain/arbitrum/test/UpgradeArbitrumL1BitcoinDepositorTo968.verify.test.ts
- cross-chain/base/test/UpgradeBaseL1BitcoinDepositorTo968.verify.test.ts
- solidity/test/depositor/UpgradeNativeBTCDepositorTo968.verify.test.ts
- cross-chain/starknet/test/UpgradeStarkNetBitcoinDepositorTo968.verify.test.ts
Collapses two-arg `path.join` calls and an `0x..."` constant onto single lines per prettier's 80-column wrap (CI's contracts-format job flagged both as `prettier/prettier` errors on the previous push). Behavior is unchanged.
piotr-roslaniec
previously approved these changes
Jun 8, 2026
…t no-op The Native upgrade ran forceImport(proxy, factory) then prepareUpgrade(proxy, factory). Because #968 is a logic-only change, forceImport registered the CURRENT on-chain implementation under the new factory's bytecode-version slot, so prepareUpgrade short-circuited to that cached entry and returned the current implementation instead of deploying #968 — emitting a no-op upgrade(proxy, currentImpl) that ships no new bytecode while governance believes #968 landed. Re-key the entries forceImport injects off the version slot (the upgrade-safety baseline is still resolved by address scan) so prepareUpgrade deploys the new implementation; this mirrors the workaround already proven in test/depositor/UpgradeNativeBTCDepositorTo968.test.ts. Add a self-protecting guard that throws if the emitted implementation equals the proxy's current on-chain implementation, so the script can never silently emit a no-op. Verified by invoking the deploy script on a mainnet fork: it now deploys a fresh implementation and emits upgrade(proxy, newImpl).
ESLint's quotes rule requires double quotes for template-literal segments without ${} interpolation. The two non-interpolated segments of the no-op guard's error message failed contracts-format; switch them to double-quoted strings (the interpolated middle segment stays a template literal).
… test The verification unit test invokes func with a stubbed forceImport (no manifest written) under a temp project root, so the unconditional manifest read after forceImport threw ENOENT and the provider-based guard had no live chain. Guard the manifest re-keying with fs.existsSync (the mainnet run always writes the manifest; the mock does not) and replace the EIP-1967 chain read with a plain lowercased comparison against the known current implementation. Re-verified on a mainnet fork: func still deploys a fresh implementation, and the mocked verification suite passes.
piotr-roslaniec
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#968 ("Make tx max fee reimbursement best effort") is merged on
main(d24b9c8f) but inert until each live L1 depositor proxy is pointed at a freshly compiled implementation. This PR adds the deployment tooling for all five rails — Native, Arbitrum, Base, StarkNet, and Sui — one hardhat-deploy upgrade-in-place script per rail. Each script deploys a new #968 implementation for its Transparent proxy, emits (but does not broadcast) the governanceProxyAdmin.upgrade(proxy, newImpl)calldata for that rail's owner to execute, updates the deployment artifact, and Etherscan-verifies the new implementation. Each rail also ships a mainnet-fork regression test asserting that the live application storage survives the simulated upgrade and that the new implementation carries theDepositTxMaxFeeReimbursementSkippedbest-effort path.Per-rail notes
The five proxies are owned by distinct authorities (a 6-of-9 council Safe for Native, a shared 24h timelock for Arbitrum and Base, and single-key EOAs for StarkNet and Sui), so the scripts emit calldata only and never broadcast the upgrade. Each rail compiles its own variant from #968 source: Arbitrum and Base via
copyWormholeV2Artifactstaging of the per-variant V2 contracts (the deliberate +1 storage-slot offset between them is preserved), Native via the patchedAbstractL1BTCDepositorbase, and StarkNet via its already-#968 vendored tree.Sui is the notable case: it previously resolved
BTCDepositorWormholefrom the published@keep-network/tbtc-v2@1.8.0dependency, which predates #968 — so as-templated it would have shipped pre-#968 bytecode. This PR stages the localsolidity/#968BTCDepositorWormholeartifact into the Sui package (mirroring Arbitrum/Base) and repoints the deploycontractNameonto it. A dependency bump is not viable: the latest published release (1.8.2) also predates the #968 merge.Testing
The fork regression tests are gated on
FORKING_URLand run against a local anvil mainnet fork under--network system_tests, since Hardhat's built-in forking cannot initialize against an RPC that omitstotalDifficulty. All five rails pass locally: Arbitrum, Base, StarkNet, and Sui assert storage-unchanged plus #968-present across the simulated upgrade; Native is covered by a mocked forceImport-ordering unit test. These fork suites are local-only (there is no cross-chain CI workflow for Base/StarkNet/Sui) and must be run one rail per fresh anvil instance — OpenZeppelin keeps a shared per-dev-instance manifest in$TMPDIR/openzeppelin-upgrades, so running several rails against a single anvil cross-contaminates it. CI runs the non-fork suites (calldata-shape and staged-ABI #968-presence), which pass for every rail.A shared-helper fix was needed for the Sui rail: packages that consume a staged artifact but compile no local upgradeable source had no top-level
versionin their OpenZeppelin validations cache, whichprepareUpgraderejected as outdated. The staging helper now backfills that version when absent; Arbitrum/Base, which already carry one, are untouched.Out of scope
d24b9c8f.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores