chore: allow mock input validator in CRISP#782
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
WalkthroughRefactors CRISP input-validator provisioning to accept an injected validator address, adds a MockCRISPInputValidator, updates deployment to choose and pass the validator address to the factory, updates example E3 program addresses, refreshes some artifact build metadata, and consolidates deploy logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Deployer
participant MockValidator as MockCRISPInputValidator
participant RealValidator as CRISPInputValidator
participant Factory as CRISPInputValidatorFactory
Deployer->>Deployer: Read USE_MOCK_INPUT_VALIDATOR
alt use mock
Deployer->>MockValidator: deploy()
Deployer->>Deployer: inputValidator = MockValidator.address
else use real
Deployer->>RealValidator: deploy()
Deployer->>Deployer: inputValidator = RealValidator.address
end
Deployer->>Factory: new CRISPInputValidatorFactory(inputValidator)
Note right of Deployer: Log consolidated deployment summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol (1)
42-48: Consider documenting why state is unused in the mock.The
validatefunction skips all verification checks (appropriate for a mock), but thepolicy,noirVerifierstate variables andInvalidInputData,InvalidNoirProoferrors are never used. This might confuse developers reading the code.Consider adding a comment explaining that this mock bypasses validation for testing purposes, or remove the unused state/errors if they serve no purpose in the mock implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/client/.env.example(1 hunks)examples/CRISP/contracts/CRISPInputValidatorFactory.sol(1 hunks)examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol(1 hunks)examples/CRISP/deploy/Deploy.s.sol(2 hunks)examples/CRISP/server/.env.example(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(1 hunks)packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json(1 hunks)packages/enclave-contracts/scripts/deployEnclave.ts(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
examples/CRISP/server/.env.example
[warning] 16-16: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (11)
packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json (1)
222-222: Build metadata updated — no ABI changes in this artifactLooks fine as a buildInfoId bump aligned with other artifacts.
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json (1)
707-707: Build metadata updated — interface unchangedAcknowledging buildInfoId refresh; no functional diffs indicated.
packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json (1)
308-309: Approve metadata-only change: verified that ABI, bytecode, and deployedBytecode in all three JSON artifacts match main; only buildInfoId was updated.examples/CRISP/client/.env.example (1)
4-4: LGTM! Address update is consistent.The E3_PROGRAM_ADDRESS update aligns with the new deployment flow introduced in this PR.
Note: Static analysis suggests adding quotes and reordering keys, but these are optional style preferences for .env files and can be safely ignored.
examples/CRISP/server/.env.example (1)
16-16: LGTM! Consistent address update.The E3_PROGRAM_ADDRESS update matches the client configuration and aligns with the new deployment flow.
examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol (1)
30-36: LGTM! Initialization logic is correct.The
_initializefunction properly decodes the policy and verifier addresses from appended bytes, matching the encoding in the factory'sdeploy()method.examples/CRISP/deploy/Deploy.s.sol (3)
35-36: LGTM! Necessary imports added.The imports for both
CRISPInputValidatorandMockCRISPInputValidatorare correctly added to support the conditional deployment logic.
184-194: LGTM! Conditional deployment logic is well-structured.The conditional deployment pattern matches the existing
USE_MOCK_VERIFIERapproach and provides clear logging for which validator is deployed. The default to the real validator (false) is appropriate for production use.
196-196: LGTM! Factory instantiation updated correctly.The factory constructor now receives the deployed input validator address, successfully implementing the dependency injection pattern introduced in this PR.
examples/CRISP/contracts/CRISPInputValidatorFactory.sol (2)
15-15: LGTM! Constructor refactored for dependency injection.The constructor now accepts the input validator address as a parameter, enabling the factory to work with either the real or mock validator implementation. This is a clean separation of concerns.
27-27: Removeinitialize()compatibility concern. BothCRISPInputValidatorandMockCRISPInputValidatorinherit the externalinitialize()implementation fromClone, so casting toCRISPInputValidatorand callinginitialize()on any clone (including those of the mock) is safe.Likely an incorrect or invalid review comment.
ctrlc03
force-pushed
the
chore/mock-input-val
branch
from
3ac7ea6 to
8a80981
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol (3)
16-20: Verify: Unused state variables.The
policyandnoirVerifierstate variables are initialized in_initialize()but never used invalidate(). If this mock is intentionally bypassing policy checks and proof verification for testing purposes, consider removing these unused variables to clarify intent. Otherwise, ensure they're utilized appropriately.
22-27: Consider removing unused error definitions.
InvalidInputDataandInvalidNoirProofare defined but never thrown. If they're retained for interface consistency with the real validator, consider adding a comment to clarify. Otherwise, remove them to reduce clutter.
38-48: Consider handling decode failures gracefully.The
validate()function decodes input data without error handling. If the data structure doesn't match(bytes, bytes, bytes32[], bytes),abi.decodewill revert with a generic error rather thanInvalidInputData. For better debugging in test scenarios, consider wrapping the decode in a try-catch block.Example:
function validate(address sender, bytes memory data) external returns (bytes memory input) { if (data.length == 0) revert EmptyInputData(); - (,,, bytes memory vote) = abi.decode(data, (bytes, bytes, bytes32[], bytes)); + try this.decodeData(data) returns (bytes memory vote) { + input = vote; + } catch { + revert InvalidInputData(data); + } +} - input = vote; +function decodeData(bytes memory data) external pure returns (bytes memory) { + (,,, bytes memory vote) = abi.decode(data, (bytes, bytes, bytes32[], bytes)); + return vote; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/client/.env.example(1 hunks)examples/CRISP/contracts/CRISPInputValidatorFactory.sol(1 hunks)examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol(1 hunks)examples/CRISP/deploy/Deploy.s.sol(2 hunks)examples/CRISP/server/.env.example(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(1 hunks)packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json(1 hunks)packages/enclave-contracts/scripts/deployEnclave.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
- packages/enclave-contracts/scripts/deployEnclave.ts
- examples/CRISP/deploy/Deploy.s.sol
- examples/CRISP/contracts/CRISPInputValidatorFactory.sol
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
examples/CRISP/server/.env.example
[warning] 16-16: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: test_contracts
- GitHub Check: build_e3_support_risc0
🔇 Additional comments (3)
examples/CRISP/client/.env.example (1)
4-4: LGTM! Address update aligns with deployment changes.The new E3 program address reflects the updated CRISP deployment flow with injectable input validators.
examples/CRISP/server/.env.example (1)
16-16: LGTM! Consistent address update.The E3_PROGRAM_ADDRESS update matches the client configuration and reflects the new deployment flow.
examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol (1)
1-15: LGTM! Contract declaration and imports are correct.The mock validator correctly implements the required interfaces and imports necessary dependencies.
cedoor
left a comment
cedoor
left a comment
There was a problem hiding this comment.
utACK 👍🏽 Ready to be merged.
ctrlc03
force-pushed
the
chore/mock-input-val
branch
from
8a80981 to
29b7102
Compare
ctrlc03
force-pushed
the
chore/mock-input-val
branch
from
29b7102 to
6382c74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol (4)
16-20: Consider removing unused state variables.The
policyandnoirVerifierstate variables are initialized in_initializebut never referenced in thevalidatefunction. If they're intentionally present for future use or to maintain structural consistency with the real validator, document this. Otherwise, remove them to reduce confusion.Apply this diff if the state is genuinely unused:
- /// @notice The policy that will be used to validate the input. - IBasePolicy internal policy; - - /// @notice The verifier that will be used to validate the input. - IVerifier internal noirVerifier;And simplify
_initialize:function _initialize() internal virtual override(Clone) { super._initialize(); - - (address policyAddr, address verifierAddr) = abi.decode(_getAppendedBytes(), (address, address)); - policy = IBasePolicy(policyAddr); - noirVerifier = IVerifier(verifierAddr); + // Mock: appended bytes ignored for testing }
24-27: Remove unused error definitions.
InvalidInputDataandInvalidNoirProofare defined but never emitted. Remove them to reduce confusion about the mock's behavior.Apply this diff:
- /// @notice The error emitted when the input data is invalid. - error InvalidInputData(bytes reason); - /// @notice The error emitted when the Noir proof is invalid. - error InvalidNoirProof();
42-48: Remove unusedsenderparameter.The
senderparameter is declared but not used in the validation logic. Consider removing it or documenting why it's present but unused.Apply this diff:
- function validate(address sender, bytes memory data) external returns (bytes memory input) { + function validate(address /* sender */, bytes memory data) external returns (bytes memory input) {Alternatively, if removing would break the interface:
function validate(address /* sender */, bytes memory data) external returns (bytes memory input) {
13-15: Add documentation clarifying mock behavior.Since this is a mock validator that bypasses actual validation, add NatSpec comments explaining:
- What validation is being skipped
- Why certain state/errors from the real validator are unused
- The intended test scenarios
Example:
/// @title MockCRISPInputValidator. -/// @notice Mock Enclave Input Validator +/// @notice Mock Enclave Input Validator for testing purposes. +/// @dev This mock bypasses policy and proof validation, directly extracting +/// the vote payload from input data. Use only in test environments. contract MockCRISPInputValidator is IInputValidator, Clone {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/CRISP/client/.env.example(1 hunks)examples/CRISP/contracts/CRISPInputValidatorFactory.sol(1 hunks)examples/CRISP/contracts/Mocks/MockCRISPInputValidator.sol(1 hunks)examples/CRISP/deploy/Deploy.s.sol(2 hunks)examples/CRISP/server/.env.example(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json(1 hunks)packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json(1 hunks)packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json(1 hunks)packages/enclave-contracts/scripts/deployEnclave.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/enclave-contracts/artifacts/contracts/registry/NaiveRegistryFilter.sol/NaiveRegistryFilter.json
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
examples/CRISP/client/.env.example
[warning] 4-4: [UnorderedKey] The VITE_E3_PROGRAM_ADDRESS key should go before the VITE_ENCLAVE_API key
(UnorderedKey)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
examples/CRISP/server/.env.example
[warning] 16-16: [UnorderedKey] The E3_PROGRAM_ADDRESS key should go before the ENCLAVE_ADDRESS key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
🔇 Additional comments (6)
packages/enclave-contracts/scripts/deployEnclave.ts (1)
72-78: LGTM! Previous readability issue resolved.The consolidated deployment summary is clear and well-formatted. The spaces after colons that were requested in the previous review have been successfully applied, improving readability.
examples/CRISP/client/.env.example (1)
4-4: LGTM! Environment address updated consistently.The E3_PROGRAM_ADDRESS has been updated to match the new deployment address, consistent with the server-side configuration.
examples/CRISP/server/.env.example (1)
16-16: LGTM! Environment address synchronized.The E3_PROGRAM_ADDRESS update aligns with the client configuration and deployment changes.
examples/CRISP/contracts/CRISPInputValidatorFactory.sol (1)
14-15: Good refactor! Address injection enables flexible validator selection.The updated constructor now accepts an external validator address instead of deploying a new instance internally. This enables runtime selection between
CRISPInputValidatorandMockCRISPInputValidator, aligning well with the PR objectives.examples/CRISP/deploy/Deploy.s.sol (2)
35-36: LGTM! Imports added for validator selection.The new imports support the runtime selection between mock and real input validators.
184-200: Well-implemented validator selection logic.The runtime selection between
MockCRISPInputValidatorandCRISPInputValidatorvia theUSE_MOCK_INPUT_VALIDATORenvironment variable is clean and well-logged. The deployed validator address is correctly passed to the factory constructor, enabling the intended testing flexibility.
(cherry picked from commit 2861d9b)
* chore: update zkvm to v3.0.0 (#791) * chore: update zkvm to v3.0.0 * chore: update deployment script (cherry picked from commit 2d9133e) * chore: update risc0 version v3.0.3 (#793) * chore: update risc0 version * chore: remove risc0 crates from CRISP (cherry picked from commit c94f5f3) * chore: update lock and Dockerfile (#794) (cherry picked from commit c2d7827) * fix: risc0 dev mode env var (#810) * fix: risc0 dev mode env var * chore: lint * chore: add dev comment (cherry picked from commit 7e09bad) * chore: allow mock input validator in CRISP (#782) (cherry picked from commit 2861d9b) * chore: update cargo toml and contract --------- Co-authored-by: ctrlc03 <93448202+ctrlc03@users.noreply.github.com> Co-authored-by: гλ <contact@rudiyardley.com>
* chore: update zkvm to v3.0.0 (#791) * chore: update zkvm to v3.0.0 * chore: update deployment script (cherry picked from commit 2d9133e) * chore: update risc0 version v3.0.3 (#793) * chore: update risc0 version * chore: remove risc0 crates from CRISP (cherry picked from commit c94f5f3) * chore: update lock and Dockerfile (#794) (cherry picked from commit c2d7827) * fix: risc0 dev mode env var (#810) * fix: risc0 dev mode env var * chore: lint * chore: add dev comment (cherry picked from commit 7e09bad) * chore: allow mock input validator in CRISP (#782) (cherry picked from commit 2861d9b) * chore: update cargo toml and contract --------- Co-authored-by: ctrlc03 <93448202+ctrlc03@users.noreply.github.com> Co-authored-by: гλ <contact@rudiyardley.com>
2 participants
Allow CRISP to have a mock input validator for testing purposes
Summary by CodeRabbit