Fix: Resolve CI Pipeline Compilation and Clippy Errors#292
Closed
walterthesmart wants to merge 1 commit into
Closed
Fix: Resolve CI Pipeline Compilation and Clippy Errors#292walterthesmart wants to merge 1 commit into
walterthesmart wants to merge 1 commit into
Conversation
- Resolved unit test context panics in common crate - Fixed metering billing calculation error - Removed redundant clippy attribute in teye-state-channel - Fixed zkv verification arguments in identity - Resolved clippy constant assertion warnings in zk_verifier - Aligned vision_records methods and access level
WalkthroughMultiple contracts updated across key functionality: test scaffolding executes within contract contexts; ZK credential verification implemented with full proof validation and contract client invocation; billing cost calculation simplified; record retrieval methods require provider address parameter; access control standardized to Admin level; type imports reorganized by submodule. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Env as Env / Storage
participant Cred as verify_zk_credential
participant VKClient as ZkVerifierContractClient
participant Verifier as External Verifier
Test->>Cred: invoke(env, user, resource_id, proofs, public_inputs, expires_at, nonce)
Cred->>Env: read verifier_address from storage
alt Verifier not configured
Cred-->>Test: CredentialError::VerifierNotSet
else Verifier found
Cred->>Cred: validate proof lengths & materialize G1/G2 components
Cred->>Cred: construct Proof(a, b, c) & AccessRequest
Cred->>VKClient: try_verify_access(proof, access_request)
VKClient->>Verifier: invoke_verify(...)
alt Verification succeeds
Verifier-->>VKClient: Ok(true)
VKClient-->>Cred: Ok(true)
Cred-->>Test: Ok(true)
else Verification fails
Verifier-->>VKClient: Err(VerificationFailed)
VKClient-->>Cred: Err(...)
Cred-->>Test: Err(CredentialError::ZkVerificationFailed)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 (3)
contracts/vision_records/src/test_batch.rs (1)
259-264: Use a provider principal in the provider-scoped not-found test.At Line 264, using
admincan mask provider-path auth regressions if admin is privileged. Prefer a registered provider actor for this specific test intent.Suggested patch
fn test_batch_get_records_not_found() { let (env, client, admin) = setup(); + let provider = register_provider(&env, &client, &admin); let mut ids = Vec::new(&env); ids.push_back(999u64); - let result = client.try_get_records(&admin, &ids); + let result = client.try_get_records(&provider, &ids); assert_eq!( result.err().unwrap().unwrap(), ContractError::RecordNotFound ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vision_records/src/test_batch.rs` around lines 259 - 264, The test currently calls client.try_get_records(&admin, &ids) which uses the privileged admin principal and can mask provider-scoped auth regressions; change the test to use a registered provider principal instead (obtain or create a provider actor from the test fixture/setup and pass that principal to client.try_get_records). Specifically replace the use of admin in the call to client.try_get_records (and any setup that creates ids via Vec::new(&env) / ids.push_back(999u64)) with the provider principal returned or registered via your test setup so the test exercises provider-path authorization rather than admin privileges.contracts/vision_records/src/test_occ.rs (1)
97-99: Consider adding one least-privilege negative case for update authorization.These tests now always grant admin-level access; adding a single
AccessLevel::Read(or lower) update attempt that must fail would better guard authorization boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vision_records/src/test_occ.rs` around lines 97 - 99, Add a negative authorization test alongside the existing Admin-case by creating a token or context with AccessLevel::Read (instead of AccessLevel::Admin) and attempting the same update operation you use in this test file; assert that the update call returns an error/unauthorized result (e.g., Err or specific auth error) rather than succeeding. Locate where AccessLevel::Admin is used in test_occ.rs, duplicate that setup but swap to AccessLevel::Read and expect failure when calling the update function (the same update invocation used in the current test).contracts/identity/src/credential.rs (1)
56-74: Safe unwrap usage, but consider explicit handling for defense-in-depth.The
try_into().unwrap()calls on lines 56-65 are safe because the source slices are guaranteed to be exactly 32 bytes (derived from fixed-size arrays). However, for smart contract code where panics can have significant consequences, you might consider usingtry_into().map_err(|_| CredentialError::ZkVerificationFailed)?for defense-in-depth.♻️ Optional: Replace unwrap with explicit error handling
- let a_x = BytesN::from_array(env, &a_buf[0..32].try_into().unwrap()); - let a_y = BytesN::from_array(env, &a_buf[32..64].try_into().unwrap()); + let a_x = BytesN::from_array(env, &a_buf[0..32].try_into().map_err(|_| CredentialError::ZkVerificationFailed)?); + let a_y = BytesN::from_array(env, &a_buf[32..64].try_into().map_err(|_| CredentialError::ZkVerificationFailed)?);Apply similarly to
b_x0,b_x1,b_y0,b_y1,c_x, andc_y.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity/src/credential.rs` around lines 56 - 74, Replace the unchecked try_into().unwrap() calls used to build BytesN values (a_x, a_y, b_x0, b_x1, b_y0, b_y1, c_x, c_y) with fallible conversions that map failures to a CredentialError (e.g. use try_into().map_err(|_| CredentialError::ZkVerificationFailed)?), and propagate the Result out of the surrounding function if it currently returns () so the ? can be used; update the construction of Proof (G1Point/G2Point) to occur after successful conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/identity/src/credential.rs`:
- Around line 76-91: The AccessRequest is being built with a hardcoded nonce
(0); instead, before constructing AccessRequest in credential.rs use the
existing ZkVerifierContractClient (ZkVerifierContractClient::new -> client) to
retrieve the current per-user nonce via client.get_nonce(&user) and assign that
to AccessRequest.nonce (or alternatively accept nonce as a parameter and pass it
through); also handle any error from get_nonce and map it to
CredentialError::ZkVerificationFailed consistent with the try_verify_access
error path so that the nonce fetch failure doesn't crash and subsequent
try_verify_access calls use the correct nonce.
---
Nitpick comments:
In `@contracts/identity/src/credential.rs`:
- Around line 56-74: Replace the unchecked try_into().unwrap() calls used to
build BytesN values (a_x, a_y, b_x0, b_x1, b_y0, b_y1, c_x, c_y) with fallible
conversions that map failures to a CredentialError (e.g. use
try_into().map_err(|_| CredentialError::ZkVerificationFailed)?), and propagate
the Result out of the surrounding function if it currently returns () so the ?
can be used; update the construction of Proof (G1Point/G2Point) to occur after
successful conversions.
In `@contracts/vision_records/src/test_batch.rs`:
- Around line 259-264: The test currently calls client.try_get_records(&admin,
&ids) which uses the privileged admin principal and can mask provider-scoped
auth regressions; change the test to use a registered provider principal instead
(obtain or create a provider actor from the test fixture/setup and pass that
principal to client.try_get_records). Specifically replace the use of admin in
the call to client.try_get_records (and any setup that creates ids via
Vec::new(&env) / ids.push_back(999u64)) with the provider principal returned or
registered via your test setup so the test exercises provider-path authorization
rather than admin privileges.
In `@contracts/vision_records/src/test_occ.rs`:
- Around line 97-99: Add a negative authorization test alongside the existing
Admin-case by creating a token or context with AccessLevel::Read (instead of
AccessLevel::Admin) and attempting the same update operation you use in this
test file; assert that the update call returns an error/unauthorized result
(e.g., Err or specific auth error) rather than succeeding. Locate where
AccessLevel::Admin is used in test_occ.rs, duplicate that setup but swap to
AccessLevel::Read and expect failure when calling the update function (the same
update invocation used in the current test).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/common/src/concurrency.rscontracts/identity/src/credential.rscontracts/identity/tests/core.rscontracts/metering/src/lib.rscontracts/state_channel/src/test.rscontracts/vision_records/src/lib.rscontracts/vision_records/src/test_batch.rscontracts/vision_records/src/test_occ.rscontracts/zk_verifier/tests/test_exports.rs
💤 Files with no reviewable changes (1)
- contracts/state_channel/src/test.rs
Comment on lines
+76
to
+91
| let request = AccessRequest { | ||
| user: user.clone(), | ||
| resource_id, | ||
| proof, | ||
| public_inputs, | ||
| expires_at, | ||
| nonce, | ||
| }; | ||
|
|
||
| let client = ZkVerifierContractClient::new(env, &verifier_id); | ||
| let result = client.try_verify_access(&request); | ||
|
|
||
| match result { | ||
| Ok(Ok(true)) => Ok(true), | ||
| _ => Err(CredentialError::ZkVerificationFailed), // Any failure maps to VerificationFailed | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for nonce handling in zk_verifier
rg -n -A 5 'nonce' contracts/zk_verifier/src/Repository: Stellar-Teye/Teye-Contracts
Length of output: 2460
🏁 Script executed:
#!/bin/bash
# Look for verify_access implementation
ast-grep --pattern $'fn verify_access($$$) {
$$$
}'Repository: Stellar-Teye/Teye-Contracts
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check the verify_access method in the contract client
rg -n -B 2 -A 10 'try_verify_access' contracts/identity/src/credential.rsRepository: Stellar-Teye/Teye-Contracts
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Get context on the lib.rs nonce usage mentioned in review
rg -n -B 3 -A 3 'nonce' contracts/identity/src/lib.rsRepository: Stellar-Teye/Teye-Contracts
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Search for get_nonce calls in identity contract
rg -n 'get_nonce\|nonce' contracts/identity/src/Repository: Stellar-Teye/Teye-Contracts
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Look at the full context of the verify_access_with_proof function in lib.rs
rg -n -B 10 -A 5 'verify_access_with_proof' contracts/identity/src/lib.rs | head -50Repository: Stellar-Teye/Teye-Contracts
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search entire codebase for get_nonce usage
rg -n 'get_nonce' contracts/Repository: Stellar-Teye/Teye-Contracts
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Check tests or examples for verify_access_with_proof patterns
rg -n -B 3 -A 3 'verify_access_with_proof' contracts/Repository: Stellar-Teye/Teye-Contracts
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Look at the nonce usage pattern in test_zk_access.rs
rg -n -B 5 -A 10 'let nonce = client.get_nonce' contracts/zk_verifier/tests/test_zk_access.rsRepository: Stellar-Teye/Teye-Contracts
Length of output: 761
🏁 Script executed:
#!/bin/bash
# Check the exact location in credential.rs where the nonce should be fetched
sed -n '450,470p' contracts/identity/src/lib.rsRepository: Stellar-Teye/Teye-Contracts
Length of output: 809
Critical: The nonce parameter must be fetched from the ZK verifier before each call to avoid verification failure on subsequent invocations.
The zk_verifier contract does enforce strict per-user nonce validation (checked at line 434-438 in zk_verifier/src/lib.rs and incremented after each successful call at line 490). However, the identity contract breaks this integration by passing a hardcoded nonce: 0 every time, causing the function to fail on the second invocation because the stored nonce will have advanced to 1.
The correct pattern—shown in test_zk_access.rs lines 1080-1087—is to fetch the current nonce from the verifier before constructing the AccessRequest:
let nonce = client.get_nonce(&user);
let request = AccessRequest {
// ...
nonce,
};Update credential.rs to accept the current nonce as a parameter or fetch it from the ZK verifier client within the function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/identity/src/credential.rs` around lines 76 - 91, The AccessRequest
is being built with a hardcoded nonce (0); instead, before constructing
AccessRequest in credential.rs use the existing ZkVerifierContractClient
(ZkVerifierContractClient::new -> client) to retrieve the current per-user nonce
via client.get_nonce(&user) and assign that to AccessRequest.nonce (or
alternatively accept nonce as a parameter and pass it through); also handle any
error from get_nonce and map it to CredentialError::ZkVerificationFailed
consistent with the try_verify_access error path so that the nonce fetch failure
doesn't crash and subsequent try_verify_access calls use the correct nonce.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This pull request resolves a series of compilation failures, test panics, and clippy warnings that were breaking the CI pipeline across multiple workspace crates.
Changes Implemented
common: Fixed unit test storage panics (MissingValue) due to lack of a contract environment. Injected aTestContractmock registry and wrapped all tests inside closures usingenv.as_contract.metering: Addressed an arithmetic calculation bug resulting in incorrectly overpriced invoices. Removed redundantsaturating_mul()calls withinclose_billing_cycleas tracking components already native map metric bounds appropriately.identity: Fixedverify_zk_credentialcompilation issues by removing invalid explicit0offset arguments fromcopy_into_slicecalls.teye-state-channel: Removed redundant explicit global#![cfg(test)]attribute intest.rsto fix-D clippy::duplicated-attributeserrors.zk_verifier: Removed excessive and redundantassert!(true)statements in test validation macros halting builds under-D clippy::assertions_on_constantsfilters.vision_records: Defined aligned namespaces withAccessLevel::Admin, implemented missing methods such asget_records&batch_countand addressed variables name shadowing compiler breaks.Checklist
TestContractregistry wrappingenv.as_contract. (commoncrate)close_billing_cyclearithmetic execution flaws (meteringcrate).teye-state-channel.identity.vision_records.cargo fmt --all -- --check).cargo check --workspace).cargo clippy --workspace --all-targets -- -D warnings).cargo test).Summary by CodeRabbit
New Features
Bug Fixes
Improvements