add tasks for activating E3, removing ciphernodes and generating sibling data #146
Conversation
WalkthroughThis changeset introduces new tasks for managing ciphernodes and enabling E3 programs within the registry. Specifically, it adds the Changes
Poem
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 (
|
samepant
force-pushed
the
sam/enclave-task-upgrade
branch
from
48d35b0 to
00cbbec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/evm/tasks/enclave.ts (2)
141-157: Approve implementation with suggestions for improvementThe new
enclave:enableE3task is well-structured and correctly implemented. However, consider the following improvements:
- Add specific error handling to provide more informative error messages.
- Include a check to verify if the E3 program is already enabled before attempting to enable it.
- Add a verification step after enabling the E3 program to confirm its status.
Here's a suggested implementation with these improvements:
task("enclave:enableE3", "Enable an E3 program") .addParam("e3Address", "address of the E3 program") .setAction(async function (taskArguments: TaskArguments, hre) { try { const enclave = await hre.deployments.get("Enclave"); const enclaveContract = await hre.ethers.getContractAt( "Enclave", enclave.address ); // Check if the E3 program is already enabled const isEnabled = await enclaveContract.isE3ProgramEnabled(taskArguments.e3Address); if (isEnabled) { console.log(`E3 program at ${taskArguments.e3Address} is already enabled.`); return; } const tx = await enclaveContract.enableE3Program(taskArguments.e3Address); console.log("Enabling E3 program... ", tx.hash); await tx.wait(); // Verify the E3 program is now enabled const isNowEnabled = await enclaveContract.isE3ProgramEnabled(taskArguments.e3Address); if (isNowEnabled) { console.log(`E3 program at ${taskArguments.e3Address} has been successfully enabled.`); } else { console.error(`Failed to enable E3 program at ${taskArguments.e3Address}. Please check the contract state.`); } } catch (error) { console.error("Error enabling E3 program:", error.message); } });This implementation adds error handling, checks if the program is already enabled, and verifies the enabled status after the transaction.
Line range hint
1-157: Consider refactoring common patternsWhile the changes are focused and well-implemented, there's an opportunity to improve the overall structure of the file:
- Consider extracting the common pattern of retrieving the Enclave contract into a shared helper function.
- Implement a consistent error handling strategy across all tasks.
- Use a common logging format for transaction hashes and confirmations.
These refactorings would improve maintainability and reduce code duplication.
Here's an example of how you could implement a helper function for retrieving the Enclave contract:
async function getEnclaveContract(hre) { const enclave = await hre.deployments.get("Enclave"); return await hre.ethers.getContractAt("Enclave", enclave.address); }You can then use this helper function in all tasks that interact with the Enclave contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/evm/tasks/ciphernode.ts (2 hunks)
- packages/evm/tasks/enclave.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: ci
packages/evm/tasks/ciphernode.ts
[failure] 54-54:
'hre' is defined but never used. Allowed unused args must match /_/u
🔇 Additional comments (1)
packages/evm/tasks/enclave.ts (1)
141-157: Approve integration of new taskThe new
enclave:enableE3task is well-integrated into the existing codebase. It follows the established patterns and structure of other tasks in the file, maintaining consistency and not interfering with existing functionality.
packages/evm/tasks/ciphernode.ts
Outdated
| "ciphernodeAddresses", | ||
| "comma separated addresses of ciphernodes in the order they were added to the registry", | ||
| ) | ||
| .setAction(async function (taskArguments: TaskArguments, hre) { |
There was a problem hiding this comment.
Remove unused 'hre' parameter in 'setAction' function
In the ciphernode:siblings task, the hre parameter is defined but never used in the setAction callback function at line 54. This leads to a linting error and can be safely removed to clean up the code.
Apply this diff to fix the issue:
- .setAction(async function (taskArguments: TaskArguments, hre) {
+ .setAction(async function (taskArguments: TaskArguments) {📝 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.
| .setAction(async function (taskArguments: TaskArguments, hre) { | |
| .setAction(async function (taskArguments: TaskArguments) { |
🧰 Tools
🪛 GitHub Check: ci
[failure] 54-54:
'hre' is defined but never used. Allowed unused args must match /_/u
| const index = tree.indexOf(BigInt(taskArguments.ciphernodeAddress)); | ||
| const { siblings } = tree.generateProof(index); | ||
|
|
There was a problem hiding this comment.
Add error handling for missing ciphernode address in the tree
The code currently assumes that the ciphernodeAddress exists in the tree. If the address is not found, tree.indexOf may return -1, which could lead to errors when calling tree.generateProof(index). It's advisable to check if the index is valid before proceeding.
Apply this diff to handle cases where the ciphernode address is not found:
const index = tree.indexOf(BigInt(taskArguments.ciphernodeAddress));
+ if (index === -1) {
+ console.error(`Ciphernode address ${taskArguments.ciphernodeAddress} not found in the tree.`);
+ return;
+ }
const { siblings } = tree.generateProof(index);📝 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.
| const index = tree.indexOf(BigInt(taskArguments.ciphernodeAddress)); | |
| const { siblings } = tree.generateProof(index); | |
| const index = tree.indexOf(BigInt(taskArguments.ciphernodeAddress)); | |
| if (index === -1) { | |
| console.error(`Ciphernode address ${taskArguments.ciphernodeAddress} not found in the tree.`); | |
| return; | |
| } | |
| const { siblings } = tree.generateProof(index); |
| const siblings = taskArguments.siblings | ||
| .split(",") | ||
| .map((s: string) => BigInt(s)); |
There was a problem hiding this comment.
Add validation for 'siblings' input
Similar to ciphernodeAddresses, the siblings input is parsed and converted to BigInt without validation. Malformed inputs could cause exceptions. It's recommended to validate or handle potential errors when parsing siblings.
Example of adding input validation:
const siblings = taskArguments.siblings
.split(",")
- .map((s: string) => BigInt(s));
+ .map((s: string) => {
+ try {
+ return BigInt(s);
+ } catch (e) {
+ throw new Error(`Invalid sibling value: ${s}`);
+ }
+ });📝 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.
| const siblings = taskArguments.siblings | |
| .split(",") | |
| .map((s: string) => BigInt(s)); | |
| const siblings = taskArguments.siblings | |
| .split(",") | |
| .map((s: string) => { | |
| try { | |
| return BigInt(s); | |
| } catch (e) { | |
| throw new Error(`Invalid sibling value: ${s}`); | |
| } | |
| }); |
| const addresses = taskArguments.ciphernodeAddresses.split(","); | ||
|
|
||
| for (const address of addresses) { | ||
| tree.insert(BigInt(address)); | ||
| } |
There was a problem hiding this comment.
Validate 'ciphernodeAddresses' input before processing
When processing user-provided inputs like ciphernodeAddresses, converting each address to BigInt without validation may result in runtime errors if the input is invalid. Consider adding input validation or error handling to ensure robustness.
Here's how you might add input validation:
const addresses = taskArguments.ciphernodeAddresses.split(",");
for (const address of addresses) {
+ let addressBigInt;
+ try {
+ addressBigInt = BigInt(address);
+ } catch (e) {
+ console.error(`Invalid address: ${address}`);
+ return;
+ }
tree.insert(addressBigInt);
}📝 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.
| const addresses = taskArguments.ciphernodeAddresses.split(","); | |
| for (const address of addresses) { | |
| tree.insert(BigInt(address)); | |
| } | |
| const addresses = taskArguments.ciphernodeAddresses.split(","); | |
| for (const address of addresses) { | |
| let addressBigInt; | |
| try { | |
| addressBigInt = BigInt(address); | |
| } catch (e) { | |
| console.error(`Invalid address: ${address}`); | |
| return; | |
| } | |
| tree.insert(addressBigInt); | |
| } |
Summary by CodeRabbit
New Features
ciphernode:removefor removing a ciphernode from the registry.ciphernode:siblingsfor retrieving siblings of a specified ciphernode.enclave:enableE3task to enable an E3 program by providing its address.Bug Fixes
Documentation