Remove address from encoding #119
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the handling of events related 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: 2
🧹 Outside diff range and nitpick comments (8)
packages/evm/test/fixtures/MockE3Program.fixture.ts (1)
6-8: LGTM: Contract deployment updated correctly.The
deploymethod now correctly includes theinputValidatorAddressparameter, aligning with the PR objectives.Consider using object destructuring for better readability:
- const deployment = await ( - await ethers.getContractFactory("MockE3Program") - ).deploy(inputValidatorAddress); + const MockE3Program = await ethers.getContractFactory("MockE3Program"); + const deployment = await MockE3Program.deploy(inputValidatorAddress);packages/ciphernode/enclave_node/src/bin/pack_e3_params.rs (2)
Line range hint
13-25: Update struct documentation to reflect removed fieldsThe removal of
no_crpandinput_validatorfields from theArgsstruct aligns with the PR objective. This simplification of the command-line interface is a positive change.Consider updating the struct's documentation comment to reflect these changes. The current comment mentions CRP generation, which is no longer applicable:
- #[command(author, version, about="Encodes BFV parameters whilst generating a random CRP", long_about = None)] + #[command(author, version, about="Encodes BFV parameters", long_about = None)]
32-33: LGTM: Simplified encoding processThe removal of
encode_e3_paramsand direct use ofencode_bfv_paramsaligns with the PR objective and simplifies the encoding process. This change maintains the core functionality while removing unnecessary complexity.For improved clarity, consider adding a brief comment explaining the purpose of the
encodedvariable:+ // Encode BFV parameters let encoded = encode_bfv_params(args.moduli, args.degree, args.plaintext_modulus);tests/basic_integration/test.sh (2)
Line range hint
95-106: LGTM: Efficient parallel launch of cipher nodes.The parallel launch of four cipher nodes with unique addresses is well-implemented and aligns with the PR objectives. This approach potentially improves the efficiency of the testing process.
Consider using a loop to launch the cipher nodes, which could make the code more maintainable:
for address in $CIPHERNODE_ADDRESS_1 $CIPHERNODE_ADDRESS_2 $CIPHERNODE_ADDRESS_3 $CIPHERNODE_ADDRESS_4; do heading "Launch ciphernode $address" yarn ciphernode:launch --address $address --rpc "$RPC_URL" --enclave-contract $ENCLAVE_CONTRACT --registry-contract $REGISTRY_CONTRACT & done
Line range hint
166-166: LGTM: Clarifying comment for plaintext comparison.The added comment "Assume plaintext is shorter" provides useful context for the subsequent comparison operation, improving code readability.
Consider expanding the comment to provide more context about why this assumption is made and what implications it has. For example:
# Assume decrypted plaintext might be longer due to padding, so we check if it starts with the expected plaintextpackages/evm/test/Enclave.spec.ts (2)
47-49: LGTM! Consider adding a comment for clarity.The changes to the
deployE3ProgramFixturefunction call and the simplification ofe3ProgramParamsalign well with the PR objective of removing the address from encoding. This should make the tests more focused on the actual behavior rather than the encoding details.Consider adding a brief comment explaining the significance of the
"0x12345678"value used fore3ProgramParams. This would help other developers understand the purpose and meaning of this test data.Also applies to: 84-84
544-547: LGTM! Consider adding an assertion for improved clarity.The changes to this test case appropriately adjust for the new
e3ProgramParamsformat and focus on the correct behavior when the E3 Program doesn't return a valid input validator address. This is an important edge case to cover.To improve the clarity of the test, consider adding an assertion before the main test expectation to verify that the input validator has indeed been set to the zero address. This would make the test setup more explicit:
expect(await mocks.e3Program.inputValidator()).to.equal(ethers.ZeroAddress);Also applies to: 555-555
packages/ciphernode/core/src/evm_enclave.rs (1)
Line range hint
15-29: Consider removinginputValidatorfrom theE3struct if it's no longer requiredSince the
inputValidatoris no longer being encoded withine3ProgramParamsas per the PR objectives, and given that the encoding functions have been removed, consider removing theinputValidatorfield from theE3struct if it is no longer used elsewhere in the codebase. This will help simplify the structure and prevent potential confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/ciphernode/core/src/evm_enclave.rs (2 hunks)
- packages/ciphernode/core/src/lib.rs (0 hunks)
- packages/ciphernode/enclave_node/src/bin/pack_e3_params.rs (2 hunks)
- packages/evm/contracts/test/MockE3Program.sol (1 hunks)
- packages/evm/deploy/mocks.ts (1 hunks)
- packages/evm/test/Enclave.spec.ts (7 hunks)
- packages/evm/test/fixtures/MockE3Program.fixture.ts (1 hunks)
- tests/basic_integration/test.sh (3 hunks)
💤 Files with no reviewable changes (1)
- packages/ciphernode/core/src/lib.rs
🔇 Additional comments (22)
packages/evm/test/fixtures/MockE3Program.fixture.ts (3)
5-5: LGTM: Function signature updated correctly.The addition of the
inputValidatorAddressparameter aligns with the PR objectives and allows for more flexible testing scenarios.
10-10: LGTM: Return statement remains correct.The return statement correctly creates and returns a contract instance using the deployed address. No changes were necessary here.
5-10: Summary: Changes successfully implement PR objectives.The modifications to
deployE3ProgramFixturealign well with the PR objectives:
- The function now accepts
inputValidatorAddressas a parameter.- The
MockE3Programcontract deployment uses this address directly.- This change removes the need for encoding the
inputValidatorwithin thee3Params.These changes should improve clarity and maintainability as intended.
To ensure these changes are consistently applied throughout the codebase, run the following script:
✅ Verification successful
Verification Successful: All calls to
deployE3ProgramFixtureinclude theinputValidatorAddressparameter.The modifications to
deployE3ProgramFixturehave been consistently applied throughout the codebase:
- All instances of
deployE3ProgramFixturenow include theinputValidatorAddressparameter as intended.These changes confirm that the PR objectives are fully met, enhancing the clarity and maintainability of the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to deployE3ProgramFixture include the inputValidatorAddress parameter # Test: Search for function calls to deployE3ProgramFixture # Expect: All calls should include a parameter for inputValidatorAddress rg --type typescript 'deployE3ProgramFixture\s*\(' -A 3Length of output: 92
Script:
#!/bin/bash # Description: Verify that all calls to deployE3ProgramFixture include the inputValidatorAddress parameter # Test: Search for function calls to deployE3ProgramFixture without specifying the file type # Expect: All calls should include a parameter for inputValidatorAddress rg 'deployE3ProgramFixture\s*\(' -A 3Length of output: 682
packages/ciphernode/enclave_node/src/bin/pack_e3_params.rs (2)
2-2: LGTM: Import change aligns with PR objectiveThe change from
encode_e3_paramstoencode_bfv_paramsis consistent with the PR's goal of removing address encoding and simplifying the parameter encoding process.
Line range hint
1-38: Summary: Changes align well with PR objectivesThe modifications in this file consistently support the PR's goal of removing address encoding from the
e3Params. The simplification of theArgsstruct, removal ofencode_e3_params, and focus on BFV parameter encoding contribute to a more streamlined and focused codebase. These changes appear to be well-considered and align with the discussion mentioned in the PR description.packages/evm/deploy/mocks.ts (4)
14-18: LGTM. Deployment order changed as expected.The deployment of MockDecryptionVerifier remains unchanged in its implementation, but its position in the deployment order has been adjusted as mentioned in the AI summary. This change doesn't affect its functionality or usage later in the file.
20-24: LGTM. New MockInputValidator aligns with PR objectives.The addition of the MockInputValidator deployment is in line with the PR objectives of relying on the Mock contract to provide the inputValidator. This change supports the removal of inputValidator encoding from e3Params.
Line range hint
1-54: Overall, changes align well with PR objectives.The modifications in this file successfully implement the intended changes outlined in the PR objectives. The reordering and renaming of mock contract deployments, along with the introduction of MockInputValidator, effectively support the removal of inputValidator encoding from e3Params. The changes are consistent and well-implemented.
26-30: LGTM. Significant change in MockE3Program deployment.The deployment of MockE3Program (previously MockComputeProvider) now includes the address of MockInputValidator as an argument. This change is crucial and aligns perfectly with the PR objectives of removing the encoding of the inputValidator from e3Params and instead relying on the Mock contract to provide it.
To ensure this change is reflected in the contract implementation, please run the following script:
#!/bin/bash # Check the MockE3Program contract for changes related to inputValidator ast-grep --lang solidity --pattern 'contract MockE3Program { $$$ constructor($_) { $$$ } $$$ }'tests/basic_integration/test.sh (4)
93-94: LGTM: Clear comment for cipher node launch.This comment effectively introduces the subsequent operations for launching cipher nodes, improving code readability.
107-108: LGTM: Aggregator node configuration.The addition of an aggregator node with specific configuration is well-implemented and aligns with the PR objectives. The comment provides useful context about its role.
Could you clarify which of the four previously launched nodes is configured as the aggregator? It might be helpful to add this information to the comment or consider launching the aggregator node with one of the predefined addresses for consistency.
Line range hint
1-180: Overall assessment: Well-implemented changes with minor suggestions for improvement.The modifications to this integration test script align well with the PR objectives. The changes improve the handling of cipher nodes, including the parallel launch of multiple nodes and the addition of an aggregator node. The removal of the
inputValidatorfrom the encoded parameters is consistent with the PR summary.Key improvements:
- Efficient parallel launch of cipher nodes
- Clear identification of the aggregator node
- Removal of
inputValidatorfrom encoded parameters- Enhanced comments for better readability
Consider implementing the suggested refactorings and clarifications to further improve the script's maintainability and clarity.
128-129: LGTM: Removal of input validator from encoded parameters.The removal of the
--input-validatorparameter from theENCODED_PARAMSassignment aligns with the PR objectives and the discussion mentioned in the PR summary.To ensure this change doesn't have unintended consequences, please verify that:
- The Mock contract correctly provides the
inputValidator.- Any code that previously relied on the
inputValidatorbeing part ofENCODED_PARAMShas been updated accordingly.You can use the following command to check for any remaining references to
inputValidatorin the codebase:packages/evm/test/Enclave.spec.ts (2)
Line range hint
577-594: LGTM! Test case properly updated.The changes to this test case correctly adapt to the new
e3ProgramParamsformat and the way the input validator address is handled. The test now directly verifies that the correct input validator address is set in the E3 instance, which maintains good test coverage for this functionality.
Line range hint
1-1337: Overall, the changes look good and maintain test coverage.The modifications in this file consistently adapt the tests to the new
e3ProgramParamsformat, removing the encoding of the input validator address. The changes are focused and appear to maintain the existing test coverage while reflecting the updated implementation.Key points:
- The setup function has been updated to pass the input validator address separately.
- Test cases checking the E3 instance properties have been adjusted to verify the input validator address directly.
- The
e3ProgramParamsformat has been simplified across all relevant tests.These changes align well with the PR objective of removing the address from encoding.
packages/evm/contracts/test/MockE3Program.sol (4)
9-10: Declaration ofstorageInputValidatoris appropriateAdding
storageInputValidatoras a private state variable of typeIInputValidatorappropriately stores the input validator instance.
11-13: Constructor correctly initializesstorageInputValidatorThe constructor accepts an
IInputValidatorand initializesstorageInputValidator, which is correct.
26-26: Correctly updatedvalidatefunction toviewAdding the
viewmodifier tovalidateis appropriate since it now reads from the state variablestorageInputValidator.
33-33:validatefunction now usesstorageInputValidatordirectlyAssigning
inputValidator = storageInputValidatoraligns with the PR objective of removing the encoding ofinputValidatorfrome3ProgramParams.packages/ciphernode/core/src/evm_enclave.rs (3)
9-9: Import ofAddressfromalloy::primitivesis necessary and correctThe addition of
primitives::{Address}ensures that theAddresstype is available for use in this file, particularly in theconnect_evm_enclavefunction wherecontract_address: Addressis a parameter.
53-53: Direct assignment ofprogram_paramstoparamssimplifies the conversionThe direct conversion of
e3ProgramParamsto a vector and assigning it toparamsremoves unnecessary decoding logic. This aligns with the goal of streamlining parameter handling and enhances code clarity.
Line range hint
70-80: Ensure test coverage is maintained after removing the test moduleThe removal of the test module (
#[cfg(test)] mod tests) may reduce the overall test coverage for this file. To maintain code reliability and prevent regressions, please ensure that adequate tests are added elsewhere to cover the functionalities that were previously tested.Run the following script to identify areas lacking test coverage:
Additionally, verify that there are no residual references to
inputValidator:✅ Verification successful
Test Coverage Maintained
Inline tests using
#[test]annotations are present inevents.rsandordered_set.rs, ensuring that test coverage remains intact despite the removal of the separate test module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all Rust test modules in the codebase to verify test coverage. # Test: Search for all occurrences of test modules. Expect: Tests exist for critical functionalities. rg --type rust '#\[cfg\(test\)\] mod tests' --prettyLength of output: 54
Script:
#!/bin/bash # Description: Search for any remaining references to `inputValidator` in the codebase. # Test: Find all occurrences of `inputValidator`. Expect: Only necessary usages remain. rg --type rust 'inputValidator'Length of output: 107
Script:
#!/bin/bash # Description: Search for Rust test functions and test files to verify test coverage. # Test: Find all occurrences of test functions annotated with #[test]. rg --type rust '#\[test\]' --pretty # Test: Find all test files following the *_test.rs naming convention or located in the tests/ directory. rg --type rust 'tests?(/|/.*_test\.rs)' --filesLength of output: 423
After the conversation with @auryn-macmillan this removes the encoding of the inputValidator from the e3Params and relies on the Mock contract to pass in the inputValidator
e3ProgramParams now only need to be the bytes representation of the BfvParamerters
Summary by CodeRabbit
Release Notes
New Features
E3RequestedandCiphertextOutputPublishedevents.MockE3Programcontract to allow dynamic input validator management.Bug Fixes
Enclavecontract.Chores