Better deploy script #52
Conversation
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces several changes to the Ethereum Virtual Machine (EVM) package, including the addition of an Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant Enclave
participant CyphernodeRegistryOwnable
participant NaiveRegistryFilter
Deployer->>Enclave: Deploy Enclave with args
Deployer->>CyphernodeRegistryOwnable: Deploy CyphernodeRegistryOwnable
Deployer->>NaiveRegistryFilter: Deploy NaiveRegistryFilter
Enclave-->>Deployer: Return Enclave contract instance
Deployer->>Enclave: Get current registry address
Enclave-->>Deployer: Return registry address
Deployer->>Enclave: Set new registry if addresses differ
Enclave-->>Deployer: Confirm registry update
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/evm/deploy/deploy.ts (1)
5-5: Clarify the purpose and usage of theaddressOneconstant.Please provide a brief explanation of what this address represents and how it is used in the deployment process. Consider adding a comment to improve code readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/evm/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (6)
- packages/evm/.env.example (1 hunks)
- packages/evm/contracts/registry/NaiveRegistryFilter.sol (1 hunks)
- packages/evm/contracts/test/MockRegistryFilter.sol (1 hunks)
- packages/evm/deploy/deploy.ts (1 hunks)
- packages/evm/hardhat.config.ts (3 hunks)
- packages/evm/package.json (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/evm/.env.example
- packages/evm/contracts/test/MockRegistryFilter.sol
Additional comments not posted (11)
packages/evm/deploy/deploy.ts (4)
11-12: LGTM!The code changes are approved.
15-15: LGTM, but clarify the purpose ofaddressOne.The code changes are approved.
However, as mentioned in a previous comment, please provide clarification on the purpose and usage of the
addressOneconstant.
21-42: LGTM!The code changes are approved.
44-61: LGTM!The code changes are approved.
packages/evm/package.json (1)
28-28: LGTM!The code changes are approved.
packages/evm/hardhat.config.ts (4)
2-2: LGTM!The code changes are approved.
11-21: LGTM!The code changes are approved.
69-69: LGTM!The code changes are approved.
73-73: LGTM!The code changes are approved.
packages/evm/contracts/registry/NaiveRegistryFilter.sol (2)
56-57: LGTM!The constructor changes are approved.
The parameter name change from
_enclaveto_registryaligns with the contract's purpose of interacting with a registry contract. The constructor is correctly initializing the contract by calling theinitializefunction with the provided_ownerand_registryaddresses.
Line range hint
60-65: LGTM!The
initializefunction changes are approved.The parameter name change from
_enclaveto_registryaligns with the contract's purpose of interacting with a registry contract. The function is correctly initializing the contract by calling the__Ownable_initfunction, setting the registry address, and transferring ownership to the provided_owneraddress.
samepant
force-pushed
the
deploy-cleanup
branch
from
3b190e1 to
6df113f
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (22)
packages/evm/README.md (1)
5-7: Specify a language for the fenced code block.To enhance syntax highlighting and readability, it's recommended to specify a language for fenced code blocks.
Add
bashas the language specifier for the fenced code block:-``` +```bash yarn deploy --network [network]<details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 5-5: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> <details> <summary>packages/evm/deploy/deploy.ts (2)</summary><blockquote> `5-5`: **Add a comment and consider using an environment variable.** Please add a comment explaining the purpose and usage of the `addressOne` constant. Also, consider moving the hardcoded address to an environment variable or configuration file to improve flexibility and maintainability. --- `21-42`: **Clarify the purpose of the new contracts and add error handling.** 1. Please provide a brief explanation of the purpose and role of the newly added contracts (CyphernodeRegistryOwnable and NaiveRegistryFilter) in the system. This will help in understanding the overall architecture and the interaction between the contracts. 2. Consider adding error handling for the contract deployments to manage cases where the deployments fail. This will make the script more resilient and informative. </blockquote></details> <details> <summary>packages/evm/deployments/sepolia/solcInputs/d0e6e4f19028714f394c36db62dff2be.json (19)</summary><blockquote> `13-14`: **Use a more specific SPDX license identifier.** The current SPDX license identifier "LGPL-3.0-only" is valid but not as specific as it could be. Consider using "LGPL-3.0-or-later" to allow for future versions of the LGPL license, providing more flexibility. --- `22-40`: **Consider adding comments to explain the purpose of each storage variable.** While the variable names are somewhat self-explanatory, adding brief comments to describe the purpose and usage of each storage variable can improve code readability and maintainability. --- `48-71`: **Use more descriptive error messages.** Some of the error messages, such as "E3NotReady()" or "InvalidComputation()", could be more descriptive to help with debugging and understanding the specific issue. Consider adding more context to the error messages. --- Line range hint `97-106`: **Avoid duplicate initialization logic.** The `initialize` function contains initialization logic that is already present in the constructor. To avoid duplication and potential inconsistencies, consider removing the `initialize` function and performing all initialization in the constructor. --- Line range hint `114-164`: **Use `require` consistently for input validation.** The `request` function uses a mix of `require` statements and if-else blocks for input validation. For consistency and readability, consider using `require` statements for all input validation checks. --- Line range hint `166-184`: **Handle potential reentrancy vulnerabilities.** The `activate` function interacts with the `cyphernodeRegistry` contract, which could potentially lead to reentrancy vulnerabilities if not properly handled. Consider using the checks-effects-interactions pattern or a reentrancy guard to prevent unexpected behavior. --- Line range hint `186-205`: **Validate input data size.** The `publishInput` function does not validate the size of the input data. Depending on the expected input size and gas constraints, it may be necessary to limit the size of the input data to prevent excessive gas consumption or out-of-gas errors. --- Line range hint `207-231`: **Clarify the purpose of the `publishCiphertextOutput` function.** The purpose and usage of the `publishCiphertextOutput` function are not entirely clear from the code and comments. Consider adding more documentation or comments to explain when and how this function should be called, and how it relates to the overall E3 workflow. --- Line range hint `233-255`: **Validate plaintext output size.** Similar to the `publishInput` function, the `publishPlaintextOutput` function does not validate the size of the plaintext output data. Consider adding a size check to prevent excessive gas consumption or out-of-gas errors. --- Line range hint `263-269`: **Emit an event when setting the maximum duration.** When the `setMaxDuration` function is called, it should emit an event to notify listeners about the change in the maximum duration value. This can help with monitoring and auditing changes to the contract's configuration. --- Line range hint `271-286`: **Use a more specific error message for invalid cyphernode registry.** The `InvalidCyphernodeRegistry` error message could be more specific about what makes a cyphernode registry invalid. Consider adding more context to the error message, such as "CyphernodeRegistryZeroAddress" or "CyphernodeRegistryAlreadySet". --- Line range hint `288-320`: **Add events for enabling and disabling modules.** When the `enableComputationModule`, `enableExecutionModule`, `disableComputationModule`, and `disableExecutionModule` functions are called, they should emit corresponding events to notify listeners about the changes in the module configuration. This can help with monitoring and auditing changes to the contract's setup. --- Line range hint `328-335`: **Use a more gas-efficient method to check for the existence of an E3.** The `getE3` function currently checks for the existence of an E3 by comparing the `computationModule` to the zero address. This requires a storage read operation, which can be expensive gas-wise. Consider using a more gas-efficient method, such as storing a boolean flag indicating the existence of an E3 or using a mapping with a default value. --- `40-41`: **Remove commented-out code.** The `publishCommittee` function contains commented-out code that checks if the cyphernodes are enabled. If this functionality is no longer needed, remove the commented-out code to improve code readability and maintainability. If the functionality is still relevant, consider uncommenting the code and updating it as necessary. --- `43-44`: **Use a more descriptive name for the `proof` parameter.** The `publishCommittee` function takes a `proof` parameter, but it's unclear what this proof represents or how it's used. Consider renaming the parameter to something more descriptive, such as `selectionProof` or `committeeProof`, to clarify its purpose. --- `43-44`: **Use a more descriptive name for the `nodes` parameter.** The `publishCommittee` function takes a `nodes` parameter, but it's unclear what these nodes represent or how they're selected. Consider renaming the parameter to something more descriptive, such as `selectedNodes` or `committeeNodes`, to clarify its purpose. --- `16-18`: **Add documentation for the `validate` function.** Consider adding a comment or documentation to explain the purpose and usage of the `validate` function, including the expected format of the `params` parameter and the significance of the returned `IInputValidator` instance. This can help developers better understand how to implement and interact with the computation module. --- `16-18`: **Consider adding an `e3Id` parameter to the `validate` function.** The `validate` function currently only takes the computation parameters as input. However, in some cases, the validation logic may depend on the specific E3 instance. Consider adding an `e3Id` parameter to allow the computation module to perform E3-specific validation if necessary. --- `20-27`: **Add documentation for the `verify` function.** Consider adding a comment or documentation to explain the purpose and usage of the `verify` function, including the expected format of the `outputData` parameter and the significance of the returned `output` and `success` values. This can help developers better understand how to implement and interact with the computation module. </blockquote></details> </blockquote></details> <details> <summary>Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>Commits</summary> Files that changed from the base of the PR and between 5f50d8d66e2b35d584b8db15c4157134efbf5478 and 6df113f3e8dbc99ed80c297852710ec4b8ade4f0. </details> <details> <summary>Files ignored due to path filters (1)</summary> * `packages/evm/yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>Files selected for processing (14)</summary> * .gitignore (1 hunks) * packages/evm/.env.example (1 hunks) * packages/evm/.gitignore (0 hunks) * packages/evm/README.md (1 hunks) * packages/evm/contracts/registry/NaiveRegistryFilter.sol (1 hunks) * packages/evm/contracts/test/MockRegistryFilter.sol (1 hunks) * packages/evm/deploy/deploy.ts (1 hunks) * packages/evm/deployments/sepolia/.chainId (1 hunks) * packages/evm/deployments/sepolia/CyphernodeRegistryOwnable.json (1 hunks) * packages/evm/deployments/sepolia/NaiveRegistryFilter.json (1 hunks) * packages/evm/deployments/sepolia/solcInputs/d0e6e4f19028714f394c36db62dff2be.json (1 hunks) * packages/evm/hardhat.config.ts (3 hunks) * packages/evm/package.json (2 hunks) * packages/evm/tasks/enclave.ts (0 hunks) </details> <details> <summary>Files not reviewed due to no reviewable changes (2)</summary> * packages/evm/.gitignore * packages/evm/tasks/enclave.ts </details> <details> <summary>Files skipped from review due to trivial changes (3)</summary> * .gitignore * packages/evm/.env.example * packages/evm/deployments/sepolia/.chainId </details> <details> <summary>Files skipped from review as they are similar to previous changes (3)</summary> * packages/evm/contracts/registry/NaiveRegistryFilter.sol * packages/evm/contracts/test/MockRegistryFilter.sol * packages/evm/package.json </details> <details> <summary>Additional context used</summary> <details> <summary>Markdownlint</summary><blockquote> <details> <summary>packages/evm/README.md</summary><blockquote> 5-5: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> </details> <details> <summary>Additional comments not posted (18)</summary><blockquote> <details> <summary>packages/evm/README.md (1)</summary><blockquote> `1-12`: **Great job on the README file!** The content is clear, concise, and provides actionable steps for users to deploy the Enclave EVM. The emphasis on configuring network settings before deployment is crucial for a smooth user experience. The file structure and formatting make it easy to follow the instructions. <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 5-5: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> <details> <summary>packages/evm/deploy/deploy.ts (3)</summary><blockquote> `11-12`: **LGTM!** The comment accurately describes the purpose of the subsequent code block. --- `44-61`: **LGTM!** The logic for updating the registry address in the Enclave contract is implemented correctly. The code is using appropriate checks and logging to handle different scenarios, and the early exit optimization is a good addition. --- `15-15`: **Verify the constructor arguments.** Please ensure that: 1. The Enclave contract's constructor is updated to accept the new arguments (`deployer` and `addressOne`) in the same order. 2. The contract is using the new constructor arguments correctly. Run the following script to verify the constructor definition: <details> <summary>Verification successful</summary> --- <!-- [monologue] [task analyze_enclave_contract_1/1] Analyze the content of the Enclave.sol file and determine if the constructor arguments are correct. [observations] - The Enclave contract is defined and includes both a constructor and an initialize function. - The constructor takes three parameters: address _owner, ICiphernodeRegistry _ciphernodeRegistry, and uint256 _maxDuration. - The initialize function is called within the constructor and takes the same parameters. - The contract is using the OpenZeppelin upgradeable contracts pattern. [analysis] - The constructor arguments in the deploy script (deployer, addressOne, THIRTY_DAYS_IN_SECONDS) match the expected types (address, ICiphernodeRegistry, uint256) in the Enclave contract. - The order of the arguments in the deploy script matches the order in the contract's constructor. - The use of both a constructor and an initialize function is consistent with the upgradeable pattern, where the constructor sets up the initial state and the initialize function can be used for upgrades. [actions] Generate the final response confirming that the constructor arguments are correct and explaining the contract's initialization pattern. --> **Constructor arguments are correct and consistent with the contract.** The Enclave contract's constructor is defined with three parameters that match the arguments provided in the deploy script: 1. `address _owner` (corresponds to `deployer`) 2. `ICiphernodeRegistry _ciphernodeRegistry` (corresponds to `addressOne`) 3. `uint256 _maxDuration` (corresponds to `THIRTY_DAYS_IN_SECONDS`) The contract uses the OpenZeppelin upgradeable contracts pattern, which explains the presence of both a constructor and an initialize function. The constructor calls the initialize function with the provided arguments, ensuring proper initialization for both new deployments and potential future upgrades. No changes are required in the deploy script or the contract implementation regarding the constructor arguments. </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify the Enclave contract's constructor arguments. # Test: Search for the constructor definition. Expect: Constructor with 3 arguments in the order: address, address, uint256. ast-grep --lang solidity --pattern $'constructor($_, $_, $_) { $$$ }'Length of output: 189
Script:
#!/bin/bash # Description: Search for the Enclave contract and its constructor definition # Search for Enclave contract with a constructor that has three parameters rg -U "contract Enclave.*?\{.*?constructor\s*\([^)]+,[^)]+,[^)]+\)" --type-add 'solidity:*.sol' -tsolidity # If the above doesn't yield results, search for any Enclave contract definition rg "contract Enclave" --type-add 'solidity:*.sol' -tsolidityLength of output: 257
Script:
#!/bin/bash # Description: Examine the Enclave contract for constructor or initializer # Search for constructor in Enclave.sol echo "Searching for constructor:" rg "constructor\s*\([^)]*\)" packages/evm/contracts/Enclave.sol -A 10 # Search for initialize function in Enclave.sol echo "Searching for initialize function:" rg "function\s+initialize\s*\([^)]*\)" packages/evm/contracts/Enclave.sol -A 10Length of output: 295
Script:
#!/bin/bash # Description: Display the content of Enclave.sol echo "Content of Enclave.sol:" cat packages/evm/contracts/Enclave.sol echo -e "\nFile information:" ls -l packages/evm/contracts/Enclave.solLength of output: 15125
packages/evm/hardhat.config.ts (5)
2-2: LGTM!The import statement for the
dotenvpackage is correct. Usingdotenvis a good practice for managing environment variables in Node.js projects.
11-19: Great job on improving the environment variable handling!The code segment correctly loads environment variables from a
.envfile usingdotenv.config(). Destructuring the required variables (INFURA_KEY,MNEMONIC, andETHERSCAN_API_KEY) improves code readability.The runtime check for missing environment variables is an excellent addition. It helps catch configuration issues early and provides a clear error message indicating which variables are missing.
20-21: LGTM!The assignments of
MNEMONICandINFURA_KEYenvironment variables tomnemonicandinfuraApiKeyconstants are correct. The type annotations are also consistent with the expected types of the environment variables.
69-69: LGTM!The assignment of
ETHERSCAN_API_KEYenvironment variable to themainnetproperty of theapiKeyobject in theetherscanconfiguration is correct. Using the environment variable directly, instead ofvars.get(), is consistent with the changes made to load environment variables usingdotenv.
73-73: LGTM!The assignment of
ETHERSCAN_API_KEYenvironment variable to thesepoliaproperty of theapiKeyobject in theetherscanconfiguration is correct. Using the environment variable directly, instead ofvars.get(), is consistent with the changes made to load environment variables usingdotenv.
packages/evm/deployments/sepolia/NaiveRegistryFilter.json (8)
5-19: LGTM!The constructor correctly initializes the contract by calling the
initializefunction with the provided_ownerand_enclaveaddresses.
129-145: LGTM!The
initializefunction correctly sets up the contract's initial state by:
- Calling
__Ownable_initto set the initial owner.- Setting the
registryaddress.- Transferring ownership to the provided
_owneraddress.The
initializermodifier ensures the contract can only be initialized once, preventing unauthorized re-initialization.
202-225: LGTM!The
requestCommitteefunction correctly initiates the committee selection process by:
- Using the
onlyRegistrymodifier to restrict access to only the authorized registry contract.- Checking if a committee already exists for the given
e3Idand reverting if true to prevent duplicate requests.- Storing the provided
thresholdfor thee3Idto be used later in the committee selection process.
159-181: LGTM!The
publishCommitteefunction correctly publishes the selected committee's details by:
- Using the
onlyOwnermodifier to restrict access to only the contract owner.- Checking if the committee's public key has already been published for the given
e3Idand reverting if true to prevent duplicate publishing.- Storing the selected
nodesandpublicKeyfor thee3Id.- Calling the
publishCommitteefunction on theregistrycontract with thee3Id, encodednodes, andpublicKeyto propagate the committee details to the registry.
226-238: LGTM!The
setRegistryfunction allows the contract owner to update theregistrycontract address. TheonlyOwnermodifier provides access control, ensuring only the contract owner can modify the registry address.
146-158: Skipped ReviewThe
ownerfunction is a standard public view function inherited from theOwnableUpgradeablecontract. It allows anyone to view the current owner of the contract.
195-201: Skipped ReviewThe
renounceOwnershipfunction is a standard public function inherited from theOwnableUpgradeablecontract. It allows the current owner to renounce ownership, leaving the contract without an owner. This can only be called by the current owner.
239-250: Skipped ReviewThe
transferOwnershipfunction is a standard public function inherited from theOwnableUpgradeablecontract. It allows the current owner to transfer ownership of the contract to a new account (newOwner). This can only be called by the current owner.
packages/evm/deployments/sepolia/CyphernodeRegistryOwnable.json (1)
1-2: LGTM!The function logic is correct, and the implementation is accurate.
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\ninterface IOutputVerifier {\n /// @notice This function should be called by the Enclave contract to verify the output of a computation.\n /// @param e3Id ID of the E3.\n /// @param data ABI encoded output data to be verified.\n /// @return output Ciphertext output of the given computation.\n function verify(\n uint256 e3Id,\n bytes memory data\n ) external view returns (bytes memory output, bool success);\n}\n" | ||
| }, | ||
| "contracts/interfaces/IRegistryFilter.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\ninterface IRegistryFilter {\n function requestCommittee(\n uint256 e3Id,\n uint32[2] calldata threshold\n ) external returns (bool success);\n}\n" | ||
| }, | ||
| "contracts/registry/CyphernodeRegistryOwnable.sol": { |
There was a problem hiding this comment.
Consider using a more gas-efficient data structure for allowed modules.
The current implementation uses two separate mappings for allowed computation and execution modules. Depending on the expected number of modules and the frequency of lookups, using a more gas-efficient data structure like an array or a bitmap could be more efficient.
| "*": [ | ||
| "abi", | ||
| "evm.bytecode", | ||
| "evm.deployedBytecode", | ||
| "evm.methodIdentifiers", | ||
| "metadata", | ||
| "devdoc", | ||
| "userdoc", | ||
| "storageLayout", | ||
| "evm.gasEstimates" | ||
| ], | ||
| "": [ | ||
| "ast" | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate constructor parameters.
The constructor function should validate the input parameters, particularly the _maxDuration and _cyphernodeRegistry, to ensure they are within acceptable ranges and not zero addresses. This can help prevent initialization with invalid values.
| "contracts/test/MockComputationModule.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport {\n IComputationModule,\n IInputValidator\n} from \"../interfaces/IComputationModule.sol\";\n\ncontract MockComputationModule is IComputationModule {\n error invalidParams(bytes params);\n\n function validate(\n bytes memory params\n ) external pure returns (IInputValidator inputValidator) {\n require(params.length == 32, \"invalid params\");\n // solhint-disable no-inline-assembly\n assembly {\n inputValidator := mload(add(params, 32))\n }\n }\n\n function verify(\n uint256,\n bytes memory data\n ) external pure returns (bytes memory output, bool success) {\n output = data;\n if (output.length > 0) success = true;\n }\n}\n" |
There was a problem hiding this comment.
Validate the publicKey parameter.
The publishCommittee function does not validate the publicKey parameter before storing it. Consider adding a check to ensure that the publicKey is not empty and has a valid format to prevent storing invalid or malformed public keys.
| "contracts/test/MockCyphernodeRegistry.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport { ICyphernodeRegistry } from \"../interfaces/ICyphernodeRegistry.sol\";\n\ncontract MockCyphernodeRegistry is ICyphernodeRegistry {\n function requestCommittee(\n uint256,\n address filter,\n uint32[2] calldata\n ) external pure returns (bool success) {\n if (filter == address(2)) {\n success = false;\n } else {\n success = true;\n }\n }\n\n function publishCommittee(\n uint256,\n bytes calldata,\n bytes calldata\n ) external {} // solhint-disable-line no-empty-blocks\n\n function committeePublicKey(\n uint256 e3Id\n ) external pure returns (bytes memory) {\n if (e3Id == type(uint256).max) {\n return hex\"\";\n } else {\n return abi.encodePacked(keccak256(abi.encode(e3Id)));\n }\n }\n\n function isCyphernodeEligible(address) external pure returns (bool) {\n return false;\n }\n}\n\ncontract MockCyphernodeRegistryEmptyKey is ICyphernodeRegistry {\n function requestCommittee(\n uint256,\n address filter,\n uint32[2] calldata\n ) external pure returns (bool success) {\n if (filter == address(2)) {\n success = false;\n } else {\n success = true;\n }\n }\n\n function publishCommittee(\n uint256,\n bytes calldata,\n bytes calldata\n ) external {} // solhint-disable-line no-empty-blocks\n\n function committeePublicKey(uint256) external pure returns (bytes memory) {\n return hex\"\";\n }\n\n function isCyphernodeEligible(address) external pure returns (bool) {\n return false;\n }\n}\n" | ||
| }, | ||
| "contracts/test/MockExecutionModule.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport {\n IExecutionModule,\n IOutputVerifier\n} from \"../interfaces/IExecutionModule.sol\";\n\ncontract MockExecutionModule is IExecutionModule {\n error invalidParams();\n\n function validate(\n bytes memory params\n ) external pure returns (IOutputVerifier outputVerifier) {\n require(params.length == 32, invalidParams());\n // solhint-disable no-inline-assembly\n assembly {\n outputVerifier := mload(add(params, 32))\n }\n (outputVerifier) = abi.decode(params, (IOutputVerifier));\n }\n}\n" | ||
| }, |
There was a problem hiding this comment.
Emit an event when setting the enclave address.
When the setEnclave function is called, it should emit an event to notify listeners about the change in the enclave address. This can help with monitoring and auditing changes to the contract's configuration.
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport { IInputValidator } from \"../interfaces/IInputValidator.sol\";\n\ncontract MockInputValidator is IInputValidator {\n function validate(\n address,\n bytes memory params\n ) external pure returns (bytes memory input, bool success) {\n input = params;\n\n if (input.length == 3) {\n success = false;\n } else {\n success = true;\n }\n }\n}\n" | ||
| }, | ||
| "contracts/test/MockOutputVerifier.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport { IOutputVerifier } from \"../interfaces/IOutputVerifier.sol\";\n\ncontract MockOutputVerifier is IOutputVerifier {\n function verify(\n uint256,\n bytes memory data\n ) external pure returns (bytes memory output, bool success) {\n output = data;\n\n if (output.length > 0) success = true;\n }\n}\n" | ||
| }, | ||
| "contracts/test/MockRegistryFilter.sol": { |
There was a problem hiding this comment.
Validate the cyphernode address when adding or removing.
The addCyphernode and removeCyphernode functions do not validate the node parameter before updating the isEnabled mapping. Consider adding a check to ensure that the node address is not zero and is a valid Ethereum address to prevent adding or removing invalid cyphernodes.
| "contracts/test/MockComputationModule.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport {\n IComputationModule,\n IInputValidator\n} from \"../interfaces/IComputationModule.sol\";\n\ncontract MockComputationModule is IComputationModule {\n error invalidParams(bytes params);\n\n function validate(\n bytes memory params\n ) external pure returns (IInputValidator inputValidator) {\n require(params.length == 32, \"invalid params\");\n // solhint-disable no-inline-assembly\n assembly {\n inputValidator := mload(add(params, 32))\n }\n }\n\n function verify(\n uint256,\n bytes memory data\n ) external pure returns (bytes memory output, bool success) {\n output = data;\n if (output.length > 0) success = true;\n }\n}\n" | ||
| }, | ||
| "contracts/test/MockCyphernodeRegistry.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport { ICyphernodeRegistry } from \"../interfaces/ICyphernodeRegistry.sol\";\n\ncontract MockCyphernodeRegistry is ICyphernodeRegistry {\n function requestCommittee(\n uint256,\n address filter,\n uint32[2] calldata\n ) external pure returns (bool success) {\n if (filter == address(2)) {\n success = false;\n } else {\n success = true;\n }\n }\n\n function publishCommittee(\n uint256,\n bytes calldata,\n bytes calldata\n ) external {} // solhint-disable-line no-empty-blocks\n\n function committeePublicKey(\n uint256 e3Id\n ) external pure returns (bytes memory) {\n if (e3Id == type(uint256).max) {\n return hex\"\";\n } else {\n return abi.encodePacked(keccak256(abi.encode(e3Id)));\n }\n }\n\n function isCyphernodeEligible(address) external pure returns (bool) {\n return false;\n }\n}\n\ncontract MockCyphernodeRegistryEmptyKey is ICyphernodeRegistry {\n function requestCommittee(\n uint256,\n address filter,\n uint32[2] calldata\n ) external pure returns (bool success) {\n if (filter == address(2)) {\n success = false;\n } else {\n success = true;\n }\n }\n\n function publishCommittee(\n uint256,\n bytes calldata,\n bytes calldata\n ) external {} // solhint-disable-line no-empty-blocks\n\n function committeePublicKey(uint256) external pure returns (bytes memory) {\n return hex\"\";\n }\n\n function isCyphernodeEligible(address) external pure returns (bool) {\n return false;\n }\n}\n" |
There was a problem hiding this comment.
Use a more gas-efficient method to check if the public key is empty.
The publishCommittee function currently checks if the public key is empty by comparing the keccak256 hash of the stored public key to the hash of an empty string. This requires a storage read operation and two hash calculations, which can be expensive gas-wise. Consider using a more gas-efficient method, such as storing a boolean flag indicating whether the public key has been set or checking the length of the stored public key directly.
| "contracts/test/MockExecutionModule.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport {\n IExecutionModule,\n IOutputVerifier\n} from \"../interfaces/IExecutionModule.sol\";\n\ncontract MockExecutionModule is IExecutionModule {\n error invalidParams();\n\n function validate(\n bytes memory params\n ) external pure returns (IOutputVerifier outputVerifier) {\n require(params.length == 32, invalidParams());\n // solhint-disable no-inline-assembly\n assembly {\n outputVerifier := mload(add(params, 32))\n }\n (outputVerifier) = abi.decode(params, (IOutputVerifier));\n }\n}\n" |
There was a problem hiding this comment.
Validate the publicKey parameter.
The publishCommittee function does not validate the publicKey parameter before storing it. Consider adding a check to ensure that the publicKey is not empty and has a valid format to prevent storing invalid or malformed public keys.
| "contracts/test/MockInputValidator.sol": { | ||
| "content": "// SPDX-License-Identifier: LGPL-3.0-only\npragma solidity >=0.8.26;\n\nimport { IInputValidator } from \"../interfaces/IInputValidator.sol\";\n\ncontract MockInputValidator is IInputValidator {\n function validate(\n address,\n bytes memory params\n ) external pure returns (bytes memory input, bool success) {\n input = params;\n\n if (input.length == 3) {\n success = false;\n } else {\n success = true;\n }\n }\n}\n" |
There was a problem hiding this comment.
Emit an event when publishing the committee.
When the publishCommittee function is called, it should emit an event to notify listeners about the newly published committee. This can help with monitoring and auditing the committee selection process.
| { | ||
| "inputs": [ | ||
| { |
There was a problem hiding this comment.
Reminder: Address the TODO comment.
The TODO comment indicates that tests are missing for this function. Please ensure that the function is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
| "internalType": "address", | ||
| "name": "_enclave", | ||
| "type": "address" | ||
| } | ||
| ], | ||
| "stateMutability": "nonpayable", | ||
| "type": "constructor" | ||
| }, | ||
| { | ||
| "inputs": [], | ||
| "name": "CommitteeAlreadyPublished", | ||
| "type": "error" | ||
| }, |
There was a problem hiding this comment.
Consider Adjusting the Fee or Discount Structure.
The implementation of a flat $20 service fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Consider revising either the discount percentages or the service fee application to better align with customer incentives. Some options:
- Waive the service fee for discounted purchases
- Apply a percentage-based service fee that scales with the discount tier
- Increase the discount percentages to offset the impact of the service fee
This refreshed deploy script deploys Enclave, the registry and the filter, then sets the registry address on the Enclave contract.
Also moved env var handling to .env file, using
dotenvSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
dotenvpackage for improved environment variable management.