Conversation
- Updated `deploy:sepolia` script to deploy the DonationHandler implementation. - Added new upgrade scripts for both mainnet and sepolia, including a test fork option. - Expanded README with detailed instructions on upgrading existing deployments and verifying upgrades. - Improved logging in the deployment script for better visibility of deployment details.
- Added .env.local to .gitignore to prevent local environment files from being tracked. - Included a new script path for retrieving proxy addresses in package.json.
- Changed `upgrade:mainnet:fork` to `upgrade:mainnet:simulate` in package.json for consistency. - Updated documentation across README and PRODUCTION_UPGRADE.md to reflect the new simulate command. - Enhanced comments in deployment scripts for better understanding of deployment and upgrade processes.
- Introduced new scripts for deploying the DonationHandler implementation on both mainnet and sepolia in package.json. - Expanded PRODUCTION_UPGRADE.md with instructions for deploying the implementation without ProxyAdmin access, including verification steps and handoff to the proxy owner.
- Added new RPC endpoints for Base, Arbitrum, Gnosis, Celo, Optimism, and Polygon in .env.example and foundry.toml. - Updated PRODUCTION_UPGRADE.md to reflect the new deployment command format for different chains.
| @@ -0,0 +1,98 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.22; | |||
There was a problem hiding this comment.
test files should go into the test folder, not the script folder, should use .t.sol file pattern not .s.sol
|
|
||
| ## 🎯 Networks to Upgrade | ||
|
|
||
| Based on your deployments: |
There was a problem hiding this comment.
Do you also have any deployments on Arbitrum or Polygon that should be included?
- Added new environment variables for Safe upgrade proposals in .env.example. - Updated .gitignore to include upgrade-payload.json. - Introduced new scripts in package.json for generating upgrade payloads and submitting upgrades to Safe. - Added dependencies for Safe API and protocol kits to support the new upgrade functionality.
- Added new parameters to .env.example for Safe upgrade proposals. - Modified package.json to streamline the upgrade submission script. - Expanded MULTISIG_UPGRADE_GUIDE.md with detailed steps for proposing upgrades. - Enhanced submit-upgrade-to-safe.mjs to load env and improve error messaging. Made-with: Cursor
WalkthroughAdds end-to-end upgrade infrastructure for DonationHandler: new Forge scripts for implementation and proxy upgrades, test/validation scripts, shell and Node.js tooling for Safe multisig flows, expanded multi-network RPC/config, and extensive upgrade documentation (production, testnet, multisig). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator as Operator\n(shell / Node script)
participant SafeAPI as Safe Transaction Service
participant ProxyAdmin as ProxyAdmin (on-chain)
participant Proxy as TransparentUpgradeableProxy
participant Chain as Blockchain (RPC)
Operator->>SafeAPI: propose/signal upgrade (signed tx via Safe SDK)
SafeAPI-->>Operator: proposal accepted (tx id / url)
Note right of SafeAPI: signers approve in UI / via SDK
SafeAPI->>Chain: Safe executes transaction -> calls ProxyAdmin.upgrade(proxy, impl)
ProxyAdmin->>Proxy: update implementation slot (EIP‑1967)
Proxy->>Chain: new implementation becomes active
Operator->>Chain: verify implementation slot & run sanity tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 9
🧹 Nitpick comments (4)
script/DeployDonationHandler.s.sol (1)
14-17: Unused constants - consider removing or documenting purpose.The
ETHEREUM_PROXY_ADMIN,GNOSIS_PROXY_ADMIN,OPTIMISM_PROXY_ADMIN, andPOLYGON_PROXY_ADMINconstants are declared but never used in therun()function. The script creates a new ProxyAdmin automatically via theTransparentUpgradeableProxyconstructor.If these are meant for reference/documentation of existing deployments, consider moving them to a separate constants file or adding a comment explaining their purpose. Otherwise, they could be removed to reduce confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/DeployDonationHandler.s.sol` around lines 14 - 17, The four unused constants ETHEREUM_PROXY_ADMIN, GNOSIS_PROXY_ADMIN, OPTIMISM_PROXY_ADMIN, and POLYGON_PROXY_ADMIN in DeployDonationHandler.s.sol are declared but never referenced (run() creates a new ProxyAdmin via TransparentUpgradeableProxy); either remove these constants to avoid confusion or clearly document their purpose (e.g., comment that they are addresses of existing deployed ProxyAdmin contracts) or move them into a dedicated constants/config file for reference; update any top-of-file declarations and add a short comment referencing TransparentUpgradeableProxy and run() so it’s clear why they’re unused.package.json (2)
19-19: Inconsistent private key handling between deploy scripts.
deploy:mainnetuses--account $MAINNET_DEPLOYER_NAME(Foundry's encrypted keystore), butdeploy:sepoliadoes not specify an account flag, implying it relies onPRIVATE_KEYenv var. This inconsistency could cause confusion or deployment failures.Consider aligning the approach—either both use keystore accounts or both use
PRIVATE_KEY:Option: Use keystore for Sepolia
- "deploy:sepolia": "bash -c 'source .env && forge script script/DeployDonationHandler.s.sol:DeployDonationHandler --rpc-url $SEPOLIA_RPC --broadcast --verify -vvvv'", + "deploy:sepolia": "bash -c 'source .env && forge script script/DeployDonationHandler.s.sol:DeployDonationHandler --rpc-url $SEPOLIA_RPC --account $SEPOLIA_DEPLOYER_NAME --broadcast --verify -vvvv'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 19, The deploy scripts are inconsistent: "deploy:mainnet" uses Forge's keystore account flag (--account $MAINNET_DEPLOYER_NAME) while "deploy:sepolia" relies on an env private key; pick one approach and make both scripts consistent. Update the "deploy:sepolia" npm script (the value that runs forge script script/DeployDonationHandler.s.sol:DeployDonationHandler ...) to either add the same keystore flag (--account $SEPOLIA_DEPLOYER_NAME) if you want keystore-based deploys, or explicitly pass the private key flag (e.g., using the equivalent Forge option or ensuring $PRIVATE_KEY is used consistently), and mirror the choice in "deploy:mainnet" so both scripts use the same authentication method.
32-34: Upgrade scripts also lack--accountflag for key management.The upgrade scripts read
PRIVATE_KEYdirectly from env (as seen inUpgradeDonationHandler.s.solline 13), whiledeploy:mainnetuses Foundry's encrypted keystore via--account. For production mainnet upgrades, using the keystore is more secure as it avoids plaintext private keys in environment variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 32 - 34, The upgrade npm scripts ("upgrade:mainnet", "upgrade:mainnet:simulate", "upgrade:sepolia") currently rely on a plaintext PRIVATE_KEY in env (used in UpgradeDonationHandler.s.sol line 13); update these scripts to pass Foundry's keystore via the --account flag instead of relying on PRIVATE_KEY: change each script to include --account $ACCOUNT (or the appropriate keystore identifier/environment variable) and ensure your .env and deployment docs set ACCOUNT to the encrypted keystore/alias used by Foundry; keep the simulate script behavior but also pass --account so key management is consistent for testing and production upgrades.script/UpgradeDonationHandler.s.sol (1)
12-39: Consider logging the deployer address for auditability.The script logs proxy and implementation addresses but not the deployer/caller address. Adding this would help with post-deployment verification and audit trails.
💡 Suggested enhancement
address proxyAddress = vm.envAddress('PROXY_ADDRESS'); address proxyAdminAddress = vm.envAddress('PROXY_ADMIN_ADDRESS'); + address deployer = vm.addr(deployerPrivateKey); console.log('=== Upgrading DonationHandler ==='); + console.log('Deployer:', deployer); console.log('Proxy Address:', proxyAddress);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/UpgradeDonationHandler.s.sol` around lines 12 - 39, Add logging for the deployer address in run(): derive the deployer address from the existing deployerPrivateKey (use vm.addr(deployerPrivateKey)) and log it before vm.startBroadcast so the script prints who is performing the upgrade; reference the run() function, deployerPrivateKey variable, and vm.startBroadcast to locate where to insert the new deployer address calculation and console.log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 14-20: The .env.example is missing the PRIVATE_KEY entry required
by the deployment/upgrade flow; add a PRIVATE_KEY= line to the template
(alongside SAFE_ADDRESS, PROXY_ADMIN_ADDRESS, etc.) with a short comment noting
it's required for Forge scripts (e.g., "required: private key for deployer") so
users copying .env.example won't have missing env variable errors when code
reads process.env.PRIVATE_KEY or any script referencing PRIVATE_KEY.
In `@docs/MULTISIG_UPGRADE_GUIDE.md`:
- Around line 144-163: Replace the placeholder selector guidance with a
reference to the repository script and/or the exact cast command so signers get
deterministic calldata; specifically tell them to run the provided yarn script
named upgrade:generate-payload (which invokes cast calldata
"upgrade(address,address)" <proxy> <implementation>) or run cast calldata
directly to produce the full calldata instead of manually using a partial
selector like 0x99.... Update the text around the upgrade(address,address)
example and the “Selector: 0x99...” line to point to the yarn script or the cast
command and explain that it outputs the exact hex calldata to paste into the
Safe UI.
In `@PRODUCTION_UPGRADE.md`:
- Around line 158-163: The guide's manual upgrade commands for Gnosis and Celo
use the forge script invocation "forge script
script/UpgradeDonationHandler.s.sol:UpgradeDonationHandler" without the legacy
flag; update the PRODUCTION_UPGRADE.md commands that target Gnosis and Celo to
include the "--legacy" flag (i.e., add "--legacy" to the existing forge script
lines) so those transactions use legacy-style signing consistent with
scripts/deploy-implementation.sh handling of gnosis and celo.
In `@README.md`:
- Around line 296-303: The README shows using cast call to invoke
implementation() on a TransparentUpgradeableProxy, which will revert for
non-admin callers; instead update the verification instructions to read the
implementation address from the EIP-1967 storage slot (the bytes32 slot computed
from keccak256("eip1967.proxy.implementation") - 1) for the given PROXY_ADDRESS
and MAINNET_RPC; reference TransparentUpgradeableProxy and implementation() in
the explanation and replace the failing cast call with a command that reads the
implementation storage slot directly.
In `@script/DeployDonationHandlerImplementation.s.sol`:
- Around line 25-28: The printed shell export includes an unintended space
because console.log is called with two arguments; update the hand-off console
output (the three console.log lines around the proxy handoff that reference
address(implementation)) so the export is emitted as a single contiguous string
(no space after '=') — e.g., concatenate or interpolate 'export
NEW_IMPLEMENTATION_ADDRESS=' and address(implementation) into one argument so a
user can copy-paste a valid shell assignment.
In `@scripts/generate-upgrade-payload.sh`:
- Around line 73-100: The JSON currently declares contractMethod name "upgrade"
with two inputs; update it to "upgradeAndCall" and add the third parameter for
the call data: in the contractMethod object change "name" from "upgrade" to
"upgradeAndCall" and add an inputs entry { "name": "data", "type": "bytes" };
then update contractInputsValues to include "data": "$CALLDATA" (using the
existing CALLDATA variable) alongside "proxy": "$PROXY" and "implementation":
"$NEW_IMPL" so the JSON matches the upgradeAndCall(proxy, implementation, data)
signature.
- Around line 36-37: The script currently builds calldata using the removed
ProxyAdmin signature "upgrade(address,address)"; update all occurrences to use
"upgradeAndCall(address,address,bytes)" with the empty bytes argument "0x"
(e.g., set CALLDATA via cast calldata "upgradeAndCall(address,address,bytes)"
"$PROXY" "$NEW_IMPL" "0x"), and mirror this change in all user-visible strings
and JSON fields (console docs, safe UI instructions, tx description and the JSON
contractMethod) so they reference upgradeAndCall and include the empty data
parameter.
In `@scripts/submit-upgrade-to-safe.mjs`:
- Around line 125-127: The script is encoding the wrong function signature;
replace the use of ethers.Interface with the ProxyAdmin v5 signature by creating
an Interface for 'function upgradeAndCall(address proxy, address implementation,
bytes data)' and change the encodeFunctionData call from 'upgrade' to
'upgradeAndCall', passing [proxy, newImplementation, "0x"] (or an appropriate
init calldata) so the generated tx matches OpenZeppelin v5 ProxyAdmin's
upgradeAndCall(proxy, implementation, bytes) expectation; update any reference
to iface.encodeFunctionData('upgrade', ...) accordingly.
In `@TESTNET_DEPLOYMENT.md`:
- Around line 134-137: The script uses a direct selector call ("0x5c60da1b")
against $PROXY_ADDRESS to read the implementation; replace that with an EIP-1967
storage-slot read or a ProxyAdmin helper: use eth_getStorageAt for slot
0x360894a13ba1a3210667c828492db98dca3e2848 (or call
ProxyAdmin.getProxyImplementation(proxy) / ERC1967Utils.getImplementation()) to
retrieve the implementation address reliably, updating the occurrences where
OLD_IMPL is set (the block referencing $PROXY_ADDRESS and selector 0x5c60da1b
and the similar block later) so the script reads the storage slot or calls the
admin helper instead of using the selector.
---
Nitpick comments:
In `@package.json`:
- Line 19: The deploy scripts are inconsistent: "deploy:mainnet" uses Forge's
keystore account flag (--account $MAINNET_DEPLOYER_NAME) while "deploy:sepolia"
relies on an env private key; pick one approach and make both scripts
consistent. Update the "deploy:sepolia" npm script (the value that runs forge
script script/DeployDonationHandler.s.sol:DeployDonationHandler ...) to either
add the same keystore flag (--account $SEPOLIA_DEPLOYER_NAME) if you want
keystore-based deploys, or explicitly pass the private key flag (e.g., using the
equivalent Forge option or ensuring $PRIVATE_KEY is used consistently), and
mirror the choice in "deploy:mainnet" so both scripts use the same
authentication method.
- Around line 32-34: The upgrade npm scripts ("upgrade:mainnet",
"upgrade:mainnet:simulate", "upgrade:sepolia") currently rely on a plaintext
PRIVATE_KEY in env (used in UpgradeDonationHandler.s.sol line 13); update these
scripts to pass Foundry's keystore via the --account flag instead of relying on
PRIVATE_KEY: change each script to include --account $ACCOUNT (or the
appropriate keystore identifier/environment variable) and ensure your .env and
deployment docs set ACCOUNT to the encrypted keystore/alias used by Foundry;
keep the simulate script behavior but also pass --account so key management is
consistent for testing and production upgrades.
In `@script/DeployDonationHandler.s.sol`:
- Around line 14-17: The four unused constants ETHEREUM_PROXY_ADMIN,
GNOSIS_PROXY_ADMIN, OPTIMISM_PROXY_ADMIN, and POLYGON_PROXY_ADMIN in
DeployDonationHandler.s.sol are declared but never referenced (run() creates a
new ProxyAdmin via TransparentUpgradeableProxy); either remove these constants
to avoid confusion or clearly document their purpose (e.g., comment that they
are addresses of existing deployed ProxyAdmin contracts) or move them into a
dedicated constants/config file for reference; update any top-of-file
declarations and add a short comment referencing TransparentUpgradeableProxy and
run() so it’s clear why they’re unused.
In `@script/UpgradeDonationHandler.s.sol`:
- Around line 12-39: Add logging for the deployer address in run(): derive the
deployer address from the existing deployerPrivateKey (use
vm.addr(deployerPrivateKey)) and log it before vm.startBroadcast so the script
prints who is performing the upgrade; reference the run() function,
deployerPrivateKey variable, and vm.startBroadcast to locate where to insert the
new deployer address calculation and console.log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ae3789e-e1c9-404d-83fd-b03f80d3ac4e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.env.example.gitignorePRODUCTION_UPGRADE.mdREADME.mdTESTNET_DEPLOYMENT.mddocs/MULTISIG_UPGRADE_GUIDE.mdfoundry.tomlpackage.jsonscript/DeployDonationHandler.s.solscript/DeployDonationHandlerImplementation.s.solscript/TestUpgrade.s.solscript/UpgradeDonationHandler.s.solscripts/deploy-implementation.shscripts/generate-upgrade-payload.shscripts/submit-upgrade-to-safe.mjs
| # For upgrade:submit-to-safe (proposer must be a Safe owner) | ||
| SAFE_ADDRESS= | ||
| PROXY_ADMIN_ADDRESS= | ||
| PROXY_ADDRESS= | ||
| NEW_IMPLEMENTATION_ADDRESS= | ||
| PROPOSER_PRIVATE_KEY= | ||
| SAFE_API_KEY= # required: get at https://developer.safe.global |
There was a problem hiding this comment.
Add PRIVATE_KEY to the example env template.
The new deployment/upgrade flow still depends on PRIVATE_KEY, but this template never documents it. Copying .env.example as-is will leave the Forge scripts failing immediately when they try to read that value.
Suggested patch
+PRIVATE_KEY=
+
MAINNET_RPC=
MAINNET_DEPLOYER_NAME=🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 16-16: [UnorderedKey] The PROXY_ADMIN_ADDRESS key should go before the SAFE_ADDRESS key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The PROXY_ADDRESS key should go before the PROXY_ADMIN_ADDRESS key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEW_IMPLEMENTATION_ADDRESS key should go before the PROXY_ADDRESS key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The PROPOSER_PRIVATE_KEY key should go before the PROXY_ADDRESS key
(UnorderedKey)
[warning] 20-20: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 20-20: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 14 - 20, The .env.example is missing the
PRIVATE_KEY entry required by the deployment/upgrade flow; add a PRIVATE_KEY=
line to the template (alongside SAFE_ADDRESS, PROXY_ADMIN_ADDRESS, etc.) with a
short comment noting it's required for Forge scripts (e.g., "required: private
key for deployer") so users copying .env.example won't have missing env variable
errors when code reads process.env.PRIVATE_KEY or any script referencing
PRIVATE_KEY.
| # Upgrade | ||
| forge script script/UpgradeDonationHandler.s.sol:UpgradeDonationHandler \ | ||
| --rpc-url $GNOSIS_RPC \ | ||
| --broadcast \ | ||
| --verify \ | ||
| -vvvv |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PRODUCTION_UPGRADE.md" -o -name "deploy-implementation.sh" | head -20Repository: Giveth/donation-handler-foundry
Length of output: 133
🏁 Script executed:
# Check if PRODUCTION_UPGRADE.md exists and view the content at the specified lines
if [ -f "PRODUCTION_UPGRADE.md" ]; then
echo "=== Lines 150-170 of PRODUCTION_UPGRADE.md ==="
sed -n '150,170p' PRODUCTION_UPGRADE.md
echo ""
echo "=== Lines 190-210 of PRODUCTION_UPGRADE.md ==="
sed -n '190,210p' PRODUCTION_UPGRADE.md
fiRepository: Giveth/donation-handler-foundry
Length of output: 1064
🏁 Script executed:
# Look for scripts/deploy-implementation.sh
if [ -f "scripts/deploy-implementation.sh" ]; then
echo "=== scripts/deploy-implementation.sh ==="
cat -n scripts/deploy-implementation.sh
fiRepository: Giveth/donation-handler-foundry
Length of output: 1626
Add --legacy to the Gnosis and Celo upgrade commands.
This PR already special-cases gnosis and celo as legacy-transaction networks in scripts/deploy-implementation.sh (lines 25-30), but the manual forge script commands in the production guide omit that flag. Following this documentation as written will cause transaction failures on those networks, as they do not support EIP-1559 / eth_feeHistory.
Suggested patch
forge script script/UpgradeDonationHandler.s.sol:UpgradeDonationHandler \
--rpc-url $GNOSIS_RPC \
--broadcast \
+ --legacy \
--verify \
-vvvv forge script script/UpgradeDonationHandler.s.sol:UpgradeDonationHandler \
--rpc-url $CELO_RPC \
--broadcast \
+ --legacy \
--verify \
-vvvv🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PRODUCTION_UPGRADE.md` around lines 158 - 163, The guide's manual upgrade
commands for Gnosis and Celo use the forge script invocation "forge script
script/UpgradeDonationHandler.s.sol:UpgradeDonationHandler" without the legacy
flag; update the PRODUCTION_UPGRADE.md commands that target Gnosis and Celo to
include the "--legacy" flag (i.e., add "--legacy" to the existing forge script
lines) so those transactions use legacy-style signing consistent with
scripts/deploy-implementation.sh handling of gnosis and celo.
| ### ✅ Verify Upgrade | ||
|
|
||
| After upgrading, verify the new implementation: | ||
|
|
||
| ```bash | ||
| # Check current implementation address | ||
| cast call $PROXY_ADDRESS "implementation()(address)" --rpc-url $MAINNET_RPC | ||
| ``` |
There was a problem hiding this comment.
Incorrect verification command for TransparentUpgradeableProxy.
The implementation() function on a TransparentUpgradeableProxy is only callable by the admin (ProxyAdmin contract), not by external callers. This cast call will revert or return unexpected results.
To verify the implementation, read the EIP-1967 implementation slot directly:
📝 Corrected verification command
-# Check current implementation address
-cast call $PROXY_ADDRESS "implementation()(address)" --rpc-url $MAINNET_RPC
+# Check current implementation address (EIP-1967 slot)
+cast storage $PROXY_ADDRESS 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc --rpc-url $MAINNET_RPCThe slot 0x360894... is bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1).
📝 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.
| ### ✅ Verify Upgrade | |
| After upgrading, verify the new implementation: | |
| ```bash | |
| # Check current implementation address | |
| cast call $PROXY_ADDRESS "implementation()(address)" --rpc-url $MAINNET_RPC | |
| ``` | |
| ### ✅ Verify Upgrade | |
| After upgrading, verify the new implementation: | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 296 - 303, The README shows using cast call to invoke
implementation() on a TransparentUpgradeableProxy, which will revert for
non-admin callers; instead update the verification instructions to read the
implementation address from the EIP-1967 storage slot (the bytes32 slot computed
from keccak256("eip1967.proxy.implementation") - 1) for the given PROXY_ADDRESS
and MAINNET_RPC; reference TransparentUpgradeableProxy and implementation() in
the explanation and replace the failing cast call with a command that reads the
implementation storage slot directly.
| console.log('\n=== Hand off to proxy owner ==='); | ||
| console.log('Give this address to the ProxyAdmin owner to run:'); | ||
| console.log(' proxyAdmin.upgrade(proxy,', address(implementation), ')'); | ||
| console.log('export NEW_IMPLEMENTATION_ADDRESS=', address(implementation)); |
There was a problem hiding this comment.
Emit copy-pasteable hand-off commands.
console.log('export NEW_IMPLEMENTATION_ADDRESS=', address(implementation)) prints a space after =, so the shown command is not a valid shell assignment if someone pastes it directly.
Suggested patch
vm.stopBroadcast();
console.log('\n=== Hand off to proxy owner ===');
console.log('Give this address to the ProxyAdmin owner to run:');
- console.log(' proxyAdmin.upgrade(proxy,', address(implementation), ')');
- console.log('export NEW_IMPLEMENTATION_ADDRESS=', address(implementation));
+ console.log(string.concat(' proxyAdmin.upgrade(proxy, ', vm.toString(address(implementation)), ')'));
+ console.log(string.concat('export NEW_IMPLEMENTATION_ADDRESS=', vm.toString(address(implementation))));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/DeployDonationHandlerImplementation.s.sol` around lines 25 - 28, The
printed shell export includes an unintended space because console.log is called
with two arguments; update the hand-off console output (the three console.log
lines around the proxy handoff that reference address(implementation)) so the
export is emitted as a single contiguous string (no space after '=') — e.g.,
concatenate or interpolate 'export NEW_IMPLEMENTATION_ADDRESS=' and
address(implementation) into one argument so a user can copy-paste a valid shell
assignment.
| # Build calldata: upgrade(address proxy, address implementation) | ||
| CALLDATA=$(cast calldata "upgrade(address,address)" "$PROXY" "$NEW_IMPL") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenZeppelin v5 ProxyAdmin upgrade function signature
💡 Result:
In OpenZeppelin Contracts v5, ProxyAdmin no longer exposes a separate upgrade(...) function when UPGRADE_INTERFACE_VERSION == "5.0.0"; instead it exposes only:
function upgradeAndCall(
ITransparentUpgradeableProxy proxy,
address implementation,
bytes memory data
) public payable virtual onlyOwner;[1][2]
🏁 Script executed:
# Check the actual content of the script at the mentioned lines
cat -n scripts/generate-upgrade-payload.sh | sed -n '30,100p'Repository: Giveth/donation-handler-foundry
Length of output: 2842
Critical: upgrade(address,address) does not exist in OpenZeppelin v5 ProxyAdmin.
OpenZeppelin v5's ProxyAdmin contract only exposes upgradeAndCall(ITransparentUpgradeableProxy proxy, address implementation, bytes memory data). The upgrade(address,address) function was removed. This script generates invalid calldata that will fail when executed.
The script references the deprecated function in five places (lines 36–37, 48, 64, 79, 91):
Locations requiring fixes
- Line 36–37:
cast calldatacall with deprecated signature - Line 48: Console output documentation
- Line 64: Safe UI instructions
- Line 79: JSON transaction description
- Line 91: JSON contractMethod name
Fix all occurrences to use upgradeAndCall(address,address,bytes) with "0x" for the empty data parameter:
🐛 Fix example
-# Build calldata: upgrade(address proxy, address implementation)
-CALLDATA=$(cast calldata "upgrade(address,address)" "$PROXY" "$NEW_IMPL")
+# Build calldata: upgradeAndCall(address proxy, address implementation, bytes data)
+CALLDATA=$(cast calldata "upgradeAndCall(address,address,bytes)" "$PROXY" "$NEW_IMPL" "0x")Apply the same change to the documentation strings and JSON fields referenced above.
📝 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.
| # Build calldata: upgrade(address proxy, address implementation) | |
| CALLDATA=$(cast calldata "upgrade(address,address)" "$PROXY" "$NEW_IMPL") | |
| # Build calldata: upgradeAndCall(address proxy, address implementation, bytes data) | |
| CALLDATA=$(cast calldata "upgradeAndCall(address,address,bytes)" "$PROXY" "$NEW_IMPL" "0x") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-upgrade-payload.sh` around lines 36 - 37, The script
currently builds calldata using the removed ProxyAdmin signature
"upgrade(address,address)"; update all occurrences to use
"upgradeAndCall(address,address,bytes)" with the empty bytes argument "0x"
(e.g., set CALLDATA via cast calldata "upgradeAndCall(address,address,bytes)"
"$PROXY" "$NEW_IMPL" "0x"), and mirror this change in all user-visible strings
and JSON fields (console docs, safe UI instructions, tx description and the JSON
contractMethod) so they reference upgradeAndCall and include the empty data
parameter.
| cat > "$OUTPUT_JSON" << EOF | ||
| { | ||
| "version": "1.0", | ||
| "chainId": "$CHAIN", | ||
| "meta": { | ||
| "name": "DonationHandler upgrade", | ||
| "description": "ProxyAdmin.upgrade(proxy, implementation)" | ||
| }, | ||
| "transactions": [ | ||
| { | ||
| "to": "$PROXY_ADMIN", | ||
| "value": "0", | ||
| "data": "$CALLDATA", | ||
| "contractMethod": { | ||
| "inputs": [ | ||
| { "name": "proxy", "type": "address" }, | ||
| { "name": "implementation", "type": "address" } | ||
| ], | ||
| "name": "upgrade", | ||
| "payable": false | ||
| }, | ||
| "contractInputsValues": { | ||
| "proxy": "$PROXY", | ||
| "implementation": "$NEW_IMPL" | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
JSON payload also uses wrong function signature.
The contractMethod in the generated JSON references upgrade with only two address inputs. This needs to be updated to upgradeAndCall with three parameters (proxy, implementation, data).
🐛 Proposed fix for JSON structure
"contractMethod": {
"inputs": [
{ "name": "proxy", "type": "address" },
- { "name": "implementation", "type": "address" }
+ { "name": "implementation", "type": "address" },
+ { "name": "data", "type": "bytes" }
],
- "name": "upgrade",
+ "name": "upgradeAndCall",
"payable": false
},
"contractInputsValues": {
"proxy": "$PROXY",
- "implementation": "$NEW_IMPL"
+ "implementation": "$NEW_IMPL",
+ "data": "0x"
}📝 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.
| cat > "$OUTPUT_JSON" << EOF | |
| { | |
| "version": "1.0", | |
| "chainId": "$CHAIN", | |
| "meta": { | |
| "name": "DonationHandler upgrade", | |
| "description": "ProxyAdmin.upgrade(proxy, implementation)" | |
| }, | |
| "transactions": [ | |
| { | |
| "to": "$PROXY_ADMIN", | |
| "value": "0", | |
| "data": "$CALLDATA", | |
| "contractMethod": { | |
| "inputs": [ | |
| { "name": "proxy", "type": "address" }, | |
| { "name": "implementation", "type": "address" } | |
| ], | |
| "name": "upgrade", | |
| "payable": false | |
| }, | |
| "contractInputsValues": { | |
| "proxy": "$PROXY", | |
| "implementation": "$NEW_IMPL" | |
| } | |
| } | |
| ] | |
| } | |
| cat > "$OUTPUT_JSON" << EOF | |
| { | |
| "version": "1.0", | |
| "chainId": "$CHAIN", | |
| "meta": { | |
| "name": "DonationHandler upgrade", | |
| "description": "ProxyAdmin.upgrade(proxy, implementation)" | |
| }, | |
| "transactions": [ | |
| { | |
| "to": "$PROXY_ADMIN", | |
| "value": "0", | |
| "data": "$CALLDATA", | |
| "contractMethod": { | |
| "inputs": [ | |
| { "name": "proxy", "type": "address" }, | |
| { "name": "implementation", "type": "address" }, | |
| { "name": "data", "type": "bytes" } | |
| ], | |
| "name": "upgradeAndCall", | |
| "payable": false | |
| }, | |
| "contractInputsValues": { | |
| "proxy": "$PROXY", | |
| "implementation": "$NEW_IMPL", | |
| "data": "0x" | |
| } | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-upgrade-payload.sh` around lines 73 - 100, The JSON
currently declares contractMethod name "upgrade" with two inputs; update it to
"upgradeAndCall" and add the third parameter for the call data: in the
contractMethod object change "name" from "upgrade" to "upgradeAndCall" and add
an inputs entry { "name": "data", "type": "bytes" }; then update
contractInputsValues to include "data": "$CALLDATA" (using the existing CALLDATA
variable) alongside "proxy": "$PROXY" and "implementation": "$NEW_IMPL" so the
JSON matches the upgradeAndCall(proxy, implementation, data) signature.
| // Calldata: upgrade(address proxy, address implementation) | ||
| const iface = new ethers.Interface(['function upgrade(address proxy, address implementation)']); | ||
| const data = iface.encodeFunctionData('upgrade', [proxy, newImplementation]); |
There was a problem hiding this comment.
Critical: Same upgrade(address,address) signature issue as the shell script.
This script encodes upgrade(address,address) but OpenZeppelin v5's ProxyAdmin only has upgradeAndCall(address,address,bytes). The generated transaction will fail.
🐛 Proposed fix
- // Calldata: upgrade(address proxy, address implementation)
- const iface = new ethers.Interface(['function upgrade(address proxy, address implementation)']);
- const data = iface.encodeFunctionData('upgrade', [proxy, newImplementation]);
+ // Calldata: upgradeAndCall(address proxy, address implementation, bytes data)
+ // Empty bytes since no re-initialization is needed
+ const iface = new ethers.Interface(['function upgradeAndCall(address proxy, address implementation, bytes data)']);
+ const data = iface.encodeFunctionData('upgradeAndCall', [proxy, newImplementation, '0x']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/submit-upgrade-to-safe.mjs` around lines 125 - 127, The script is
encoding the wrong function signature; replace the use of ethers.Interface with
the ProxyAdmin v5 signature by creating an Interface for 'function
upgradeAndCall(address proxy, address implementation, bytes data)' and change
the encodeFunctionData call from 'upgrade' to 'upgradeAndCall', passing [proxy,
newImplementation, "0x"] (or an appropriate init calldata) so the generated tx
matches OpenZeppelin v5 ProxyAdmin's upgradeAndCall(proxy, implementation,
bytes) expectation; update any reference to iface.encodeFunctionData('upgrade',
...) accordingly.
| ```bash | ||
| # Get current implementation address | ||
| OLD_IMPL=$(cast call $PROXY_ADDRESS "0x5c60da1b" --rpc-url $SEPOLIA_RPC) | ||
| echo "Old Implementation: $OLD_IMPL" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenZeppelin TransparentUpgradeableProxy / ProxyAdmin, what is the supported way to read the current implementation address after an upgrade: calling selector 0x5c60da1b on the proxy, calling a ProxyAdmin helper, or reading the EIP-1967 implementation slot?
💡 Result:
The supported/stable way is to read the ERC‑1967 (EIP‑1967) implementation storage slot (0x360894a13b…2bbc) via eth_getStorageAt (off-chain), or equivalently via OZ helpers like ERC1967Utils.getImplementation() (in-contract). OpenZeppelin explicitly documents this as the way “clients can read” the current implementation. [1]
ProxyAdmin.getProxyImplementation(proxy) is just a convenience wrapper (mainly for the admin/owner workflow) that does a staticcall to selector 0x5c60da1b (implementation()) on the proxy. [2]
Directly calling selector 0x5c60da1b on the proxy is not the generally supported/client approach, because implementation() is an admin-only introspection function in the transparent proxy pattern (and newer TransparentUpgradeableProxy versions don’t even expose a normal ABI), so it’s not reliable for arbitrary callers. [3]
So:
- Best/supported for “read current implementation after upgrade”: read the ERC‑1967 implementation slot. [1]
ProxyAdmin.getProxyImplementation: OK as an admin tool; it internally uses0x5c60da1b. [2]- Calling
0x5c60da1byourself: not the recommended/general client method; admin-gated/not ABI-stable. [2][3]
References:
[1] (docs.openzeppelin.com)
[2] (forum.openzeppelin.com)
[3] (docs.openzeppelin.com)
Citations:
- 1: https://docs.openzeppelin.com/contracts/5.x/api/proxy
- 2: https://forum.openzeppelin.com/t/wny-does-getproxyimplementation-method-in-proxyadmin-contract-need-to-be-view/35372
- 3: https://docs.openzeppelin.com/contracts/3.x/api/proxy
🏁 Script executed:
cat -n TESTNET_DEPLOYMENT.md | sed -n '130,175p'Repository: Giveth/donation-handler-foundry
Length of output: 1428
Replace 0x5c60da1b selector calls with EIP-1967 slot reads or ProxyAdmin helpers.
The direct cast call approach using selector 0x5c60da1b is not the supported way to read a transparent proxy's implementation address. OpenZeppelin documents that clients should read the ERC-1967 implementation storage slot (0x360894a13ba1a3210667c828492db98dca3e2848), or use ProxyAdmin.getProxyImplementation(proxy) if operating as the admin. Direct selector calls can fail even when the upgrade succeeded.
Update both locations (lines 134–137 and 167–170) to use eth_getStorageAt or an equivalent in-contract helper like ERC1967Utils.getImplementation().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TESTNET_DEPLOYMENT.md` around lines 134 - 137, The script uses a direct
selector call ("0x5c60da1b") against $PROXY_ADDRESS to read the implementation;
replace that with an EIP-1967 storage-slot read or a ProxyAdmin helper: use
eth_getStorageAt for slot 0x360894a13ba1a3210667c828492db98dca3e2848 (or call
ProxyAdmin.getProxyImplementation(proxy) / ERC1967Utils.getImplementation()) to
retrieve the implementation address reliably, updating the occurrences where
OLD_IMPL is set (the block referencing $PROXY_ADDRESS and selector 0x5c60da1b
and the similar block later) so the script reads the storage slot or calls the
admin helper instead of using the selector.
- Replaced upgrade scripts in package.json with a new direct upgrade command. - Expanded MULTISIG_UPGRADE_GUIDE.md to clarify upgrade flows for both multisig and EOA owners. - Updated UpgradeDonationHandler script to utilize pre-deployed implementation addresses. - Improved clarity in upgrade instructions and environment variable requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/upgrade-direct.sh`:
- Around line 21-22: The script sources .env but doesn't export those variables
to child processes, causing forge script to run without PRIVATE_KEY and other
vars; wrap the sourcing with automatic export using set -a before sourcing and
set +a after to ensure variables (e.g., PRIVATE_KEY used/validated and consumed
by forge script calls) are exported to child processes, update the block around
the source .env lines accordingly so validation still runs but forge script
inherits the env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c45a89c2-5806-4e89-b958-5c08e129b50f
📒 Files selected for processing (4)
docs/MULTISIG_UPGRADE_GUIDE.mdpackage.jsonscript/UpgradeDonationHandler.s.solscripts/upgrade-direct.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/MULTISIG_UPGRADE_GUIDE.md
| cd "$(dirname "$0")/.." | ||
| source .env |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect dotenv files without executing them.
fd -HI '^\.env(\.example)?$'
rg -n '^(export[[:space:]]+)?(PRIVATE_KEY|PROXY_ADDRESS|PROXY_ADMIN_ADDRESS|NEW_IMPLEMENTATION_ADDRESS|[A-Z0-9_]+_RPC)=' .env .env.example 2>/dev/null || true
# Demonstrate the Bash behavior this script currently relies on.
bash -lc '
PRIVATE_KEY=demo
if env | grep -q "^PRIVATE_KEY="; then
echo "plain assignment exported"
else
echo "plain assignment is NOT exported to child processes"
fi
'Repository: Giveth/donation-handler-foundry
Length of output: 589
🏁 Script executed:
# Check the exact content of scripts/upgrade-direct.sh around lines 21-22
head -n 30 scripts/upgrade-direct.sh | tail -n +15Repository: Giveth/donation-handler-foundry
Length of output: 544
🏁 Script executed:
# Verify the full context: check if forge script is invoked and what env vars it expects
grep -n "forge script\|PRIVATE_KEY" scripts/upgrade-direct.shRepository: Giveth/donation-handler-foundry
Length of output: 532
Export the sourced .env variables before calling forge script.
Lines 21–22 source .env without exporting variables, so they exist only in the current shell. Line 33 validates PRIVATE_KEY succeeds, but forge script (lines 62, 69) runs as a child process and won't inherit these variables. The validation checks will pass while forge script fails with missing env vars. Wrap the source with set -a / set +a to auto-export all sourced variables.
Proposed fix
cd "$(dirname "$0")/.."
-source .env
+set -a
+source .env
+set +a📝 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.
| cd "$(dirname "$0")/.." | |
| source .env | |
| cd "$(dirname "$0")/.." | |
| set -a | |
| source .env | |
| set +a |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/upgrade-direct.sh` around lines 21 - 22, The script sources .env but
doesn't export those variables to child processes, causing forge script to run
without PRIVATE_KEY and other vars; wrap the sourcing with automatic export
using set -a before sourcing and set +a after to ensure variables (e.g.,
PRIVATE_KEY used/validated and consumed by forge script calls) are exported to
child processes, update the block around the source .env lines accordingly so
validation still runs but forge script inherits the env.
2 participants
Add an script to deploy a new version of the contract and upgrade it in the proxy contract
Summary by CodeRabbit
New Features
Documentation
Chores