chore: verify proxy and implementation contracts#1005
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughDeployment, upgrade, and verification scripts were made proxy-aware: proxies are deployed with Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Deploy/Verify CLI
participant Proxy as TransparentUpgradeableProxy
participant ProxyAdmin as ProxyAdmin
participant Impl as Implementation
participant Store as deployed_contracts.json
CLI->>Proxy: deploy proxy (constructor arg: owner)
Proxy-->>CLI: returns proxyAddress
CLI->>ProxyAdmin: query admin for proxyAddress
ProxyAdmin-->>CLI: returns proxyAdminAddress
CLI->>Store: persist proxyRecords {initData, initialOwner, proxyAddress, proxyAdminAddress, implementationAddress}
note right of CLI: Verification (proxy-aware)
CLI->>Store: read proxyRecords → detect proxy
CLI->>Proxy: verify TransparentUpgradeableProxy (constructor args from proxyRecords)
CLI->>ProxyAdmin: verify ProxyAdmin (constructor args from proxyRecords)
CLI->>Impl: verify implementation at implementationAddress (no constructor args)
Impl-->>CLI: verification results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-19T11:16:53.825ZApplied to files:
📚 Learning: 2025-10-10T12:56:40.538ZApplied to files:
🧬 Code graph analysis (3)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (2)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (2)
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (10)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
219-229: Data loss:proxyRecordsreplacement discards deployment metadata.The upgrade function replaces the entire
proxyRecordsobject with onlyimplementationAddress, discardinginitData,initialOwner,proxyAddress, andproxyAdminAddressfrom the initial deployment. This metadata loss could break verification workflows and make it difficult to track proxy configuration history.Apply this diff to preserve existing proxy metadata:
storeDeploymentArgs( { ...preDeployedArgs, proxyRecords: { + ...preDeployedArgs.proxyRecords, // initData, //TODO: Add init data if needed implementationAddress: newImplementationAddress, }, }, "BondingRegistry", chain, );
🧹 Nitpick comments (1)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
210-211: Track upgrade initialization data requirements.The TODO comments indicate that upgrade initialization data handling is incomplete. While
upgradeAndCallcurrently uses emptyinitData, future implementation changes might require initialization functions to be called during upgrades.Would you like me to open an issue to track this requirement and ensure proper initialization data handling is implemented before any upgrades that require it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/enclave-contracts/deployed_contracts.json(2 hunks)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts(4 hunks)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts(3 hunks)packages/enclave-contracts/scripts/deployAndSave/enclave.ts(4 hunks)packages/enclave-contracts/scripts/utils.ts(1 hunks)packages/enclave-contracts/scripts/verify.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.tspackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/scripts/deployAndSave/enclave.ts
📚 Learning: 2025-10-10T12:56:40.538Z
Learnt from: 0xjei
Repo: gnosisguild/enclave PR: 830
File: templates/default/README.md:123-128
Timestamp: 2025-10-10T12:56:40.538Z
Learning: In the Enclave repository, the hard-coded Hardhat development private key `0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80` is acceptable in template README files and documentation for local testing/interaction purposes.
Applied to files:
packages/enclave-contracts/deployed_contracts.json
🧬 Code graph analysis (3)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
packages/enclave-contracts/scripts/proxy.ts (1)
getProxyAdmin(22-35)
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (2)
packages/enclave-contracts/scripts/proxy.ts (1)
getProxyAdmin(22-35)packages/enclave-contracts/scripts/utils.ts (1)
storeDeploymentArgs(35-67)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
packages/enclave-contracts/scripts/proxy.ts (1)
getProxyAdmin(22-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: test_contracts
- GitHub Check: rust_integration
🔇 Additional comments (4)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (4)
18-29: LGTM: Required fields improve type safety.Making all deployment parameters required aligns well with the validation logic below and prevents runtime errors from missing configuration.
118-118: LGTM: Correct admin parameter for transparent proxy.Passing
owneras the admin parameter ensures the ProxyAdmin contract is owned by the specified owner address rather than the deploying signer.
122-123: LGTM: ERC1967-compliant ProxyAdmin address retrieval.Retrieving the ProxyAdmin address via the ERC1967 storage slot is the correct approach for transparent proxies.
137-143: LGTM: Comprehensive proxy metadata storage.The new
proxyRecordsstructure captures all essential proxy-related information, enabling proper verification and upgrade workflows.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
204-207: Duplicate: Keep proxyRecords intact during upgradesThis issue has already been flagged in a previous review comment on lines 205-207. The upgrade path overwrites
proxyRecordswith only the newimplementationAddress, discarding critical metadata needed byverify.ts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts(3 hunks)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts(2 hunks)packages/enclave-contracts/scripts/deployAndSave/enclave.ts(3 hunks)packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts(1 hunks)packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts(1 hunks)packages/enclave-contracts/scripts/upgrade/enclave.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
packages/enclave-contracts/scripts/upgrade/enclave.tspackages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.tspackages/enclave-contracts/scripts/deployAndSave/bondingRegistry.tspackages/enclave-contracts/scripts/upgrade/bondingRegistry.tspackages/enclave-contracts/scripts/deployAndSave/enclave.ts
🧬 Code graph analysis (2)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
packages/enclave-contracts/scripts/proxy.ts (1)
getProxyAdmin(22-35)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
packages/enclave-contracts/scripts/proxy.ts (1)
getProxyAdmin(22-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: rust_unit
🔇 Additional comments (7)
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (1)
29-33: LGTM!The validation now correctly checks for proxy-based deployments using the new
proxyRecordsstructure, aligning with the updated deployment metadata format.packages/enclave-contracts/scripts/upgrade/enclave.ts (1)
29-33: LGTM!The validation correctly enforces proxy-based deployment requirements using the new
proxyRecords.implementationAddressfield.packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (1)
34-38: LGTM!The validation is consistent with other upgrade scripts and correctly enforces the proxy-based deployment requirement.
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (2)
105-107: LGTM!Correctly deploys the TransparentUpgradeableProxy with
owneras the initial admin, which is the proper pattern for this proxy type.
109-127: LGTM!The deployment correctly retrieves the proxy admin address and stores comprehensive proxy metadata including initData, initialOwner, proxyAddress, proxyAdminAddress, and implementationAddress. This provides all necessary information for future upgrades and verification.
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (2)
118-120: LGTM!Correctly deploys the TransparentUpgradeableProxy with
owneras the initial admin parameter.
122-143: LGTM!The deployment correctly retrieves the proxy admin address and stores comprehensive metadata in
proxyRecords, ensuring all necessary information is available for verification and future upgrades.
ctrlc03
force-pushed
the
chore/verify-proxy-contracts
branch
from
354c210 to
09928a5
Compare
ctrlc03
force-pushed
the
chore/verify-proxy-contracts
branch
from
09928a5 to
4813127
Compare
2 participants
Summary by CodeRabbit
Chores
Refactor
Bug Fixes