feat: add ZK authorizations to CW #346
Conversation
WalkthroughThis update introduces a zero-knowledge (ZK) authorization framework to the codebase. It adds a new verification-gateway contract for ZK proof validation, extends the authorization contract to manage and execute ZK authorizations, and updates message schemas, state, and error handling to support ZK workflows. Related utilities and test helpers are also enhanced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthorizationContract
participant VerificationGateway
User->>AuthorizationContract: ExecuteZkAuthorization { label, message, proof }
AuthorizationContract->>VerificationGateway: Query VerifyProof { vk, proof, inputs }
VerificationGateway-->>AuthorizationContract: Verification result (bool)
AuthorizationContract-->>User: Execution response (success/error)
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🔭 Outside diff range comments (2)
packages/authorization-utils/src/lib.rs (1)
1-9: 💡 Verification agent🧩 Analysis chain
Verify test coverage for the ZK authorization module
While the code structure looks good, the PR mentions limited testing due to the absence of a guest program. Consider adding unit tests that don't require a guest program to ensure the basic functionality works as expected.
🏁 Script executed:
#!/bin/bash # Check if there are any tests for the zk_authorization module find . -name "zk_authorization.rs" -o -name "*zk_authorization*_test.rs" | grep -v "target" find . -type f -name "*.rs" -exec grep -l "zk_authorization" {} \; | grep -v "target" | grep "test"Length of output: 243
Add Missing Unit Tests for
zk_authorizationModuleNo existing tests reference the
zk_authorizationmodule. To ensure core functionality is covered without relying on a guest program, please:• Create unit tests for
packages/authorization-utils/src/zk_authorization.rs
• Focus on the module’s public API (e.g., key generation, proof verification)
• Use Rust’s#[cfg(test)]framework so tests can run in CI without external dependenciescontracts/authorization/src/contract.rs (1)
571-609:⚠️ Potential issueUnvalidated recipient addresses in
mint_authorizationsEach
Mintentry’saddressis forwarded directly into a token-factory message withoutdeps.api.addr_validate, risking invalid bech32 strings and downstream failure.Insert validation:
for mint in &mints { - mint_msg( - env.contract.address.to_string(), - mint.address.clone(), + deps.api.addr_validate(&mint.address)?; // new + mint_msg( + env.contract.address.to_string(), + mint.address.clone(), mint.amount.u128(), build_tokenfactory_denom(env.contract.address.as_str(), &label), ) }This mirrors the strict checking done elsewhere in the file.
🧹 Nitpick comments (8)
contracts/verification-gateway/README.md (2)
3-3: Consider adding a comma for better readability.There's a potential grammatical improvement in this sentence.
-This contract is a simple contract that will perform the verification of ZK proofs. It's separated into a singleton contract to decouple the verification to the rest of the contracts and therefore can be extended with different verifying methods for different types of proofs. +This contract is a simple contract that will perform the verification of ZK proofs. It's separated into a singleton contract to decouple the verification to the rest of the contracts, and therefore can be extended with different verifying methods for different types of proofs.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing comma found.
Context: ...ple the verification to the rest of the contracts and therefore can be extended with diff...(AI_HYDRA_LEO_MISSING_COMMA)
1-3: Enhance documentation with usage examples and API details.As this is a new component in the system, consider expanding the documentation to include:
- Example usage patterns
- Supported proof systems (currently SP1 4.2.0 as mentioned in the PR description)
- Interface details for implementers who will interact with this gateway
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing comma found.
Context: ...ple the verification to the rest of the contracts and therefore can be extended with diff...(AI_HYDRA_LEO_MISSING_COMMA)
contracts/verification-gateway/src/error.rs (1)
4-8: Consider adding specific error variants for verification failures.As this contract's primary responsibility is verification of ZK proofs, consider adding specialized error variants for different verification failure scenarios. This would provide more granular error reporting and make debugging easier.
#[derive(Error, Debug)] pub enum ContractError { #[error("{0}")] Std(#[from] StdError), #[error("Invalid proof format")] InvalidProofFormat, #[error("Verification failed: proof is not valid")] VerificationFailed, #[error("Unsupported verification method")] UnsupportedVerificationMethod, #[error("Invalid verification key")] InvalidVerificationKey, }contracts/verification-gateway/Cargo.toml (1)
21-21: Consider adding a version comment for sp1-verifierThe sp1-verifier dependency is pinned to version 4.2.0 directly rather than using workspace versioning. This is likely intentional but may require manual updates in the future.
-sp1-verifier = "4.2.0" +sp1-verifier = "4.2.0" # Pinned for compatibility with SP1 ZK proof systemcontracts/verification-gateway/src/lib.rs (1)
7-10: Add documentation for the VerifyingKey enumAdding documentation would clarify the purpose of this enum and how it's intended to be extended for future proof systems.
#[cw_serde] +/// Represents different types of verification keys supported by the gateway. +/// Currently only supports SP1 verifying key hashes, but can be extended +/// to support other ZK proof systems in the future. pub enum VerifyingKey { + /// SP1 verifying key hash as a string SP1VerifyingKeyHash(String), }contracts/verification-gateway/src/contract.rs (1)
46-63: ZK proof verification logic is limited to one proof systemCurrently, the implementation only supports the SP1VerifyingKeyHash variant and returns
Ok(true)for successful verification without considering other verifying key types that might be needed in the future.As the codebase evolves, consider:
- Adding support for additional proof systems
- Enhancing error handling for each proof system type
- Implementing a proper return for the verification result rather than always returning
trueafter verificationThe return statement on line 62 should be removed, as the function already returns a boolean from the match statement. The current implementation could lead to confusion since it always returns
trueregardless of the match result.packages/authorization-utils/src/zk_authorization.rs (1)
58-63: ZkMessage structure may need additional validationThe
ZkMessagestructure contains information about the message to be executed but lacks validation parameters to ensure the message can only be executed once per block if that's the intention withvalidate_last_block_execution.Consider adding a mechanism to track the last executed block for each authorization, or include a nonce to ensure messages can't be replayed.
contracts/authorization/schema/valence-authorization.json (1)
1387-1421: Consider expanding modifiable fields
Themodify_zk_authorizationvariant allows updatingmax_concurrent_executionsandvalidate_last_block_execution, keyed bylabel. Should updates to theregistryor thevkbe supported here? If not, it would be helpful to document that those fields are immutable after creation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.toml(3 hunks)contracts/authorization/Cargo.toml(1 hunks)contracts/authorization/schema/valence-authorization.json(7 hunks)contracts/authorization/src/authorization.rs(2 hunks)contracts/authorization/src/contract.rs(15 hunks)contracts/authorization/src/error.rs(2 hunks)contracts/authorization/src/state.rs(1 hunks)contracts/processor/src/contract.rs(2 hunks)contracts/verification-gateway/.cargo/config.toml(1 hunks)contracts/verification-gateway/Cargo.toml(1 hunks)contracts/verification-gateway/README.md(1 hunks)contracts/verification-gateway/schema/valence-verification-gateway.json(1 hunks)contracts/verification-gateway/src/bin/schema.rs(1 hunks)contracts/verification-gateway/src/contract.rs(1 hunks)contracts/verification-gateway/src/error.rs(1 hunks)contracts/verification-gateway/src/lib.rs(1 hunks)contracts/verification-gateway/src/msg.rs(1 hunks)packages/authorization-utils/Cargo.toml(1 hunks)packages/authorization-utils/src/authorization.rs(2 hunks)packages/authorization-utils/src/lib.rs(1 hunks)packages/authorization-utils/src/msg.rs(5 hunks)packages/authorization-utils/src/zk_authorization.rs(1 hunks)packages/processor-utils/src/msg.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
contracts/authorization/src/authorization.rs (1)
contracts/authorization/src/contract.rs (1)
build_tokenfactory_denom(1726-1728)
🪛 LanguageTool
contracts/verification-gateway/README.md
[uncategorized] ~3-~3: Possible missing comma found.
Context: ...ple the verification to the rest of the contracts and therefore can be extended with diff...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (47)
packages/authorization-utils/src/authorization.rs (1)
187-212: Well-structured enum for authorization message managementThe new
AuthorizationMsgenum provides a clear structure for managing message operations like enqueueing, evicting, and inserting messages in the authorization queue. This supports the ZK authorization workflow where proofs need to be verified before execution.packages/authorization-utils/Cargo.toml (1)
15-15: Appropriate dependency for the ZK verification integrationAdding the
valence-verification-gatewaydependency aligns with the PR objectives of separating verification logic from authorization logic.contracts/authorization/Cargo.toml (1)
26-26: Proper dependency for ZK authorization implementationThe addition of
valence-verification-gatewayis consistent with the PR objective to separate verification logic from the authorization contract.packages/authorization-utils/src/lib.rs (1)
8-8: Good modularization of ZK authorization functionalityExposing the
zk_authorizationmodule makes the ZK-specific structures and functions available to external crates, supporting a clean separation of concerns.contracts/verification-gateway/README.md (1)
1-3: Clear introduction to the verification gateway concept.The README provides a concise explanation of the verification gateway's purpose and design philosophy.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing comma found.
Context: ...ple the verification to the rest of the contracts and therefore can be extended with diff...(AI_HYDRA_LEO_MISSING_COMMA)
contracts/verification-gateway/.cargo/config.toml (1)
1-3: Standard Cargo configuration for CosmWasm contracts.This configuration follows best practices for CosmWasm contract development with appropriate aliases for WebAssembly building and schema generation.
contracts/processor/src/contract.rs (2)
10-15: Good refactoring of imports to centralize authorization code.Moving the
AuthorizationMsgimport from processor utils to authorization utils improves code organization by centralizing authorization-related code.
17-22: Consistent import update following the authorization refactoring.This change correctly removes
AuthorizationMsgfrom the processor utils import to avoid duplication.contracts/verification-gateway/src/error.rs (1)
1-8: Basic error handling foundation for the verification gateway.The error module establishes a solid foundation with standard error handling patterns, wrapping the CosmWasm
StdError.contracts/verification-gateway/Cargo.toml (1)
1-20: LGTM - New verification gateway contract configured correctlyThe Cargo.toml is properly structured with appropriate workspace inheritance and CosmWasm dependencies.
contracts/verification-gateway/src/lib.rs (1)
1-6: LGTM - Module structure is clear and well-organizedThe module structure follows CosmWasm conventions with clear separation of contract, error, and message types.
Cargo.toml (2)
10-10: LGTM - Verification gateway added to workspace membersThe new contract is correctly added to the workspace members list.
116-116: LGTM - Verification gateway correctly added as a workspace dependencyThe dependency entry is properly configured with the library feature enabled, consistent with other contract dependencies.
contracts/verification-gateway/src/bin/schema.rs (1)
1-10: LGTM - Schema generation is correctly implementedThe schema generation file follows the standard CosmWasm pattern and will generate the necessary JSON schema files for contract integration.
contracts/authorization/src/state.rs (3)
5-5: LGTM - Added import for ZkAuthorizationThe ZkAuthorization import is added to support the new ZK authorization features.
11-12: LGTM - Added storage for verification gateway addressThe VERIFICATION_GATEWAY Item will store the address of the contract that handles ZK proof verification.
18-19:✅ Verification successful
Verify concurrency control for registry executions
The REGISTRY_LAST_BLOCK_EXECUTION map tracks the last block execution per registry ID, which appears to be for preventing replay or ensuring message ordering.
🏁 Script executed:
#!/bin/bash # Search for code that uses this registry execution tracking to understand the concurrency control rg -A 10 "REGISTRY_LAST_BLOCK_EXECUTION" --type rustLength of output: 2717
Concurrency control for REGISTRY_LAST_BLOCK_EXECUTION is correct
The load–compare–save pattern in contracts/authorization/src/contract.rs ensures that any message with a block number ≤ the last saved value is rejected (preventing replay), and valid messages atomically update the stored block number. Given CosmWasm’s single-threaded, atomic execution model, there’s no risk of concurrent writes for the same registry. No changes needed.
contracts/authorization/src/error.rs (2)
27-28: Well-structured error handling integration for ZK authorizations.The addition of the
ZKvariant toContractErrorfollows the established pattern of other error types, providing a clear way to propagate ZK-specific errors.
139-155: Comprehensive error enumeration for ZK features.The
ZKErrorReasonenum provides well-defined error variants for the ZK authorization flow, covering key validation scenarios: verification gateway configuration, proof validation, registry validation, proof expiration, and domain validation.This error structure aligns well with the PR objective of supporting ZK authorizations that verify proofs and check registries. Each error case has a clear, descriptive message.
packages/processor-utils/src/msg.rs (1)
3-3: Good refactoring to centralize AuthorizationMsg definition.Importing
AuthorizationMsgandPriorityfrom the shared package rather than defining them locally improves code maintainability by establishing a single source of truth for these types.This change supports the broader integration of ZK functionality across the codebase by ensuring consistent message types are used throughout the system.
contracts/verification-gateway/schema/valence-verification-gateway.json (1)
1-83: Well-defined schema for the new ZK verification gateway contract.The schema properly defines:
- An empty instantiate message (stateless contract)
- No execute messages (query-only contract)
- A single query method for proof verification
- Clear definitions for the verification key structure
The contract being query-only aligns well with its focused responsibility of proof verification.
However, the PR objectives mention the gateway is "designed to be extensible for other proof systems," but the schema currently only supports the SP1 verifier through the
s_p1_verifying_key_hashfield.Could you confirm if the
VerifyingKeytype (lines 56-71) is designed to be extensible with additional variants in the future for other proof systems?contracts/verification-gateway/src/msg.rs (3)
1-5: Clean import structure for the verification gateway.The imports are minimal and focused on what's needed for the contract's message types.
6-11: Empty InstantiateMsg and ExecuteMsg align with contract's query-only role.Creating a stateless, query-only contract for verification is a good architectural choice that separates concerns.
12-21: Well-structured query message for proof verification.The
VerifyProofquery message is properly designed with all necessary components:
- A verification key (
vk)- The proof data
- The proof inputs
The
#[returns(bool)]annotation correctly specifies the return type.This implementation aligns with the PR objective of having "a new verification-gateway contract added to handle simple proof verifications."
contracts/authorization/src/authorization.rs (2)
384-386: Correct usage of the extracted validate_permission functionThe code correctly uses the newly extracted
validate_permissionfunction, passing all required parameters from the authorization object. This change supports the addition of ZK authorizations by making permission validation more flexible.
468-502: Well-structured permission validation logicThe extracted
validate_permissionfunction properly implements the permission validation logic based on different authorization modes. This refactoring enables better support for ZK authorizations while maintaining the existing validation flow.packages/authorization-utils/src/msg.rs (6)
17-17: Properly imported ZK authorization typesThe import of
ZkAuthorizationandZkAuthorizationInfotypes is correctly added to support the new ZK authorization functionality.
186-188: Good addition of ZK authorization creation messageThis message variant allows for creating multiple ZK authorizations at once, following the same pattern as the existing
CreateAuthorizationsvariant.
196-200: ZK authorization modification parameters are appropriateThe
ModifyZkAuthorizationmessage allows updating specific ZK authorization attributes. The inclusion ofvalidate_last_block_executionflag is a good addition for ensuring sequence validation.
240-242: Gateway configuration message is clear and well-documentedThe
SetVerificationGatewaymessage provides a clean way to configure the verification gateway contract address used for ZK proof validation.
269-273: ZK authorization execution message has all required componentsThe
ExecuteZkAuthorizationmessage contains all necessary information: label to identify the authorization, the message to be executed, and the proof to be verified.
366-372: Query messages are well-structured and consistentThe added query messages for ZK authorizations and verification gateway follow the same pattern as existing queries, maintaining consistency in the API.
contracts/verification-gateway/src/contract.rs (3)
12-14: Good practice using environment variables for contract metadataUsing environment variables for contract name and version ensures these values are synchronized with the Cargo.toml definitions.
16-25: Clean instantiation implementationThe instantiate function properly sets the contract version and returns an appropriate response with an action attribute.
37-44: Properly structured query functionThe query function correctly handles the
VerifyProofquery variant and returns a binary JSON response.packages/authorization-utils/src/zk_authorization.rs (3)
1-11: Clean imports for ZK authorization moduleThe imports correctly reference the necessary dependencies including the verification gateway contract for VerifyingKey.
12-29: Well-documented ZK authorization info structureThe
ZkAuthorizationInfostruct is well-documented with clear comments explaining each field's purpose. The inclusion of optional fields with documented defaults is good practice.
31-44: The conversion method properly handles optional valuesThe
into_zk_authorizationmethod correctly handles optional fields by providing default values. Good practice setting a default of 1 for max_concurrent_executions.contracts/authorization/schema/valence-authorization.json (8)
1304-1327: Correctly models batch creation of ZK authorizations
The newcreate_zk_authorizationsvariant inPermissionedMsgaligns with theZkAuthorizationInfodefinition, enforcing an array of well-typed items and prohibiting extra fields. Verify that the contract’s Rust handler matches this structure and properly deserializes thezk_authorizationsarray.
1709-1738: Schema forexecute_zk_authorizationis valid
The newexecute_zk_authorizationpermissionless action correctly requires thelabel, a Base64-encodedmessage, and a Base64-encodedproof, withadditionalProperties: false. This matches the guest-program interface.
2052-2069: IntroduceVerifyingKeydefinition
TheVerifyingKeyschema wraps the SP1 verifying key hash (s_p1_verifying_key_hash) in a strict one-field object. Ensure the Rust struct and serde attributes use the same field name so JSON round-trips correctly.
2070-2111: DefineZkAuthorizationInfoclearly
This definition enforces required fields (domain,label,mode,registry,validate_last_block_execution,vk) with optionalmax_concurrent_executions. It mirrors the Rust type and ensures invalid payloads are rejected.
2223-2251: Add query for listing ZK authorizations
Thezk_authorizationsquery variant cleanly supports pagination vialimitandstart_after. Confirm that the contract’s Rust implementation uses the same cursor type (string) for thestart_afterparameter.
2252-2264: Add query for verification gateway address
Theverification_gatewayquery provides a simple way to retrieve the gateway address. This pattern is consistent with other address-only queries in the schema.
4307-4313: Response schema forverification_gateway
The new top-levelverification_gatewayresponse type correctly returns a single string address. Double-check client stubs to ensure this matches expected response shapes.
4314-4325: Response schema for ZK authorizations
Thezk_authorizationsresponse array (items →#/definitions/ZkAuthorization) matches state and includesstate,validate_last_block_execution,vk, andregistry. Verify that any future additions toZkAuthorizationInfoare mirrored here.contracts/authorization/src/contract.rs (1)
1828-1862: Nice utility helper
assign_execution_idcleanly rewrites the ID field while leaving the rest of the message intact.
Good defensive programming here. 👍
| // Domain this needs to be sent to | ||
| pub domain: Domain, |
There was a problem hiding this comment.
I'm unsure about this one. Each proof will be indeed domain specific, but it will also be a list per program. Is this authorization per domain? Or per program? If the latter, we need this to be a list
There was a problem hiding this comment.
I need this to be able to route it to the correct processor. Indeed each authorization be per domain. We can either add it to the authorization here or add it to the ZkMessage
There was a problem hiding this comment.
answering your question, authorizations are per domain and guest program specific (there is a VK and a registry associated to it)
| #[cw_serde] | ||
| pub enum VerifyingKey { | ||
| SP1VerifyingKeyHash(String), | ||
| } |
There was a problem hiding this comment.
I'd prefer to be agnostic about this and treat the verifying key just as Vec<u8> (or String, if better compatibility). This way, we don't statically define who will verify what, and it will be up to the on-chain verification contract to decide.
There was a problem hiding this comment.
OK, removed this and made it an array of bytes. The verification contract decides what do with it
| // Default will be 1, defines how many times a specific authorization can be executed concurrently | ||
| pub max_concurrent_executions: Option<u64>, |
There was a problem hiding this comment.
More of a question: do we eliminate this with ZK? Since everything is precomputed off-chain, I'm not sure we should rate limit on-chain.
There was a problem hiding this comment.
Sure, removed it for now. If we eventually find a reason to add this we can add.
| // If Authorization is permissioned we need to create the tokenfactory token and mint the corresponding amounts to the addresses that can | ||
| // execute the authorization | ||
| if let AuthorizationMode::Permissioned(permission_type) = &zk_authorization.mode { |
There was a problem hiding this comment.
We might change this in the future, when we finish co-processor identification and gas charging.
There was a problem hiding this comment.
Yes, this can be made all permissionless in the future
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/authorization/src/tests/user_actions.rs (1)
756-793: Incomplete test scaffold for ZK authorizations.The test provides a good foundation for testing ZK authorization flows, but is intentionally incomplete as noted in the TODO comment.
Suggestions:
- Consider adding assertions on the deserialized proof structure instead of just printing it
- Add comments explaining what the base64-encoded values represent in more detail
- Outline the remaining test steps that will be added when valid proofs can be generated
Once you have the capability to generate valid Groth16 proofs, ensure this test is completed to verify:
- Proof verification works correctly
- The processor can be paused using a valid ZK proof
- The processor can be resumed using a valid ZK proof
- Proper error handling for invalid proofs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contracts/authorization/Cargo.toml(1 hunks)contracts/authorization/src/tests/helpers.rs(2 hunks)contracts/authorization/src/tests/user_actions.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/authorization/Cargo.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
contracts/authorization/src/tests/helpers.rs (4)
contracts/authorization/src/tests/user_actions.rs (16)
wasm(52-52)wasm(60-60)wasm(71-72)wasm(93-94)wasm(150-150)wasm(159-160)wasm(184-185)wasm(209-210)wasm(247-247)wasm(256-257)wasm(280-281)wasm(304-305)wasm(369-369)wasm(378-379)wasm(400-401)wasm(424-425)packages/osmosis-utils/src/suite.rs (1)
new(53-59)contracts/authorization/src/tests/builders.rs (1)
new(12-18)packages/processor-utils/src/queue.rs (1)
new(16-26)
🔇 Additional comments (4)
contracts/authorization/src/tests/helpers.rs (2)
13-13: Clean addition of necessary imports.The expanded import statement includes all the required types for the new helper function.
199-242: Well-structured helper function for setting up verification gateway.This function follows the established pattern of other helpers in the codebase, handling the deployment and configuration of the verification gateway contract consistently. It correctly:
- Reads the WASM bytecode
- Stores and instantiates the contract
- Links it to the authorization contract via the permissioned action
contracts/authorization/src/tests/user_actions.rs (2)
5-5: Added SP1 SDK dependency import.Properly imported the required type for ZK proof deserialization.
21-26: Clean import organization.The imports have been properly updated to include the necessary functions and libraries for ZK authorization testing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/verification-gateway/src/contract.rs (1)
27-34:unimplemented!()will panic – consider graceful error handlingEven though the contract is intended to be query-only, calling this entry-point will
panic!, causing an abrupt abort instead of a standard CosmWasm error. A simpleErr(ContractError::Unsupported {})(or similar) avoids panics and makes debugging on-chain a lot easier.
🧹 Nitpick comments (1)
contracts/verification-gateway/src/contract.rs (1)
67-182: Huge inline binary blobs bloat the test – considerinclude_bytes!Embedding ~3 KB of proof/VK data directly in the source inflates compile times and obscures the test logic. A lightweight improvement is to store them under
tests/fixtures/and load with:let hello_vk: Vec<u8> = include_bytes!("../fixtures/hello_vk.bin").to_vec(); let hello_proof: Vec<u8> = include_bytes!("../fixtures/hello_proof.bin").to_vec();This keeps the test readable and avoids recompiling large constants when the file changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
contracts/verification-gateway/Cargo.toml(1 hunks)contracts/verification-gateway/src/contract.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/verification-gateway/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contracts/authorization/src/tests/user_actions.rs (4)
24-27: Avoid tight coupling to helpers from other test modulesThe new call to
instantiate_and_set_verification_gateway()brings an implicit dependency on test helper internals living in another file.
Consider exposing the gateway initialisation behind a smallTestFixtureBuilderstruct so future refactors of helper signatures do not require touching every individual test.
780-787: Extract oversized base-64 constants into test fixtures for readabilityKeeping ±1 KB base-64 strings in-line makes the test hard to scan and maintain.
Recommended pattern:- let program_vk = "LONG_BASE64 …"; - let full_proof_pause = "LONG_BASE64 …"; - let full_proof_resume = "LONG_BASE64 …"; + let program_vk = include_str!("fixtures/program_vk.b64").trim(); + let full_proof_pause = include_str!("fixtures/proof_pause.b64").trim(); + let full_proof_resume= include_str!("fixtures/proof_resume.b64").trim();The dedicated
fixtures/folder improves diff-size, enables reuse between tests, and lets you regenerate proofs with a helper script without touching Rust code.
792-798: Verify both proofs for full end-to-end coverageOnly
proof_pauseis fed intoGroth16Verifier::verify.
If the resume proof encoding were to degrade, the test would still pass.
Consider duplicating the call or turning it into a helper:for (bytes, inputs) in [ (&proof_pause_bytes, &proof_pause_inputs), (&proof_resume_bytes, &proof_resume_inputs), ] { Groth16Verifier::verify(bytes, inputs, &sp1_vk.bytes32(), &GROTH16_VK_BYTES) .expect("Groth16 verification failed"); }This keeps the sanity-check symmetrical.
840-850: Consider usingexpect()instead of bareunwrap()for actionable failure messagesExample:
-).unwrap(); +).expect("Failed to execute ZK pause authorization");When a proof or contract call fails, the explicit message pin-points which phase went wrong, speeding up debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(3 hunks)contracts/authorization/Cargo.toml(1 hunks)contracts/authorization/schema/valence-authorization.json(7 hunks)contracts/authorization/src/tests/user_actions.rs(3 hunks)packages/authorization-utils/src/zk_authorization.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- contracts/authorization/Cargo.toml
- packages/authorization-utils/src/zk_authorization.rs
- contracts/authorization/schema/valence-authorization.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/middleware/verification-gateway/src/contract.rs (1)
75-181: Enhance test coverage with additional test cases.The current test only covers the happy path. Consider adding:
- Test cases for invalid proofs that should fail verification
- Test cases for malformed inputs (invalid UTF-8 in VK, empty inputs, etc.)
- Consider moving the large test vectors to separate files or constants to improve readability
Example additional test:
#[test] fn test_verify_proof_invalid_vk() { // Test with invalid UTF-8 in VK let invalid_vk = Binary::new(vec![0xFF, 0xFE, 0xFD]); // Invalid UTF-8 let proof = Binary::new(vec![0; 100]); let inputs = Binary::new(vec![0; 32]); let result = verify_proof(&invalid_vk, proof, inputs); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Failed to parse vk hash")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(3 hunks)contracts/middleware/verification-gateway/.cargo/config.toml(1 hunks)contracts/middleware/verification-gateway/Cargo.toml(1 hunks)contracts/middleware/verification-gateway/README.md(1 hunks)contracts/middleware/verification-gateway/schema/valence-verification-gateway.json(1 hunks)contracts/middleware/verification-gateway/src/bin/schema.rs(1 hunks)contracts/middleware/verification-gateway/src/contract.rs(1 hunks)contracts/middleware/verification-gateway/src/error.rs(1 hunks)contracts/middleware/verification-gateway/src/lib.rs(1 hunks)contracts/middleware/verification-gateway/src/msg.rs(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- contracts/middleware/verification-gateway/.cargo/config.toml
- contracts/middleware/verification-gateway/src/lib.rs
- contracts/middleware/verification-gateway/README.md
- contracts/middleware/verification-gateway/Cargo.toml
- contracts/middleware/verification-gateway/src/bin/schema.rs
- Cargo.toml
- contracts/middleware/verification-gateway/src/error.rs
- contracts/middleware/verification-gateway/schema/valence-verification-gateway.json
- contracts/middleware/verification-gateway/src/msg.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-deploy
- GitHub Check: solidity contracts
- GitHub Check: setup-local-ic
🔇 Additional comments (1)
contracts/middleware/verification-gateway/src/contract.rs (1)
51-56: Consider the implications of hardcodedGROTH16_VK_BYTES.The contract uses a hardcoded verification key (
GROTH16_VK_BYTES) which means it only supports proofs generated with one specific trusted setup. This design choice should be documented.If this limitation is intentional, consider adding a comment explaining that this contract is designed for a specific SP1 verifier setup. If extensibility is desired in the future, you might want to store the verification key bytes in contract state or accept them as a parameter.
| @@ -157,6 +172,11 @@ pub fn execute( | |||
| PermissionlessMsg::RetryBridgeCreation { domain_name } => { | |||
| retry_bridge_creation(deps, env, domain_name) | |||
| } | |||
| PermissionlessMsg::ExecuteZkAuthorization { | |||
There was a problem hiding this comment.
regarding the enum variant naming here, we don't actually execute the underlying authorization, do we? just verify the zkp & send the msg to processor?
There was a problem hiding this comment.
yeah. Authorizations don't really get executed per se, they send the msg to the processor, although we Call/Execute the authorization contract to somehow indirectly trigger the execution with the eventual tick
Add ZK Authorizations in a similar fashion than the EVM solution.
This allows creating authorizations with a specific VK and Registry.
Once executed, the proof is verified and registry is checked to validate that the the message is correct and was proven by the guest program.
Allows executing any processor message and the creation of the message is now done and validated inside the guest program so the authorization contract doesn't perform any validations except the proof verification.
Adds a verification-gateway contract that does simple proof verifications, decoupling the verification from the authorization contract, which just sets this contract to eventually query it. For now only SP1 4.2.0 verifier is set up to use on this gateway, but can easily be extended to verify proofs generated by other provers.
Kinda hard to test right now without writing a guest program, so next steps will be writing a guest program and generate proofs with the right inputs and verify that the interaction works as expected.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores