Excubiae Integration #241
Excubiae Integration #241
Conversation
WalkthroughThis pull request introduces extensive changes across the repository. The GitHub Actions workflow now includes additional steps for running network tests, summarizing results, and updates to artifact action versions. Multiple Solidity contracts and interfaces have been updated: functions now accept a new Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GitHub as GitHub Actions
participant Prebuild as Prebuild Job
participant Test as Test Job
Dev->>GitHub: Push commit
GitHub->>Prebuild: Trigger workflow (install deps, lint, compile, prebuild tests)
Prebuild->>GitHub: Upload artifacts (using upload-artifact@v4)
GitHub->>Test: Trigger test job (set permissions, download artifacts)
Test->>GitHub: Run network tests and summarize results
sequenceDiagram
participant User
participant Enclave
participant E3Program
participant Policy
User->>Enclave: Call request(..., inputLimit, ...)
Enclave->>E3Program: Validate request with inputLimit
E3Program-->>Enclave: Return encryption scheme and IEnclavePolicy
Enclave->>Policy: Call enforceWithLimit(subject, evidence, checkType)
Policy-->>Enclave: Return enforcement result
Enclave->>User: Emit event (e.g., CloneDeployed) and respond
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
ryardley
force-pushed
the
ry/240-excubiae
branch
from
30893dc to
0fa2f50
Compare
…irectory structure
…to use InputValidatorPolicy
Apply excubiae integration
…tead of BasePolicy contract
|
So after doing this it occurred to me that a flaw in this implementation is that we are checking subject -> target -> state and that the subject and target here must be an address. This means we kind of need one address per E3Request. We may need to somehow generate an address from the e3_id which might be a possible workaround. |
The new version of Excubiae means this will be a new proxy contract deployed per E3Request via the Factory |
…inCheckAlreadyEnforced error handling
BasePolicy -> AdvancedPolicy
Upgrade excubiae
![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 comments (1)
packages/evm/tasks/enclave.ts (1)
124-134: Update request task to include the inputLimit parameter.The
requesttask needs to be updated to include the newinputLimitparameter that was added to theIEnclaveinterface.Add the following parameter to the task and update the
requestcall:task( "committee:new", "Request a new ciphernode committee, will use E3 mock contracts by default", ) + .addOptionalParam( + "inputLimit", + "maximum number of inputs allowed for this E3", + 255, + types.int, + ) // ... other parameters ... // Update request call const tx = await enclaveContract.request( filterAddress, [taskArguments.thresholdQuorum, taskArguments.thresholdTotal], [taskArguments.windowStart, taskArguments.windowEnd], taskArguments.duration, + taskArguments.inputLimit, e3Address, e3Params, computeParams, { value: "1000000000000000000" }, );
🧹 Nitpick comments (13)
packages/evm/contracts/test/MockInputValidatorPolicy.sol (2)
16-19: Documentation is clear and well-scoped.
The docstrings accurately describe the contract’s responsibilities and reference extension of Base/AdvancedPolicy.
24-37: Consider additional validation in _initialize().
If advancedCheckerAddr or sender must never be zero, you could add explicit checks to avoid a misconfigured deployment.packages/evm/contracts/Enclave.sol (4)
82-90: New Excubiae error definitions.
These are standard for advanced policy enforcement. Consider whether ZeroAddress or any reentrancy checks are needed if external calls can lead to reentrancy.
135-135: New inputLimit parameter in request.
This design effectively surfaces the notion of how many times MAIN checks are allowed. Ensure the E3 front-end acknowledges this new parameter.
249-252: Payload creation and enforceWithLimit usage.
Creating a new bytes array and passing to enforceWithLimit is correct. Be mindful of potential gas overhead from repeated memory allocations if inputs are large or frequent.
259-259: Explicit success assignment.
Setting success to true just before the event come across as straightforward, though it’s purely a local variable.packages/evm/test/Enclave.spec.ts (2)
45-54: Consider returning all events matching the name for multi-event scenarios.
Currently, this helper returns the first matching event by name. If multiple events can be emitted with the same name, you might need to capture all of them.You could optionally collect all matching events:
- const event = receipt.logs.find( - (log) => log instanceof EventLog && log.fragment.name === name, - ) as EventLog | undefined; + const events = receipt.logs.filter( + (log) => log instanceof EventLog && log.fragment.name === name, + ) as EventLog[]; + if (events.length === 0) throw new Error("Events not found"); + return events[0]; // or return all events if needed
593-611: Consider removing or refining the commented-out test.
The “XXX” note indicates a future refactor or alternative testing approach.Would you like help rewriting this test with a different strategy or to open an issue for it?
packages/evm/contracts/test/MockInputValidatorChecker.sol (1)
8-11: Improve documentation to explain success criteria.The current documentation doesn't explain why
evidence[0].length == 3leads tosuccess = false. This logic seems counterintuitive and needs clarification.Apply this diff to enhance the documentation:
/// @title MockInputValidatorChecker. /// @notice Enclave Input Validator /// @dev Extends BaseChecker for input verification. +/// @dev Returns false when evidence[0].length is 3, true otherwise. +/// @dev This is a mock implementation for testing purposes only.packages/evm/contracts/test/MockInputValidatorPolicyFactory.sol (1)
8-10: Fix incorrect NatSpec comments.The NatSpec comments incorrectly reference "AdvancedERC721Policy" instead of "MockInputValidatorPolicy".
Apply this diff to fix the documentation:
-/// @title AdvancedERC721PolicyFactory -/// @notice Factory for deploying minimal proxy instances of AdvancedERC721Policy. -/// @dev Encodes configuration data for multi-phase policy validation. +/// @title MockInputValidatorPolicyFactory +/// @notice Factory for deploying minimal proxy instances of MockInputValidatorPolicy. +/// @dev Encodes configuration data for input validation policy.packages/evm/contracts/interfaces/IE3Program.sol (1)
7-14: Update NatSpec comments for inputLimit parameter.The NatSpec comments are missing documentation for the new
inputLimitparameter.Apply this diff to update the documentation:
/// @param e3Id ID of the E3. /// @param seed Seed for the computation. + /// @param inputLimit Maximum number of times that input may be submitted. /// @param e3ProgramParams ABI encoded computation parameters. /// @param computeProviderParams ABI encoded compute provider parameters. /// @return encryptionSchemeId ID of the encryption scheme to be used for the computation. - /// @return inputValidator The input validator to be used for the computation. + /// @return inputValidator The policy to be used for input validation.packages/evm/contracts/interfaces/IE3.sol (1)
17-18: Update NatSpec comment for inputValidator field.The NatSpec comment still refers to "input validator contract" instead of "policy contract".
Apply this diff to update the documentation:
-/// @param inputValidator Address of the input validator contract. +/// @param inputValidator Address of the input validation policy contract.packages/evm/contracts/interfaces/IEnclave.sol (1)
106-106: Update function documentation to include the new parameter.The
requestfunction's documentation should be updated to include theinputLimitparameter.Add the following line to the function's documentation:
/// @param duration The duration of the computation in seconds. + /// @param inputLimit The maximum number of inputs allowed for this E3. /// @param e3Program Address of the E3 Program.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/evm/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/integration.yml(4 hunks)packages/evm/.prettierignore(1 hunks)packages/evm/.solhintignore(1 hunks)packages/evm/contracts/Enclave.sol(6 hunks)packages/evm/contracts/interfaces/IE3.sol(2 hunks)packages/evm/contracts/interfaces/IE3Program.sol(2 hunks)packages/evm/contracts/interfaces/IEnclave.sol(1 hunks)packages/evm/contracts/interfaces/IEnclaveChecker.sol(1 hunks)packages/evm/contracts/interfaces/IEnclavePolicy.sol(1 hunks)packages/evm/contracts/interfaces/IEnclavePolicyFactory.sol(1 hunks)packages/evm/contracts/interfaces/IInputValidator.sol(0 hunks)packages/evm/contracts/test/MockE3Program.sol(1 hunks)packages/evm/contracts/test/MockInputValidator.sol(0 hunks)packages/evm/contracts/test/MockInputValidatorChecker.sol(1 hunks)packages/evm/contracts/test/MockInputValidatorPolicy.sol(1 hunks)packages/evm/contracts/test/MockInputValidatorPolicyFactory.sol(1 hunks)packages/evm/deploy/mocks.ts(2 hunks)packages/evm/package.json(2 hunks)packages/evm/tasks/enclave.ts(1 hunks)packages/evm/test/Enclave.spec.ts(47 hunks)packages/evm/test/fixtures/MockE3Program.fixture.ts(1 hunks)packages/evm/test/fixtures/MockInputValidator.fixture.ts(0 hunks)packages/evm/test/fixtures/MockInputValidatorChecker.fixture.ts(1 hunks)packages/evm/test/fixtures/MockInputValidatorPolicyFactory.fixture.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/evm/contracts/interfaces/IInputValidator.sol
- packages/evm/contracts/test/MockInputValidator.sol
- packages/evm/test/fixtures/MockInputValidator.fixture.ts
✅ Files skipped from review due to trivial changes (2)
- packages/evm/.solhintignore
- packages/evm/.prettierignore
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/integration.yml
22-22: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
41-41: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
52-52: the runner of "actions/cache@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
96-96: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
101-101: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
107-107: the runner of "actions/cache@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (persist)
- GitHub Check: test (base)
🔇 Additional comments (67)
.github/workflows/integration.yml (17)
26-28: Network tests step formatting.
The "Run network tests" step, as marked on these lines, appears to have only formatting adjustments with no change in functionality.
37-39: Checkout step minor formatting update.
The diff marker on these lines indicates a minor formatting change in the "Check out the repo" step of the prebuild job. No functional impact is observed.
57-59: Dependencies installation step formatting.
The "Install the dependencies" step contains a minor formatting change (as indicated by the marker) without impacting functionality.
71-73: Run prebuild step formatting update.
The "Run prebuild" step shows minor formatting changes. Verify that "yarn test:integration prebuild" still behaves as expected.
74-82: Upload build artifacts step update.
The step now correctly uses "actions/upload-artifact@v4." This update aligns with the new requirements and appears properly configured.
88-90: Test job strategy configuration update.
The diff at these lines reflects some formatting updates in defining the matrix strategy for the test job. No functional changes are detected here.
92-94: Checkout step in test job formatting.
Minor formatting changes in the "Check out the repo" step of the test job are noted. There is no functional impact.
112-114: Install dependencies step formatting update.
The diff marker here suggests only minor formatting changes to the dependencies installation step; no further action is needed.
115-121: Download build artifacts step validation.
The "Download build artifacts" step now uses "actions/download-artifact@v4" and includes updated settings. The changes here appear to be straightforward improvements with only formatting differences.
122-127: Executable permissions step formatting update.
The diff marker in the "Set executable permissions" step indicates minor adjustments. The chmod commands look correctly configured.
127-130: Run tests step formatting update.
The changes in the "Run ${{ matrix.test-suite }} tests" step are cosmetic and do not affect functionality.
21-25: 🛠️ Refactor suggestionUpdate the setup-node action version.
The "Setup node" step in the test-net job still uses "actions/setup-node@v2," which has been flagged by static analysis as outdated. Upgrading to a more recent version (for example, "actions/setup-node@v3") will improve compatibility and reliability.Proposed diff:
- uses: actions/setup-node@v2 + uses: actions/setup-node@v3✅ Verification successful
Upgrade the Setup Node Action Version
The integration workflow still uses the outdated "actions/setup-node@v2". Since static analysis has flagged this version as outdated, please update it to "actions/setup-node@v3" as suggested.
- Update the file at .github/workflows/integration.yml (line 21) to change:
- uses: actions/setup-node@v2
- uses: actions/setup-node@v3
🧰 Tools
🪛 actionlint (1.7.4)
22-22: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
106-111: 🛠️ Refactor suggestionUpdate the cache action version in the test job.
Similar to the prebuild job, the test job's caching step uses "actions/cache@v2." An upgrade to a newer version (e.g., v3) is suggested for improved performance.Proposed diff:
- uses: actions/cache@v2 + uses: actions/cache@v3✅ Verification successful
Update the cache action version in the test job
The test job in .github/workflows/integration.yml still uses "actions/cache@v2" for caching node modules, which is inconsistent with the updated prebuild job. Please update it to "actions/cache@v3" for enhanced performance, as suggested.
- File: .github/workflows/integration.yml (lines 106–111)
- Suggested change: Replace "actions/cache@v2" with "actions/cache@v3"
🧰 Tools
🪛 actionlint (1.7.4)
107-107: the runner of "actions/cache@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
40-44: 🛠️ Refactor suggestionUpdate the setup-node action in the prebuild job.
The prebuild job’s node setup still uses "actions/setup-node@v2." It is recommended to update this to the latest version (e.g., v3) for improved support.Proposed diff:
- uses: actions/setup-node@v2 + uses: actions/setup-node@v3✅ Verification successful
Update prebuild job's Node setup action to v3
The integration workflow file (.github/workflows/integration.yml) currently uses actions/setup-node@v2. This update should be applied to use the latest version (v3) in the prebuild job, ensuring improved support and performance.
- File: .github/workflows/integration.yml (lines 40-44): Change
uses: actions/setup-node@v2touses: actions/setup-node@v3🧰 Tools
🪛 actionlint (1.7.4)
41-41: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
51-56: 🛠️ Refactor suggestionUpdate the cache action version.
The "Cache node modules" step currently uses "actions/cache@v2." As per static analysis, updating this action version may improve caching performance.Proposed diff:
- uses: actions/cache@v2 + uses: actions/cache@v3✅ Verification successful
Action: Update Cache Action Version in Workflow
The ".github/workflows/integration.yml" file still refers to "actions/cache@v2" in the "Cache node modules" step. Upgrading to "actions/cache@v3" is recommended to take advantage of improvements in caching performance and related optimizations.
- File: .github/workflows/integration.yml (lines 51–56)
- Proposed change: update the action reference from "actions/cache@v2" to "actions/cache@v3"
🧰 Tools
🪛 actionlint (1.7.4)
52-52: the runner of "actions/cache@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
95-99: 🛠️ Refactor suggestionUpdate the setup-node action in the test job.
This step also uses "actions/setup-node@v2." Upgrading to a newer version (e.g., v3) is recommended for consistency and improved support.Proposed diff:
- uses: actions/setup-node@v2 + uses: actions/setup-node@v3✅ Verification successful
Action: Update the GitHub Action to use the newer v3 version of setup-node
The integration workflow uses the outdated version (actions/setup-node@v2) in the test job. Upgrading to v3 ensures consistency across the repository and benefits from improved support and features. Please update the workflow as follows:
- File: .github/workflows/integration.yml
- Change
uses: actions/setup-node@v2touses: actions/setup-node@v3as proposed.- Also, verify if any other jobs in your repository are still using v2 and update them where applicable.
🧰 Tools
🪛 actionlint (1.7.4)
96-96: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-50: 🛠️ Refactor suggestionUpgrade the Rust toolchain action version.
The "Install Rust 1.81.0" step uses "actions-rs/toolchain@v1," which is considered outdated. Upgrading to a newer version can help avoid compatibility issues in your workflow.Proposed diff:
- uses: actions-rs/toolchain@v1 + uses: actions-rs/toolchain@v2(Note: Verify the availability and compatibility of the newer version before applying.)
✅ Verification successful
Below is a shell script to extract and verify the toolchain installation step from the integration workflow file:
Please run the above script to confirm that the workflow is indeed using "actions-rs/toolchain@v1". Once confirmed, upgrading to "actions-rs/toolchain@v2" is recommended to take advantage of the latest improvements and avoid potential compatibility issues.
🏁 Script executed:
#!/bin/bash echo "Extracting lines 40-60 from .github/workflows/integration.yml:" sed -n '40,60p' .github/workflows/integration.yml echo "" echo "Searching for occurrences of actions-rs/toolchain:" rg "actions-rs/toolchain@" .github/workflows/integration.ymlLength of output: 974
Upgrade actions-rs/toolchain to v2
The shell script results confirmed that the integration workflow file is using "actions-rs/toolchain@v1" on lines 45–50. Upgrading to "actions-rs/toolchain@v2" as proposed should help avoid compatibility issues.🧰 Tools
🪛 actionlint (1.7.4)
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
packages/evm/contracts/test/MockE3Program.sol (5)
4-5: Imports alignment looks correct.
The newly introduced imports for IEnclavePolicy and IEnclavePolicyFactory are consistent with the refactor away from IInputValidator.
12-13: Using immutable state variables is a good practice.
The introduction of immutable variables POLICY_FACTORY and ENCLAVE_CHECKER ensures they are set once and cannot be modified later, improving contract safety and clarity.
23-23: Verify the appropriateness of uint8 for inputLimit.
An 8-bit integer permits a maximum of 255 for inputLimit. If a larger limit is anticipated, consider using a larger type (like uint16 or uint256) to avoid possible overflow or re-deployment later.
28-28: Return signature alignment.
Returning (bytes32, IEnclavePolicy) aligns well with IE3Program requirements after refactoring. No further concerns here.
35-38: Ensure deploy() and setTarget() calls return valid references.
After deploying the enclave policy, consider checking that the returned address is non-zero to detect any factory errors. Relying on setTarget() after an unsuccessful deploy could silently fail.packages/evm/contracts/test/MockInputValidatorPolicy.sol (6)
1-3: File header looks good.
License and pragma statements follow standard conventions.
4-14: Imports are consistent with Excubiae structure.
The imported contracts and interfaces appear aligned with the new policy-based approach.
20-21: Error naming is consistent.
MainCalledTooManyTimes error name is clear and descriptive.
22-23: Public inputLimit is appropriate.
Exposing inputLimit publicly can help external calls query usage constraints.
39-50: Check for off-by-one edge case in enforceWithLimit.
Once status.main equals inputLimit, it reverts. Ensure that is the intended behavior (vs. reverting strictly when status.main > inputLimit).
52-57: Policy trait method is correct.
The trait() function returning "MockInputValidator" is a neat way to identify the contract.packages/evm/contracts/Enclave.sol (6)
5-12: Expanded imports accurately align with the new Excubiae policy approach.
IEnclavePolicy, IAdvancedPolicy, and Check are properly added to support the policy-based mechanism.
71-71: Updated error signature references the advanced policy interface.
Switching from IInputValidator to IAdvancedPolicy ensures error clarity under the refactored policy approach.
92-93: New CloneDeployed event is helpful for tracking ephemeral contract creation.
Ensure it is emitted at the correct location(s) once clone deployment logic is in place.
164-171: Passing inputLimit to e3Program.validate.
Implementation is consistent with the updated IE3Program interface.
254-254: PoseidonT3 for input hashing.
Hashing data & inputCounts in a single block is a good approach for input uniqueness. Carefully verify that collisions are negligible.
261-261: Event emission.
Logging InputPublished fosters transparency about new inputs. Good addition for off-chain listeners.packages/evm/test/Enclave.spec.ts (19)
8-8: No issues with the new import.
12-12: Looks good.
The import from "../types" is appropriate for test usage.
18-19: Fixture import additions are valid.
These imports are well-named and precise, aligning with the new validation approach.
41-43: The SetupConfig type is a clear approach.
Defining a dedicated type for test configuration, including 'inputLimit', improves readability and organization.
56-61: Good configurable defaults approach.
Returning merged config with a baseline of { inputLimit: 0 } is straightforward and maintainable.
63-73: Deployment function is clear.
Centralizing fixture deployments in one place simplifies configuration across tests.
75-75: Setup fixture updates are consistent.
Accepting a dynamic config parameter ensures tests can be tailored easily.
83-84: No concerns.
Storing validator contracts for further usage is correctly handled here.
87-88: Using factory addresses in e3Program is valid.
Looks consistent with the new input validator policy mechanism.
123-123: Including 'inputLimit' in the request object is fine.
This parameter flows cleanly into the subsequent logic.
234-248: Requesting with the new 'inputLimit' parameter appears correct.
The test ensures that the contract call respects the new field.
255-255: Verifying 'inputValidator' from events.
Ensuring the test checks equality with the deployed clone address is a solid addition.
485-485: New 'inputLimit' usage in tests is consistent.
Ensuring the contract call includes 'request.inputLimit' covers the updated interface.
629-631: Instantiation checks with 'inputLimit' are valid.
This ensures newly created E3 instances reflect the input limit.
642-642: Extracting 'CloneDeployed' event is appropriate.
Reusing the 'extractEventLog' helper clarifies event-based checks.
650-650: Ensuring the E3 structure references 'inputValidator' from the event.
These checks help confirm correct wiring of the newly deployed policy proxy.
985-1010: Test for unlimited input rounds (inputLimit = 0).
This scenario is well covered; verifying multiple calls to publishInput ensures the contract logic matches expectations.
1011-1035: Test for single allowed input round (inputLimit = 1).
Validating that a second input leads to a revert is crucial for checking limit enforcement.
1036-1074: Allowing inputs to separate E3 requests is properly tested.
This scenario ensures each request enforces its own input limit independently.packages/evm/contracts/interfaces/IEnclavePolicyFactory.sol (1)
1-11: Confirm 'uint8' capacity for '_inputLimit'.
This interface is concise. However, if you anticipate inputLimit values above 255, consider using a larger integer type.packages/evm/contracts/interfaces/IEnclaveChecker.sol (1)
1-12: Minimal extension interface looks clean.
Providing a contract-specific checker interface that inherits from IAdvancedChecker aligns with the new validation approach.packages/evm/test/fixtures/MockE3Program.fixture.ts (1)
5-11: LGTM! Clean refactor to support policy-based architecture.The changes align well with the PR objectives, replacing the input validator with policy factory and checker parameters.
packages/evm/test/fixtures/MockInputValidatorChecker.fixture.ts (1)
1-12: LGTM! Clean implementation of the checker fixture.The fixture follows the established pattern and integrates well with the new policy-based architecture.
packages/evm/test/fixtures/MockInputValidatorPolicyFactory.fixture.ts (1)
1-12: LGTM! Clean implementation of the policy factory fixture.The fixture follows the established pattern and integrates well with the new policy-based architecture.
packages/evm/contracts/interfaces/IEnclavePolicy.sol (2)
11-13: LGTM! Well-documented interface for policy-based validation.The interface is clean, well-documented, and appropriately extends IAdvancedPolicy.
14-18: Verify the limit parameter implementation.The function name
enforceWithLimitsuggests it enforces a limit, but the limit parameter is not explicitly visible in the function signature. Please verify:
- Where is the limit parameter stored/passed?
- How is the limit enforced within the implementation?
packages/evm/deploy/mocks.ts (3)
20-24: LGTM! Deployment configuration for MockInputValidatorChecker.The deployment configuration is correct with the empty array argument.
26-30: LGTM! Deployment configuration for InputValidatorPolicy.The deployment configuration correctly uses the
mockInputValidatorChecker.addressas an argument.
45-51: LGTM! Setting target for InputValidatorPolicy.The target is correctly set to the Enclave contract address, allowing the enclave contract to call the policy.
packages/evm/tasks/enclave.ts (1)
93-99: LGTM! Updated variable name and error message.The variable name and error message have been correctly updated to use
MockInputValidatorChecker.packages/evm/package.json (3)
46-46: LGTM! Updated solhint version.The solhint version has been updated to the latest minor version.
94-96: LGTM! Added @excubiae/contracts dependency.The @excubiae/contracts dependency has been added with version ^0.2.0.
Let's verify if this is the latest version:
88-88: Verify the update:excubiae script exists.The script
./scripts/update_excubiae.shneeds to be present in the repository.
packages/evm/contracts/test/MockInputValidatorPolicyFactory.sol
Outdated
Show resolved
Hide resolved
|
…t deploy function to owner, update mocks.ts for factory ownership transfer, and adjust Enclave.spec.ts for new setup
…ckE3Program constructor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/evm/contracts/test/MockE3Program.sol (1)
17-27: Consider combining zero-address checks for gas efficiency.While the zero-address checks are good, they could be combined to save gas:
constructor(IEnclavePolicyFactory _policyFactory, address _enclaveChecker) { - if (_enclaveChecker == address(0)) { - revert InvalidChecker(); - } - - if (address(_policyFactory) == address(0)) { - revert InvalidPolicyFactory(); - } + if (_enclaveChecker == address(0) || address(_policyFactory) == address(0)) { + revert (_enclaveChecker == address(0) ? InvalidChecker() : InvalidPolicyFactory()); + } POLICY_FACTORY = _policyFactory; ENCLAVE_CHECKER = _enclaveChecker; }packages/evm/deploy/mocks.ts (1)
20-33: Add error handling for mock deployments.While the deployments are correct, consider adding try-catch blocks to handle potential deployment failures:
- const mockInputValidatorChecker = await deploy("MockInputValidatorChecker", { - from: deployer, - args: [[]], - log: true, - }); + let mockInputValidatorChecker; + try { + mockInputValidatorChecker = await deploy("MockInputValidatorChecker", { + from: deployer, + args: [[]], + log: true, + }); + } catch (err) { + console.error("Failed to deploy MockInputValidatorChecker:", err); + process.exit(1); + }packages/evm/test/Enclave.spec.ts (2)
40-60: Consider enhancing type safety in setup configuration.While the setup configuration is well organized, consider:
- Making inputLimit required in SetupConfig
- Adding input validation for the limit value
type SetupConfig = { - inputLimit: number; + inputLimit: number & { _brand: 'InputLimit' }; }; function defaultSetupConfig(config: Partial<SetupConfig> = {}): SetupConfig { + const limit = config.inputLimit ?? 0; + if (limit < 0) throw new Error('Input limit cannot be negative'); return { - inputLimit: 0, + inputLimit: limit as SetupConfig['inputLimit'], ...config, }; }
989-1014: Consider adding edge cases to input limit tests.While the current tests cover basic scenarios, consider adding:
- Test with maximum possible input limit
- Test with very large numbers close to uint8 max
- Test behavior when limit is exceeded multiple times
it("reverts when input limit is exceeded multiple times", async function () { const fixtureSetup = () => setup(defaultSetupConfig({ inputLimit: 1 })); const { enclave, request } = await loadFixture(fixtureSetup); await enclave.request(...); await enclave.activate(0, ethers.ZeroHash); await enclave.publishInput(0, "0x12345678"); await expect(enclave.publishInput(0, "0x12345678")) .to.be.revertedWithCustomError(enclave, "MainCalledTooManyTimes"); await expect(enclave.publishInput(0, "0x12345678")) .to.be.revertedWithCustomError(enclave, "MainCalledTooManyTimes"); });Also applies to: 1015-1039
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/evm/contracts/test/MockE3Program.sol(1 hunks)packages/evm/contracts/test/MockInputValidatorPolicyFactory.sol(1 hunks)packages/evm/deploy/mocks.ts(1 hunks)packages/evm/test/Enclave.spec.ts(48 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/evm/contracts/test/MockInputValidatorPolicyFactory.sol
🧰 Additional context used
🪛 GitHub Actions: EVM
packages/evm/test/Enclave.spec.ts
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
[error] 91-91: contract runner does not support sending transactions (operation="sendTransaction", code=UNSUPPORTED_OPERATION, version=6.12.2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-net
- GitHub Check: ci
- GitHub Check: Build Image
- GitHub Check: prebuild
🔇 Additional comments (6)
packages/evm/contracts/test/MockE3Program.sol (3)
4-5: LGTM! Clean interface updates.The changes properly reflect the transition from input validation to enclave policy management with appropriate error types.
Also applies to: 9-10
14-15: LGTM! Good use of immutable variables.Using immutable variables for factory and checker addresses is a good practice as it ensures these critical addresses cannot be modified after deployment.
29-49: LGTM! Clean implementation of policy-based validation.The validate function properly:
- Accepts the new inputLimit parameter
- Uses the factory to deploy a new policy
- Sets the target address
- Returns the correct interface type
packages/evm/deploy/mocks.ts (2)
40-47: LGTM! Correct dependency injection.The MockE3Program deployment properly uses the new factory and checker addresses.
49-57: LGTM! Well-handled ownership transfer.The ownership transfer to the E3Program contract is properly implemented with appropriate error handling.
packages/evm/test/Enclave.spec.ts (1)
1040-1078: LGTM! Good test coverage for request isolation.The tests properly verify that input limits are enforced independently for each request.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/evm/test/Enclave.spec.ts (3)
44-53: Enhance error messages in extractEventLog function.The error messages could be more descriptive to aid in debugging test failures.
Consider this improvement:
- if (receipt == null) throw new Error("Receipt not returned"); + if (receipt == null) throw new Error(`Transaction receipt not returned for tx hash: ${tx.hash}`); - if (!event) throw new Error("Event not found"); + if (!event) throw new Error(`Event '${name}' not found in transaction logs`);
989-1078: Add edge cases to input limit test suite.While the current test cases cover the basic scenarios, consider adding these edge cases:
- Maximum input limit value
- Negative input limit value (should revert)
- Input limit greater than uint256 max value
Example test case:
it("reverts when input limit is negative", async function () { const fixtureSetup = () => setup(defaultSetupConfig({ inputLimit: -1 })); const { enclave, request } = await loadFixture(fixtureSetup); await expect( enclave.request( request.filter, request.threshold, request.startTime, request.duration, request.inputLimit, request.e3Program, request.e3ProgramParams, request.computeProviderParams, { value: 10 }, ), ).to.be.revertedWithCustomError(enclave, "InvalidInputLimit"); });
989-1487: Consider reorganizing test cases for better readability.The test cases for input limit functionality are mixed with other test cases. Consider grouping them in a separate describe block for better organization:
describe("input limit functionality", function () { // Move all input limit related tests here });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/evm/test/Enclave.spec.ts(48 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: prebuild
- GitHub Check: test-net
- GitHub Check: ci
- GitHub Check: Build Image
🔇 Additional comments (3)
packages/evm/test/Enclave.spec.ts (3)
8-8: LGTM! Well-structured imports and type definitions.The new imports and type definitions are clean and properly support the input limit functionality.
Also applies to: 40-42
90-94: Address TODO comment regarding ownership transfer.The TODO comment questions if the ownership transfer is too restrictive. This needs to be addressed as it could impact the contract's flexibility.
Please clarify:
- Why is this ownership transfer necessary?
- What are the implications of this restriction?
- Are there alternative approaches?
597-615: Resolve commented test case for invalid computation request.The comment suggests finding a different way to test this scenario. This is important for ensuring proper validation of computation requests.
The test case for invalid computation requests should be reimplemented. Would you like me to help design an alternative approach that aligns with the new input validation architecture?
Present a version of utilizing Excubiae for managing input validation.
At a later date we can do things like work out how to aggregate checkers especially when Excubiae creates the aggregation API which is yet to be specified.
Rhetorical considerations for reviewers
MockInputValidatorPolicyFactory- I guess we just need to be sure this is easily auditable.closes: #240
MockInputValidatorCheckerInputValidatorPolicywhich usesIBaseCheckerIInputValidatorforIBasePolicy.enforce(address, evidence)instead of.validateand update to useInputValidatorPolicy.MockInputValidatorSummary by CodeRabbit