feat: SafeERC20 + CREATE2 implementation deploy#6
Conversation
- use OpenZeppelin SafeERC20 for ERC20 donations so USDT-style tokens (no return value) work; extend tests with a no-return mock - add DeployDonationHandlerImplementation via CreateX CREATE2 with a deterministic compiler profile (metadata off) for cross-chain parity - wire yarn deploy:implementation, multi-chain RPC/etherscan in foundry.toml, and fix deploy:* npm scripts to target DeployDonationHandler.s.sol Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughAdds deterministic, multi-chain deployment tooling (CreateX CREATE2 script, deterministic Foundry profile, deploy wrapper, and npm scripts), updates RPC/keys in env and foundry configs, switches DonationHandler to OpenZeppelin SafeERC20, introduces ERC20 test mocks (failing/no-return), and updates tests and imports accordingly. ChangesDeterministic Multi-Chain Deployment + Safety & Tests
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Shell as deploy-implementation.sh
participant Script as DeployDonationHandlerImplementation
participant CreateX as CreateX Factory
participant Chain as RPC Node
participant ProxyAdmin as ProxyAdmin (off-chain/owner)
Dev->>Shell: run ./scripts/deploy-implementation.sh <chain>
Shell->>Shell: load .env, resolve <CHAIN>_RPC
Shell->>Chain: cast code CREATEX_ADDRESS (check exists)
Shell->>Script: FOUNDRY_PROFILE=deterministic forge script ... --rpc-url
Script->>Chain: read nonce/chain info, compute initCodeHash
Script->>CreateX: computeCreate2Address(salt, initCodeHash)
CreateX-->>Script: predicted address
Script->>Chain: broadcast deployCreate2(salt, initCode)
Chain->>CreateX: execute CREATE2, deploy implementation
CreateX-->>Script: deployed address
Script->>Script: require(deployed == predicted)
Script->>Dev: log deployed address and upgrade note (use ProxyAdmin.upgradeAndCall)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/DonationHandlerSetup.t.sol (1)
27-60: ⚡ Quick win
NoReturnMockERC20is duplicated verbatim intest/DonationHandler.t.sol.This contract is defined identically in both
test/DonationHandlerSetup.t.solandtest/DonationHandler.t.sol. SinceDonationHandlerSetup.t.solis already the shared test utilities home, the copy inDonationHandler.t.solshould be removed.DonationHandler.t.solcan import the contract from this file. Similarly,MockERC20andFailingMockERC20are also duplicated in both files — consider extracting all test mocks to a dedicatedtest/mocks/file for a clean separation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/DonationHandlerSetup.t.sol` around lines 27 - 60, Remove the duplicate mock contract definitions (NoReturnMockERC20, MockERC20, FailingMockERC20) from test/DonationHandler.t.sol and instead import them from the shared file (e.g., DonationHandlerSetup.t.sol) or move all mocks into a new test/mocks/ file; update DonationHandler.t.sol to import the centralized mock definitions and delete the local verbatim contract declarations so NoReturnMockERC20 and the other mocks are defined only once and reused across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/DonationHandlerSetup.t.sol`:
- Around line 27-60: Remove the duplicate mock contract definitions
(NoReturnMockERC20, MockERC20, FailingMockERC20) from test/DonationHandler.t.sol
and instead import them from the shared file (e.g., DonationHandlerSetup.t.sol)
or move all mocks into a new test/mocks/ file; update DonationHandler.t.sol to
import the centralized mock definitions and delete the local verbatim contract
declarations so NoReturnMockERC20 and the other mocks are defined only once and
reused across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b29ad126-17b2-4ba9-83d3-c21499332da7
📒 Files selected for processing (9)
.env.examplefoundry.tomlpackage.jsonscript/DeployDonationHandlerImplementation.s.solscripts/deploy-implementation.shsrc/contracts/DonationHandler.soltest/DonationHandler.t.soltest/DonationHandlerSetup.t.soltest/DonationHandlerStandardTests.t.sol
…ests - require PUBLIC_KEY matches PRIVATE_KEY in DeployDonationHandler - drop unused ProxyAdmin import; guard CreateX code on chain in implementation deploy - preflight cast code for CreateX in deploy-implementation.sh; document deterministic profile - assert SafeERC20FailedOperation on failing transfer; dedupe NoReturnMockERC20 helper Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/DeployDonationHandler.s.sol (1)
10-13:⚠️ Potential issue | 🟠 MajorChain-specific ProxyAdmin constants are defined but ignored—all deployments use the deployer EOA as admin.
Lines 10–13 declare
ethereumProxyAdmin,gnosisProxyAdmin,optimismProxyAdmin, andpolygonProxyAdmin, but line 25 passesaddress(deployer)toTransparentUpgradeableProxy, ignoring all chain-specific constants.Impact depends on OZ version:
- OZ v5: The proxy creates its own
ProxyAdmininstance, owned by the deployer EOA (not by the intended governance contract).- OZ v4: The deployer EOA becomes the direct proxy admin (not the intended governance contract).
Either way, upgrade authority is held by an EOA rather than the intended per-chain governance contract. With the expanded multi-chain deployment scope (new RPC configs in
foundry.toml), this mismatch is more likely to materialise in production.To fix: Use chain-ID–based logic to select the correct admin per chain at deploy time:
Suggested chain-based ProxyAdmin selection
+ address proxyAdmin; + uint256 chainId = block.chainid; + if (chainId == 1) proxyAdmin = ethereumProxyAdmin; + else if (chainId == 100) proxyAdmin = gnosisProxyAdmin; + else if (chainId == 10) proxyAdmin = optimismProxyAdmin; + else if (chainId == 137) proxyAdmin = polygonProxyAdmin; + else revert('Unsupported chain'); + TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy( - address(donationHandler), address(deployer), abi.encodeWithSelector(DonationHandler.initialize.selector) + address(donationHandler), proxyAdmin, abi.encodeWithSelector(DonationHandler.initialize.selector) );Also applies to: line 25 (same issue in the same deployment call).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/DeployDonationHandler.s.sol` around lines 10 - 13, The ProxyAdmin constants (ethereumProxyAdmin, gnosisProxyAdmin, optimismProxyAdmin, polygonProxyAdmin) are declared but never used—TransparentUpgradeableProxy is being created with address(deployer) which gives upgrade authority to the deployer EOA; change the deployment to select the correct admin per chain by checking block.chainid (or use a helper like getChainAdmin) and pass the matching constant into the TransparentUpgradeableProxy constructor instead of address(deployer); update the creation site where TransparentUpgradeableProxy is called (and any helper function that builds proxy args) so that on each chain the proxy admin is the intended per-chain governance address rather than the deployer EOA.
🧹 Nitpick comments (5)
scripts/deploy-implementation.sh (5)
6-6: 💤 Low value
ETHERSCAN_API_KEYcomment is misleading for multi-chain deployments.The comment documents only
ETHERSCAN_API_KEY, but--verifyon non-Ethereum chains (Base, Arbitrum, Polygon, etc.) requires chain-specific keys (e.g.,BASESCAN_API_KEY,ARBISCAN_API_KEY). Foundry resolves these through the[etherscan]section offoundry.toml. If users follow the comment and only setETHERSCAN_API_KEY, verification will silently fail or error on those chains.Consider updating the comment to point users to the
[etherscan]keys infoundry.tomland.env.example:-# Requires .env: PRIVATE_KEY, <CHAIN>_RPC (e.g. BASE_RPC), ETHERSCAN_API_KEY for --verify +# Requires .env: PRIVATE_KEY, <CHAIN>_RPC (e.g. BASE_RPC), and the chain-specific +# explorer API key defined in foundry.toml [etherscan] (e.g. BASESCAN_API_KEY, ARBISCAN_API_KEY).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-implementation.sh` at line 6, Update the top-of-file comment in deploy-implementation.sh to clarify that verification (--verify) uses chain-specific API keys configured under the [etherscan] section of foundry.toml, not just ETHERSCAN_API_KEY; mention examples like BASESCAN_API_KEY and ARBISCAN_API_KEY, advise users to set the appropriate per-chain key in their .env (and mirror these keys in .env.example), and call out that the script expects PRIVATE_KEY and <CHAIN>_RPC as before.
28-33: ⚡ Quick win
cast codestderr suppression can mask bad RPC URLs.
2>/dev/null || truesilently discards allcasterrors (network timeouts, invalid RPC URL, authentication failures). Whencastfails without producing stdout,CREATEX_CODEis empty and the script exits with the CreateX error message — which is misleading when the real cause is a bad RPC endpoint.Consider capturing the exit code separately to distinguish an unreachable RPC from a genuinely undeployed CreateX:
🛡️ Proposed fix
-CREATEX_CODE=$(cast code "$CREATEX_ADDRESS" --rpc-url "${!RPC_VAR}" 2>/dev/null || true) -if [[ -z "$CREATEX_CODE" || "$CREATEX_CODE" == "0x" ]]; then - echo "Error: CreateX not deployed at $CREATEX_ADDRESS on this RPC (cast code returned empty)." +if ! CREATEX_CODE=$(cast code "$CREATEX_ADDRESS" --rpc-url "${!RPC_VAR}" 2>&1); then + echo "Error: Could not reach RPC or cast failed: $CREATEX_CODE" + exit 1 +fi +if [[ -z "$CREATEX_CODE" || "$CREATEX_CODE" == "0x" ]]; then + echo "Error: CreateX not deployed at $CREATEX_ADDRESS on chain '$CHAIN'." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-implementation.sh` around lines 28 - 33, The script currently silences all cast errors with `2>/dev/null || true` which masks RPC failures; change the logic around CREATEX_CODE so you run `cast code "$CREATEX_ADDRESS" --rpc-url "${!RPC_VAR}"` without discarding errors, capture its exit status, and if it fails emit a distinct error mentioning the RPC problem (including the RPC URL variable and exit code/stdout/stderr) versus the case where cast succeeded but returned empty/0x indicating CreateX truly isn't deployed; update the checks that reference CREATEX_CODE and the exit path to distinguish these two failures and log the relevant diagnostic info (use the CREATEX_ADDRESS, CREATEX_CODE and the captured cast exit code/stderr).
18-18: ⚡ Quick winAdd a
.envexistence guard before sourcing.With
set -eactive,source .envon a missing file aborts with a crypticbash: .env: No such file or directory, which is especially confusing in CI. A short pre-check gives a clear error message.🛡️ Proposed fix
+if [[ ! -f .env ]]; then + echo "Error: .env not found in repo root. Copy .env.example and fill in the values." + exit 1 +fi source .env🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-implementation.sh` at line 18, Add a pre-check before the existing source .env invocation to ensure the .env file exists and exit with a clear error if it doesn't; specifically, before the source .env line (in the same script that sets set -e), test for the file (e.g., [[ -f .env ]] || { echo "Missing .env — please provide one" >&2; exit 1; }) so the script fails with a readable message instead of the cryptic bash error when source .env would run.
2-2: ⚡ Quick winAdd
set -o pipefailfor safer pipeline error propagation.
set -ealone won't catch failures in pipeline intermediaries (e.g., iftrorechoin theRPC_SUFFIXpipeline exits non-zero). Addingpipefailis a standard hardening practice for deployment scripts.🛡️ Proposed fix
-set -e +set -euo pipefailNote: Adding
-u(nounset) alongsidepipefailwould also catch unbound variable accesses, though the existing${1:?...}guard and the explicit RPC URL check already cover the most critical variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-implementation.sh` at line 2, Replace the lone "set -e" with a safer shell option that enables pipefail so pipeline failures surface (e.g., add "set -o pipefail" or use "set -euo pipefail"); update the top of the script where "set -e" appears (in scripts/deploy-implementation.sh) so that failures inside pipelines such as the RPC_SUFFIX pipeline are not masked, and optionally include "-u" if you want nounset checks as noted.
43-49: 💤 Low valueUnquoted
$LEGACY_FLAGis safe here but worth noting.
$LEGACY_FLAGis intentionally unquoted so that an empty string doesn't pass a blank argument toforge script. This is correct for Bash word-splitting semantics but will triggerSC2086in stricter shellcheck runs. If you ever addset -u, an unsetLEGACY_FLAGwould also cause an error (though it is always set above). Using an array is the idiomatic Bash approach:♻️ Idiomatic alternative using an array
LEGACY_CHAINS="celo gnosis" -LEGACY_FLAG="" +LEGACY_FLAGS=() if [[ " $LEGACY_CHAINS " == *" $CHAIN "* ]]; then - LEGACY_FLAG="--legacy" + LEGACY_FLAGS=("--legacy") fi export FOUNDRY_PROFILE=deterministic forge script script/DeployDonationHandlerImplementation.s.sol:DeployDonationHandlerImplementation \ --rpc-url "${!RPC_VAR}" \ --broadcast \ --verify \ --chain "$CHAIN" \ - $LEGACY_FLAG \ + "${LEGACY_FLAGS[@]}" \ -vvvv🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy-implementation.sh` around lines 43 - 49, The unquoted $LEGACY_FLAG in the forge script invocation can trigger SC2086 and will break under set -u; change the invocation to build and execute a bash array instead: prepare an array for the command (e.g., COMMAND or ARGS), include fixed args like "forge", "script", "script/DeployDonationHandlerImplementation.s.sol:DeployDonationHandlerImplementation", "--rpc-url", "${!RPC_VAR}", "--broadcast", "--verify", "--chain", "$CHAIN", "-vvvv", and only append $LEGACY_FLAG to the array if it is non-empty, then run the script with "${COMMAND[@]}" so word-splitting and unset-variable issues are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@script/DeployDonationHandler.s.sol`:
- Around line 10-13: The ProxyAdmin constants (ethereumProxyAdmin,
gnosisProxyAdmin, optimismProxyAdmin, polygonProxyAdmin) are declared but never
used—TransparentUpgradeableProxy is being created with address(deployer) which
gives upgrade authority to the deployer EOA; change the deployment to select the
correct admin per chain by checking block.chainid (or use a helper like
getChainAdmin) and pass the matching constant into the
TransparentUpgradeableProxy constructor instead of address(deployer); update the
creation site where TransparentUpgradeableProxy is called (and any helper
function that builds proxy args) so that on each chain the proxy admin is the
intended per-chain governance address rather than the deployer EOA.
---
Nitpick comments:
In `@scripts/deploy-implementation.sh`:
- Line 6: Update the top-of-file comment in deploy-implementation.sh to clarify
that verification (--verify) uses chain-specific API keys configured under the
[etherscan] section of foundry.toml, not just ETHERSCAN_API_KEY; mention
examples like BASESCAN_API_KEY and ARBISCAN_API_KEY, advise users to set the
appropriate per-chain key in their .env (and mirror these keys in .env.example),
and call out that the script expects PRIVATE_KEY and <CHAIN>_RPC as before.
- Around line 28-33: The script currently silences all cast errors with
`2>/dev/null || true` which masks RPC failures; change the logic around
CREATEX_CODE so you run `cast code "$CREATEX_ADDRESS" --rpc-url "${!RPC_VAR}"`
without discarding errors, capture its exit status, and if it fails emit a
distinct error mentioning the RPC problem (including the RPC URL variable and
exit code/stdout/stderr) versus the case where cast succeeded but returned
empty/0x indicating CreateX truly isn't deployed; update the checks that
reference CREATEX_CODE and the exit path to distinguish these two failures and
log the relevant diagnostic info (use the CREATEX_ADDRESS, CREATEX_CODE and the
captured cast exit code/stderr).
- Line 18: Add a pre-check before the existing source .env invocation to ensure
the .env file exists and exit with a clear error if it doesn't; specifically,
before the source .env line (in the same script that sets set -e), test for the
file (e.g., [[ -f .env ]] || { echo "Missing .env — please provide one" >&2;
exit 1; }) so the script fails with a readable message instead of the cryptic
bash error when source .env would run.
- Line 2: Replace the lone "set -e" with a safer shell option that enables
pipefail so pipeline failures surface (e.g., add "set -o pipefail" or use "set
-euo pipefail"); update the top of the script where "set -e" appears (in
scripts/deploy-implementation.sh) so that failures inside pipelines such as the
RPC_SUFFIX pipeline are not masked, and optionally include "-u" if you want
nounset checks as noted.
- Around line 43-49: The unquoted $LEGACY_FLAG in the forge script invocation
can trigger SC2086 and will break under set -u; change the invocation to build
and execute a bash array instead: prepare an array for the command (e.g.,
COMMAND or ARGS), include fixed args like "forge", "script",
"script/DeployDonationHandlerImplementation.s.sol:DeployDonationHandlerImplementation",
"--rpc-url", "${!RPC_VAR}", "--broadcast", "--verify", "--chain", "$CHAIN",
"-vvvv", and only append $LEGACY_FLAG to the array if it is non-empty, then run
the script with "${COMMAND[@]}" so word-splitting and unset-variable issues are
avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0e35f24-814f-449a-bbdd-4f668367d0e2
📒 Files selected for processing (12)
.gitignorescript/DeployDonationHandler.s.solscript/DeployDonationHandlerImplementation.s.solscripts/deploy-implementation.shtest/DonationHandler.t.soltest/DonationHandlerBugFix.t.soltest/DonationHandlerMultisigCalls.t.soltest/DonationHandlerSetup.t.soltest/DonationHandlerStandardTests.t.soltest/mocks/FailingMockERC20.soltest/mocks/MockERC20.soltest/mocks/NoReturnMockERC20.sol
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- test/mocks/MockERC20.sol
- test/DonationHandlerMultisigCalls.t.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- script/DeployDonationHandlerImplementation.s.sol
- test/DonationHandlerStandardTests.t.sol
1 participant
Summary
SafeERC20/safeTransferFromso non-standard tokens (e.g. USDT-style, no return value) work.NoReturnMockERC20and coverage inDonationHandler.t.sol(and shared setup/standard tests).DeployDonationHandlerImplementation.s.soldeploys via CreateX CREATE2 with a fixed salt;scripts/deploy-implementation.shsetsFOUNDRY_PROFILE=deterministic(metadata off for identical bytecode across chains).yarn deploy:implementation, multi-chain RPC/etherscan infoundry.toml, fixdeploy:mainnet/deploy:sepoliato targetDeployDonationHandler.s.sol..env.exampleupdated.Upgrades are intentionally out of scope (handled separately, e.g. Safe / evmcrispr).
References
Test plan
forge testMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores