feat: deploy transparent proxy contracts [skip-line-limit]#987
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughSwitches Enclave, BondingRegistry, and CiphernodeRegistryOwnable to an upgradeable proxy pattern: constructors now disable initializers, initialization is performed via initialize(), ignition/deploy scripts deploy implementations plus TransparentUpgradeableProxy with encoded init data, added proxy utilities and upgrade/upgrade-scripts, and updated example/config deployment addresses and artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Deployer
participant Ign as Ignition / Scripts
participant Impl as Implementation (uninitialized)
participant Proxy as TransparentUpgradeableProxy
participant Admin as ProxyAdmin
rect rgb(240,248,255)
Note over Dev,Impl: Old direct-constructor flow
Dev->>Ign: deploy Contract(owner, registry, ...)
Ign->>Impl: new Contract(...constructor...)
Impl-->>Ign: Deployed & initialized
end
rect rgb(255,250,240)
Note over Dev,Proxy: New proxy + initializer flow
Dev->>Ign: deploy implementation (no args)
Ign->>Impl: new Contract()
Impl-->>Ign: Implementation address
Ign->>Ign: encode initialize(owner, registry, ...)
Ign->>Proxy: deploy TransparentUpgradeableProxy(impl, owner, initData)
Proxy->>Impl: delegatecall initialize(...)
Impl-->>Proxy: initialized state via proxy
Proxy-->>Ign: Proxy address (usable)
end
rect rgb(245,255,250)
Note over Dev,Admin: Upgrade flow
Dev->>Admin: upgradeAndCall(proxy, newImpl, 0x)
Admin->>Proxy: change implementation pointer
Proxy-->>Admin: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
hmzakhalid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
179-180: Address the TODO comment for initialization data.The upgrade flow uses a placeholder
"0x"for initialization data. If the upgrade requires calling an initialization function on the new implementation, this will need to be implemented.Do you want me to help implement the initialization data encoding, or would you like me to open a new issue to track this task?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
examples/CRISP/Readme.md(2 hunks)examples/CRISP/client/.env.example(1 hunks)examples/CRISP/enclave.config.yaml(1 hunks)examples/CRISP/packages/crisp-contracts/deployed_contracts.json(1 hunks)examples/CRISP/packages/crisp-contracts/hardhat.config.ts(1 hunks)examples/CRISP/server/.env.example(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(1 hunks)packages/enclave-contracts/contracts/Enclave.sol(1 hunks)packages/enclave-contracts/contracts/registry/BondingRegistry.sol(3 hunks)packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol(1 hunks)packages/enclave-contracts/deployed_contracts.json(1 hunks)packages/enclave-contracts/hardhat.config.ts(1 hunks)packages/enclave-contracts/ignition/modules/bondingRegistry.ts(2 hunks)packages/enclave-contracts/ignition/modules/ciphernodeRegistry.ts(1 hunks)packages/enclave-contracts/ignition/modules/enclave.ts(1 hunks)packages/enclave-contracts/package.json(1 hunks)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts(3 hunks)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts(3 hunks)packages/enclave-contracts/scripts/deployAndSave/enclave.ts(3 hunks)packages/enclave-contracts/scripts/proxy.ts(1 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)packages/enclave-contracts/scripts/utils.ts(1 hunks)packages/enclave-contracts/test/Enclave.spec.ts(5 hunks)packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts(3 hunks)templates/default/enclave.config.yaml(1 hunks)templates/default/hardhat.config.ts(1 hunks)tests/integration/enclave.config.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
📚 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/ciphernodeRegistryOwnable.tspackages/enclave-contracts/scripts/deployAndSave/bondingRegistry.tspackages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.tspackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonpackages/enclave-contracts/contracts/Enclave.solexamples/CRISP/enclave.config.yamltemplates/default/enclave.config.yamlpackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/ignition/modules/ciphernodeRegistry.tspackages/enclave-contracts/package.jsonpackages/enclave-contracts/deployed_contracts.jsonpackages/enclave-contracts/scripts/upgrade/enclave.tstests/integration/enclave.config.yamlpackages/enclave-contracts/scripts/upgrade/bondingRegistry.tspackages/enclave-contracts/ignition/modules/bondingRegistry.tspackages/enclave-contracts/ignition/modules/enclave.ts
📚 Learning: 2025-09-11T13:02:56.353Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/mockComputeProvider.ts:6-6
Timestamp: 2025-09-11T13:02:56.353Z
Learning: In Hardhat v3, the import path "hardhat/types/hre" may be valid for importing HardhatRuntimeEnvironment, unlike in Hardhat v2 where the standard path was "hardhat/types".
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/enclave.ts
📚 Learning: 2025-09-11T13:02:56.353Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/mockComputeProvider.ts:6-6
Timestamp: 2025-09-11T13:02:56.353Z
Learning: In Hardhat v3, the import path "hardhat/types/hre" is valid for importing HardhatRuntimeEnvironment, which is different from Hardhat v2 where "hardhat/types" was the standard path.
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/enclave.ts
📚 Learning: 2025-09-11T13:02:56.353Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/mockComputeProvider.ts:6-6
Timestamp: 2025-09-11T13:02:56.353Z
Learning: In Hardhat v3, the correct import path for HardhatRuntimeEnvironment is "hardhat/types/hre", not "hardhat/types" as it was in Hardhat v2. The import structure has changed between major versions.
Applied to files:
packages/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/scripts/deployAndSave/enclave.tsexamples/CRISP/client/.env.exampletemplates/default/enclave.config.yamlexamples/CRISP/server/.env.exampleexamples/CRISP/Readme.md
📚 Learning: 2025-09-11T13:09:03.800Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 677
File: packages/enclave-contracts/scripts/deployAndSave/naiveRegistryFilter.ts:30-31
Timestamp: 2025-09-11T13:09:03.800Z
Learning: In Hardhat v3 deployment scripts, use `(await signer.provider?.getNetwork())?.name ?? "localhost"` instead of `hre.globalOptions.network` to reliably get the network name, as `hre.globalOptions.network` can be undefined in some contexts.
Applied to files:
packages/enclave-contracts/scripts/deployAndSave/enclave.tspackages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/enclave.config.yaml
📚 Learning: 2025-10-03T08:38:36.786Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 781
File: examples/CRISP/test/governanceCircuit.test.ts:15-50
Timestamp: 2025-10-03T08:38:36.786Z
Learning: Hardhat v3 supports Node.js's native test runner (node:test) including importing describe and it from 'node:test'. Tests using this interface are compatible with Hardhat v3's test task.
Applied to files:
packages/enclave-contracts/package.json
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
examples/CRISP/Readme.md
📚 Learning: 2024-10-23T01:59:27.215Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: tests/basic_integration/test.sh:21-21
Timestamp: 2024-10-23T01:59:27.215Z
Learning: In `tests/basic_integration/test.sh`, the hardcoded `CIPHERNODE_SECRET` is acceptable for testing purposes and does not need to be changed.
Applied to files:
examples/CRISP/Readme.md
📚 Learning: 2024-10-23T02:03:02.008Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/keyshare/src/encryption.rs:45-45
Timestamp: 2024-10-23T02:03:02.008Z
Learning: In the `packages/ciphernode/keyshare/src/encryption.rs` file, the environment variable `CIPHERNODE_SECRET` is used for the encryption password. A secure secret management solution is not currently available, but may be considered in future iterations.
Applied to files:
examples/CRISP/Readme.md
📚 Learning: 2025-11-08T01:31:47.484Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 969
File: packages/enclave-sdk/src/enclave-sdk.ts:101-103
Timestamp: 2025-11-08T01:31:47.484Z
Learning: In packages/enclave-sdk/src/enclave-sdk.ts, the TRBFV protocol parameter setup is intentionally incomplete as of PR #969. The constructor sets BFV_THRESHOLD parameters for TRBFV, but the encryption methods (encryptNumber, encryptVector, encryptNumberAndGenInputs, encryptVectorAndGenInputs) intentionally only support BFV and will throw "Protocol not supported" for TRBFV until future implementation is completed.
Applied to files:
packages/enclave-contracts/ignition/modules/enclave.ts
🧬 Code graph analysis (6)
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
upgradeAndSaveCiphernodeRegistryOwnable(123-205)
packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (2)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(75-89)storeDeploymentArgs(35-67)packages/enclave-contracts/scripts/proxy.ts (2)
getProxyAdmin(22-35)verifyProxyAdminOwner(43-54)
packages/enclave-contracts/scripts/deployAndSave/enclave.ts (2)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(75-89)storeDeploymentArgs(35-67)packages/enclave-contracts/scripts/proxy.ts (2)
getProxyAdmin(22-35)verifyProxyAdminOwner(43-54)
packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (2)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(75-89)storeDeploymentArgs(35-67)packages/enclave-contracts/scripts/proxy.ts (2)
getProxyAdmin(22-35)verifyProxyAdminOwner(43-54)
packages/enclave-contracts/scripts/upgrade/enclave.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
upgradeAndSaveEnclave(137-207)
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (2)
packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
upgradeAndSaveBondingRegistry(156-229)
🪛 dotenv-linter (4.0.0)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
examples/CRISP/server/.env.example
[warning] 16-16: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 17-17: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 17-17: [UnorderedKey] The CIPHERNODE_REGISTRY_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
🪛 Gitleaks (8.28.0)
examples/CRISP/packages/crisp-contracts/deployed_contracts.json
[high] 180-180: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/enclave-contracts/deployed_contracts.json
[high] 138-138: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 184-184: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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)
- GitHub Check: test_enclave_init
- GitHub Check: crisp_e2e
- GitHub Check: Build & Push Image
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: Build & Push Image
- GitHub Check: rust_integration
- GitHub Check: rust_unit
🔇 Additional comments (21)
packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
1-539: This is an auto-generated artifact file with only a buildInfoId update—no review needed.The buildInfoId change (line 538) reflects a compiler/toolchain update and is expected during the recompilation cycle accompanying the proxy pattern migration in this PR. All functional elements (ABI, bytecode, interface definition) remain identical.
examples/CRISP/server/.env.example (1)
16-18: Updated deployment addresses reflect new proxy-based deployment pattern.The address updates align with the introduction of transparent upgradeable proxies across the codebase. These are example addresses for local Hardhat/Anvil testing environments.
Note: The static analysis warnings about quote characters and key ordering are style preferences rather than functional issues. Both quoted and unquoted values work correctly in .env files, and key ordering doesn't affect functionality.
packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json (1)
854-854: Expected artifact regeneration from recompilation.The buildInfoId update reflects contract recompilation as part of the upgradeable proxy changes. The interface ABI remains unchanged.
examples/CRISP/client/.env.example (1)
4-4: Updated E3 program address aligns with new proxy deployments.The address update is consistent with the broader deployment reconfiguration for upgradeable proxies.
Note: Static analysis warnings about quotes and key ordering are stylistic preferences. Vite correctly handles both quoted and unquoted environment variable values.
templates/default/hardhat.config.ts (1)
121-122: Proxy contracts correctly added to support upgradeable pattern.The addition of OpenZeppelin's ProxyAdmin and TransparentUpgradeableProxy contracts enables the transparent upgradeable proxy pattern introduced in this PR. These contracts are necessary for proxy-based deployments and upgrades.
packages/enclave-contracts/scripts/utils.ts (1)
16-16: Implementation address field supports proxy deployment tracking.The optional
implementationAddressfield enables deployment records to store both the proxy address and the underlying implementation address, which is essential for the transparent upgradeable proxy pattern.examples/CRISP/packages/crisp-contracts/hardhat.config.ts (1)
146-147: Proxy contracts consistently added across all Hardhat configurations.The addition matches the same change in templates/default/hardhat.config.ts and packages/enclave-contracts/hardhat.config.ts, ensuring consistent proxy support across all contract packages.
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
980-980: Expected artifact regeneration from recompilation.The buildInfoId update reflects contract recompilation as part of the upgradeable proxy implementation. The interface ABI remains unchanged.
packages/enclave-contracts/hardhat.config.ts (1)
171-175: Build configuration updated to include required dependencies.The addition of the
npmFilesToBuildarray ensures that:
- PoseidonT3.sol (cryptographic hash function) is compiled from the npm dependency
- OpenZeppelin's ProxyAdmin and TransparentUpgradeableProxy contracts are available for the upgradeable proxy pattern
This configuration is consistent with the template and example configs, ensuring all necessary contracts are built.
packages/enclave-contracts/ignition/modules/bondingRegistry.ts (1)
21-41: LGTM! Proxy deployment pattern correctly implemented.The migration to TransparentUpgradeableProxy follows the standard pattern:
- Implementation deployed with no-arg constructor (disabling direct initialization)
- Initialization parameters encoded for delegated call via proxy
- Proxy deployed with implementation address, owner, and encoded init data
This enables future upgrades while preserving the proxy address.
packages/enclave-contracts/scripts/proxy.ts (2)
22-35: LGTM! Storage slot extraction correctly implemented.The address extraction logic properly handles ERC-1967 storage layout:
- Reads the 32-byte admin slot
- Extracts the rightmost 20 bytes (40 hex chars)
- Checksums via
getAddress()for consistent formatting
43-54: LGTM! Ownership verification handles case sensitivity properly.Case-insensitive comparison ensures addresses match regardless of checksum format.
examples/CRISP/enclave.config.yaml (1)
6-16: LGTM! Configuration addresses updated for proxy deployment.The address updates align with the proxy-based deployment pattern introduced in this PR. Ensure these addresses correspond to the deployed proxy contracts (not implementation contracts) for proper runtime interaction.
templates/default/enclave.config.yaml (1)
6-16: LGTM! Template configuration synchronized with examples.Addresses match those in examples/CRISP/enclave.config.yaml, maintaining consistency across templates and examples.
tests/integration/enclave.config.yaml (1)
9-16: LGTM! Integration test configuration updated.The enclave, ciphernode_registry, and bonding_registry addresses align with examples and templates. Note that e3_program uses a different address, which may be intentional for the integration test environment.
packages/enclave-contracts/scripts/upgrade/enclave.ts (2)
12-54: LGTM! Upgrade script follows proper validation flow.The script correctly:
- Deploys dependencies (PoseidonT3)
- Validates existing proxy deployment via
readDeploymentArgs- Verifies on-chain contract existence with
getCode- Executes upgrade through
upgradeAndSaveEnclave- Logs structured upgrade summary
The validation checks prevent upgrading non-existent deployments and provide clear error messages.
56-59: LGTM! Error handling properly exits with non-zero code.examples/CRISP/Readme.md (2)
102-102: LGTM! Documentation updated with new proxy address.
262-263: LGTM! Environment variable examples updated for proxy deployment.The addresses now reflect the proxy contract addresses, ensuring documentation aligns with the upgraded deployment pattern.
Based on learnings
packages/enclave-contracts/package.json (1)
149-151: LGTM! Upgrade scripts provide convenient upgrade interface.The three new scripts enable straightforward upgrades for each proxy-deployed contract, following a consistent naming convention.
packages/enclave-contracts/test/Enclave.spec.ts (1)
242-245: LGTM!The test correctly adapts to the proxy-based deployment pattern by connecting to the BondingRegistry through BondingRegistryFactory, ensuring tests interact with the proxy address rather than the implementation directly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (1)
12-15: Fix copy-paste error in JSDoc comment.The comment incorrectly references "Enclave contract" when it should reference "CiphernodeRegistryOwnable contract".
Apply this diff:
/** - * Upgrades the Enclave contract implementation and saves the deployment arguments + * Upgrades the CiphernodeRegistryOwnable contract implementation and saves the deployment arguments * This keeps the same proxy address, only updates the implementation */packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (1)
11-14: Fix copy-paste error in JSDoc comment.The comment incorrectly references "Enclave contract" when it should reference "BondingRegistry contract".
Apply this diff:
/** - * Upgrades the Enclave contract implementation and saves the deployment arguments + * Upgrades the BondingRegistry contract implementation and saves the deployment arguments * This keeps the same proxy address, only updates the implementation */
🧹 Nitpick comments (1)
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (1)
16-69: Consider extracting common upgrade logic.The upgrade script follows the same pattern as
enclave.tsandbondingRegistry.ts. While not critical, extracting the common validation and logging logic into a shared utility function would reduce duplication and improve maintainability.Example shared utility signature:
async function upgradeProxyContract<T>({ contractName: string, deployDependencies: () => Promise<Record<string, string>>, upgradeFunction: (deps: any) => Promise<{ contract: T; implementationAddress: string }>, hre: HardhatRuntimeEnvironment }): Promise<void>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/CRISP/circuits/src/ciphertext_addition.nr(1 hunks)examples/CRISP/packages/crisp-contracts/deploy/crisp.ts(2 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 due to trivial changes (1)
- examples/CRISP/circuits/src/ciphertext_addition.nr
🧰 Additional context used
🧠 Learnings (3)
📚 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/bondingRegistry.tspackages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/packages/crisp-contracts/deploy/crisp.ts
📚 Learning: 2024-11-25T09:47:48.863Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/tests/entrypoint.sh:4-8
Timestamp: 2024-11-25T09:47:48.863Z
Learning: When reviewing test scripts like `packages/ciphernode/net/tests/entrypoint.sh`, avoid suggesting additional error handling and cleanup for `iptables` commands, as it may not be necessary.
Applied to files:
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts
🧬 Code graph analysis (4)
packages/enclave-contracts/scripts/upgrade/enclave.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
upgradeAndSaveEnclave(137-207)
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (2)
packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
upgradeAndSaveBondingRegistry(156-229)
examples/CRISP/packages/crisp-contracts/deploy/crisp.ts (1)
packages/enclave-contracts/scripts/utils.ts (2)
readDeploymentArgs(75-89)storeDeploymentArgs(35-67)
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
upgradeAndSaveCiphernodeRegistryOwnable(123-205)
⏰ 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). (10)
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: build_e3_support_dev
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (8)
packages/enclave-contracts/scripts/upgrade/enclave.ts (3)
1-10: LGTM!License header and imports are appropriate for this upgrade script.
12-58: LGTM!The upgrade logic correctly validates proxy existence, deploys dependencies, and performs the upgrade with appropriate error handling and logging.
60-63: LGTM!Immediate execution with proper error handling is standard for Hardhat upgrade scripts.
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (1)
15-63: LGTM!The upgrade logic correctly validates proxy existence and performs the upgrade. Note that this script appropriately omits PoseidonT3 deployment since BondingRegistry doesn't require it, unlike Enclave and CiphernodeRegistryOwnable.
examples/CRISP/packages/crisp-contracts/deploy/crisp.ts (4)
19-39: LGTM!The deployment setup logic is well-structured:
- Proper initialization of ethers and signers
- Environment-based configuration for mock contracts
- Appropriate error handling for missing Enclave deployment
119-182: LGTM!The verifier deployment logic is well-implemented:
- Checks for existing deployments to avoid redundant work
- Consistent async/await patterns
- Proper storage of deployment arguments for both mock and real verifiers
189-254: LGTM!The input validator deployment logic is well-implemented:
- Checks for existing deployments to avoid redundant work
- Consistent async/await patterns
- Proper storage of deployment arguments for both mock and real validators
- Clear distinction between mock and production contract names
16-17: IMAGE_ID hardcoding is correct and should not be changed.The IMAGE_ID is the RISC Zero circuit's program identifier (
PROGRAM_IDin the auto-generated ImageID.sol files). It is immutable per circuit build and must match between Solidity and TypeScript layers. The deployment script correctly hardcodes this value—it is not a deployment configuration parameter likeUSE_MOCK_VERIFIERandUSE_MOCK_INPUT_VALIDATORwhich do use environment variables.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/enclave-contracts/contracts/Enclave.sol (1)
88-88: Consider adding storage gap for future upgradeability.For upgradeable contracts, OpenZeppelin recommends reserving storage slots with a
__gapvariable to allow adding new storage variables in future upgrades without breaking the storage layout. Consider adding after line 88:/// @notice Mapping tracking fee payments for each E3. /// @dev Stores the amount paid for an E3, distributed to committee upon completion. mapping(uint256 e3Id => uint256 e3Payment) public e3Payments; + + /// @notice Storage gap for future upgrades. + /// @dev Reserves storage slots to allow adding new state variables in future versions. + uint256[50] private __gap;packages/enclave-contracts/scripts/upgrade/enclave.ts (2)
24-46: Consider consolidating duplicate existence checks.The proxy existence is checked twice:
- Lines 24-27: Check if proxy address exists in deployment records
- Lines 40-46: Check if proxy contract exists on-chain
The on-chain check (lines 40-46) is more definitive. Consider removing the first check or clarifying why both are needed in comments.
23-23: PoseidonT3 redeployment check is redundant on upgrades—consider skipping or refactoring.The
deployAndSavePoseidonT3function uses a deterministic address from the external "poseidon-solidity" package. Since PoseidonT3 is deployed once during initial setup and always exists at the same address, calling it unconditionally on every upgrade (line 23) is unnecessary overhead. While the function safely checks if already deployed and returns early, the on-chain code check and re-storing of deployment args happen on every upgrade.Suggestion: Either skip the call for upgrade-only operations, or refactor to accept an optional flag to skip PoseidonT3 lookup when upgrading existing contracts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/enclave-contracts/contracts/Enclave.sol(1 hunks)packages/enclave-contracts/contracts/registry/BondingRegistry.sol(3 hunks)packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol(1 hunks)packages/enclave-contracts/deployed_contracts.json(1 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)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/bondingRegistry.tspackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/scripts/upgrade/enclave.tspackages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.tspackages/enclave-contracts/deployed_contracts.json
📚 Learning: 2024-11-25T09:47:48.863Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/tests/entrypoint.sh:4-8
Timestamp: 2024-11-25T09:47:48.863Z
Learning: When reviewing test scripts like `packages/ciphernode/net/tests/entrypoint.sh`, avoid suggesting additional error handling and cleanup for `iptables` commands, as it may not be necessary.
Applied to files:
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts
🧬 Code graph analysis (3)
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (2)
packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
upgradeAndSaveBondingRegistry(156-229)
packages/enclave-contracts/scripts/upgrade/enclave.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
upgradeAndSaveEnclave(137-207)
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (3)
packages/enclave-contracts/scripts/deployAndSave/poseidonT3.ts (1)
deployAndSavePoseidonT3(19-63)packages/enclave-contracts/scripts/utils.ts (1)
readDeploymentArgs(75-89)packages/enclave-contracts/scripts/deployAndSave/ciphernodeRegistryOwnable.ts (1)
upgradeAndSaveCiphernodeRegistryOwnable(123-205)
🪛 Gitleaks (8.28.0)
packages/enclave-contracts/deployed_contracts.json
[high] 138-138: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 156-156: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 184-184: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (9)
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: build_e3_support_risc0
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (16)
packages/enclave-contracts/contracts/Enclave.sol (1)
199-204: LGTM! Previous comment has been addressed.The constructor correctly implements the upgradeable proxy pattern by calling
_disableInitializers(), and the comment now accurately describes that initialization occurs viainitialize()when deployed behind a proxy.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
178-183: LGTM! Comment correction applied.The constructor correctly implements the upgradeable proxy pattern by disabling initializers on the implementation contract, and the comment now accurately describes this behavior.
packages/enclave-contracts/scripts/upgrade/ciphernodeRegistryOwnable.ts (4)
12-15: LGTM! Comment correction applied.The comment now correctly describes upgrading the CiphernodeRegistryOwnable contract instead of the Enclave contract.
16-22: LGTM!The setup logic correctly retrieves the signer and network information with appropriate fallbacks.
23-58: LGTM!The upgrade logic includes proper validation:
- Verifies proxy deployment exists
- Confirms deployment is proxy-based (lines 34-38)
- Validates on-chain contract presence (lines 45-51)
- Security enforced: the signer must be the ProxyAdmin owner (verified in
upgradeAndSaveCiphernodeRegistryOwnableviaverifyProxyAdminOwner)
60-75: LGTM!The logging and error handling are appropriate for an upgrade script, with clear output and proper exit codes.
packages/enclave-contracts/contracts/registry/BondingRegistry.sol (2)
142-147: LGTM! Past review feedback addressed.The constructor comment has been corrected and now accurately describes the upgradeable proxy pattern. The implementation properly disables initializers on the implementation contract.
85-85: Upgradeable pattern correctly implemented.The migration from in-file initialization to proxy-based initialization is correctly implemented. The
licenseActiveBpsvariable is now initialized via the setter in theinitialize()function, maintaining the same default value (8,000 basis points = 80%).Also applies to: 180-180
packages/enclave-contracts/scripts/upgrade/bondingRegistry.ts (2)
11-14: LGTM! Past review feedback addressed.The comment now correctly references the BondingRegistry contract instead of Enclave.
19-22: Verify chain identifier consistency.Line 19 derives the chain name from
signer.provider.getNetwork().name, which is used to read deployment args on line 22. However,upgradeAndSaveBondingRegistry(called on lines 48-52) useshre.globalOptions.networkto determine the chain when saving deployment args.If these two methods return different chain identifiers, deployment args could be read from one chain key but saved to another, causing inconsistencies in the deployments file.
Consider using a consistent chain identifier source:
- const chain = (await signer.provider?.getNetwork())?.name ?? "localhost"; + const chain = hre.globalOptions.network;Or verify that both methods always return the same value for your deployment environments.
Also applies to: 48-52
packages/enclave-contracts/deployed_contracts.json (2)
138-138: Static analysis false positives - safe to ignore.The static analysis tool is flagging Ethereum contract addresses as "Generic API Key". These are legitimate blockchain addresses (0x-prefixed hex strings), not API keys or secrets. The warnings can be safely ignored.
Also applies to: 156-157, 184-184
167-167: Implementation addresses correctly added for proxy deployments.The
implementationAddressfields have been appropriately added for BondingRegistry, CiphernodeRegistryOwnable, and Enclave, indicating these contracts are now deployed using the upgradeable proxy pattern. Cross-references between contracts appear consistent.Also applies to: 177-177, 192-192
packages/enclave-contracts/scripts/upgrade/enclave.ts (4)
1-11: LGTM!The license header and imports are appropriate for an upgrade script.
12-22: LGTM!The function documentation and setup are clear and follow standard patterns.
29-33: Excellent safety check!This validation prevents accidentally attempting to upgrade a non-proxy deployment, which would fail. This is an important guard rail.
54-69: LGTM!The logging and error handling are appropriate for an upgrade script. The output clearly shows both the proxy address (which remains unchanged) and the new implementation address.
ctrlc03
left a comment
ctrlc03
left a comment
There was a problem hiding this comment.
Neat work! should we add some tests for upgrades?
4 participants
The reason for skipping the line limit is that many of the changes are updated contract addresses in the JSON files/Comments/File Header fixes.
Summary by CodeRabbit
Refactor
Chores
Tests
Style