add: block number to e3 struct #150
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 and nitpick comments (5)
packages/evm/contracts/interfaces/IE3.sol (1)
25-25: Consider potential gas cost impacts.While the addition of the
requestBlockfield is straightforward and beneficial for querying, it's worth considering any potential impacts on gas costs, especially if E3 structs are frequently stored or retrieved from storage.Consider running gas benchmarks before and after this change to quantify any differences in gas consumption for operations involving E3 structs.
packages/evm/contracts/registry/NaiveRegistryFilter.sol (1)
51-58: Consider documenting the intended usage of this modifier.While the
onlyOwnerOrCiphernodemodifier is well-implemented, it's not currently applied to any functions in this contract. Consider adding a comment explaining its intended future usage or apply it to relevant functions if applicable.packages/evm/contracts/Enclave.sol (1)
163-163: LGTM:requestBlockinitialization inrequestfunctionThe addition of
requestBlock: block.number,in the E3 struct initialization within therequestfunction is correct and aligns with the intended functionality. This change effectively captures the block number at which the E3 request is made.Consider the following optimization:
To potentially save a small amount of gas, you could consider declaring a local variable for
block.numberat the beginning of the function and using it for both therequestBlockinitialization and theseedcalculation. This would ensure that the same block number is used consistently throughout the function and might save a small amount of gas by reducing the number ofBLOCKNUMBERopcodes. For example:uint256 currentBlock = block.number; uint256 seed = uint256(keccak256(abi.encode(block.prevrandao, e3Id))); // ... other code ... E3 memory e3 = E3({ // ... other fields ... requestBlock: currentBlock, // ... other fields ... });This change is optional and the current implementation is also correct.
packages/evm/test/Enclave.spec.ts (2)
590-592: Improve error handling for block retrievalThe error handling for
getBlockis good practice, but it's unclear what happens if an error occurs. Consider logging the error or handling it more explicitly.- const block = await ethers.provider.getBlock("latest").catch((e) => e); + const block = await ethers.provider.getBlock("latest").catch((e) => { + console.error("Error retrieving latest block:", e); + throw e; // or handle the error in a way that makes sense for your tests + });This change will provide more visibility into any potential issues with block retrieval.
590-595: Consider adding more test cases for requestBlockWhile the current changes adequately test the basic functionality of the new
requestBlockfield, consider adding more specific test cases to ensure comprehensive coverage. For example:
- Test that
requestBlockis set correctly for multiple E3 requests across different blocks.- Verify that
requestBlockremains unchanged after other operations like activation or output publishing.- Test edge cases, such as when the block number is at its maximum value.
These additional tests would help ensure the robustness of the new
requestBlockfunctionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/evm/contracts/Enclave.sol (1 hunks)
- packages/evm/contracts/interfaces/IE3.sol (2 hunks)
- packages/evm/contracts/registry/NaiveRegistryFilter.sol (2 hunks)
- packages/evm/test/Enclave.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/evm/contracts/interfaces/IE3.sol (2)
11-11: LGTM: Documentation updated correctly.The new documentation line for the
requestBlockfield is clear, concise, and follows the existing documentation style.
25-25: LGTM: New field added correctly. Verify usage across codebase.The
requestBlockfield has been added correctly to the E3 struct. This addition aligns with the PR objectives and should facilitate simpler queries for fetching inputs associated with a specific E3.To ensure consistency, please verify that all parts of the codebase that create or use E3 structs have been updated to include this new field. Run the following script to check for potential places that might need updates:
✅ Verification successful
LGTM:
requestBlockfield added correctly. Verified usage across codebase.The
requestBlockfield has been successfully added to theE3struct. All existing usages in the codebase have been verified to ensure consistency and correctness.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential places where E3 structs are used and might need updates. # Search for E3 struct usage echo "Searching for E3 struct usage:" rg --type solidity -i '\bE3\b' --glob '!**/interfaces/IE3.sol' # Search for functions that might create or return E3 structs echo -e "\nSearching for functions that might create or return E3 structs:" rg --type solidity -i 'function.*\bE3\b'Length of output: 387
Script:
#!/bin/bash # Description: Find potential places where E3 structs are used and might need updates. # Search for E3 struct usage in Solidity files echo "Searching for E3 struct usage in .sol files:" rg '\bE3\b' --type-add 'sol=*.sol' --type sol --glob '!**/interfaces/IE3.sol' # Search for functions that might create or return E3 structs echo -e "\nSearching for functions that might create or return E3 structs in .sol files:" rg 'function\s+\w+\s*\(.*\)\s*returns\s*\(.*E3.*\)' --type-add 'sol=*.sol' --type solLength of output: 557
Script:
#!/bin/bash # Description: Find potential places where E3 structs are used and might need updates. # Add custom type for Solidity files rg --type-add 'sol:*.sol' # Search for E3 struct usage in Solidity files echo "Searching for E3 struct usage in .sol files:" rg '\bE3\b' --type sol --glob '!**/interfaces/IE3.sol' # Search for functions that might create or return E3 structs in Solidity files echo -e "\nSearching for functions that might create or return E3 structs in .sol files:" rg 'function\s+\w+\s*\(.*\)\s*returns\s*\(.*E3.*\)' --type solLength of output: 872
Script:
#!/bin/bash # Description: Find potential places where E3 structs are used and might need updates. # Search for E3 struct usage in Solidity files, excluding interfaces/IE3.sol echo "Searching for E3 struct usage in .sol files excluding interfaces/IE3.sol:" rg '\bE3\b' -g '*.sol' -g '!interfaces/IE3.sol' # Search for functions that create or return E3 structs in Solidity files, excluding interfaces/IE3.sol echo -e "\nSearching for functions that might create or return E3 structs in .sol files excluding interfaces/IE3.sol:" rg 'function\s+\w+\s*\(.*\)\s*returns\s*\(.*E3.*\)' -g '*.sol' -g '!interfaces/IE3.sol'Length of output: 8325
packages/evm/contracts/registry/NaiveRegistryFilter.sol (3)
37-37: LGTM: New error enhances error handling for ciphernode eligibility.The
CiphernodeNotEnablederror is well-named and includes the relevantciphernodeaddress, which will be useful for debugging and logging. This addition aligns with the PR objective of improving access control for ciphernodes.
51-58: LGTM: New modifier enhances access control.The
onlyOwnerOrCiphernodemodifier correctly implements the intended access control, allowing either the contract owner or an eligible ciphernode to proceed. It appropriately uses theICiphernodeRegistryinterface for eligibility checks and throws the newly addedCiphernodeNotEnablederror when conditions are not met.
Line range hint
37-58: Summary: Enhancements to access control for ciphernodes.The changes in this file successfully implement improved access control for ciphernodes:
- A new
CiphernodeNotEnablederror provides clear error reporting.- The
onlyOwnerOrCiphernodemodifier establishes a mechanism to restrict access to either the contract owner or eligible ciphernodes.These additions align well with the PR objectives and are implemented consistently with the existing code style. Consider documenting the intended usage of the new modifier to provide clarity for future development.
packages/evm/contracts/Enclave.sol (3)
163-163: LGTM: Addition ofrequestBlockenhances E3 trackingThe addition of the
requestBlockfield to theE3struct is a good improvement. It allows for easier tracking of when each E3 request was made, which aligns with the PR objective of facilitating simpler queries for fetching inputs associated with a specific E3.Using
block.numberis a standard and reliable way to record the block at which the request occurred. This information can be valuable for various purposes, such as auditing, debugging, or implementing time-based logic in relation to E3 requests.
163-163: Summary: Addition ofrequestBlockenhances E3 tracking with minimal impactThe changes in this PR successfully add the
requestBlockfield to theE3struct, achieving the objective of facilitating simpler queries for fetching inputs associated with a specific E3. The implementation is straightforward and doesn't introduce any apparent issues.Key points:
- The change is localized to the
E3struct andrequestfunction.- It provides valuable information for tracking when each E3 request was made.
- The impact on gas costs is likely minimal but should be verified.
- External integrations may need to be updated to handle the new field.
Overall, this change improves the functionality of the Enclave contract while maintaining its overall structure and logic. Ensure that appropriate testing is done, especially regarding gas costs and integration with existing systems.
163-163: Consider the impact on gas costs and external integrationsWhile the addition of the
requestBlockfield is beneficial, it's important to consider its implications:
Gas Costs: The additional field in the
E3struct will slightly increase the gas cost of therequestfunction. While this increase is likely minimal, it's worth noting, especially if this function is called frequently.External Integrations: Ensure that any off-chain services or other contracts that interact with the
E3struct are updated to handle the new field. This might include updating any parsing logic, database schemas, or interfaces that work withE3data.Backwards Compatibility: If there are any existing
E3instances, consider how they will be handled. Will they have a default value forrequestBlock, or will there be a migration process?To assess the impact on gas costs, you could run gas profiling tests. Here's a script to help with that:
This will help quantify the gas cost impact of the change.
packages/evm/test/Enclave.spec.ts (1)
595-595: LGTM: New assertion aligns with PR objectivesThe new assertion
expect(e3.requestBlock).to.equal(block.number);correctly verifies that therequestBlockfield is set to the current block number when the E3 is created. This change aligns well with the PR objective of adding arequestBlockfield to the E3 struct.
This PR adds adds
requestBlockto the E3 struct, allowing for simpler queries to fetch inputs for a given E3.Summary by CodeRabbit
New Features
requestBlockfield to theE3structure, capturing the block number at the time of the request.onlyOwnerOrCiphernode, to improve access control for functions in theNaiveRegistryFiltercontract.Bug Fixes
CiphernodeNotEnabled, to handle eligibility issues for ciphernodes.Tests
request()method to verify the correct assignment of the newrequestBlockproperty.