Implemented the Identity Verification#51
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe vault initializer now stores an Identity Registry address. On booking, the contract reads that address and synchronously invokes the registry's Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Vault as PaymentVault
participant Store as Storage
participant Registry as IdentityRegistry
Client->>Vault: book_session(expert, amount)
Vault->>Store: get_registry_address()
Store-->>Vault: registry_address
Vault->>Registry: invoke is_verified(expert)
alt Expert Verified
Registry-->>Vault: true
Vault->>Vault: process booking (deposit, state updates)
Vault-->>Client: Success
else Expert Not Verified
Registry-->>Vault: false
Vault-->>Client: VaultError::ExpertNotVerified
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
contracts/payment-vault-contract/src/test.rs (3)
29-29: Unused parameter warning.The
expertparameter is unused. Prefix with underscore to silence the compiler warning.Suggested fix
- pub fn is_verified(env: Env, expert: Address) -> bool { + pub fn is_verified(_env: Env, _expert: Address) -> bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` at line 29, The function is_verified currently takes an unused parameter expert which triggers a compiler warning; rename the parameter to _expert (or prefix with underscore) in the is_verified function signature to silence the unused parameter warning while keeping the API shape (update any internal references if present), i.e., change the parameter from expert: Address to _expert: Address in the is_verified declaration.
22-22: Unused import in mock module.
Symbolis imported but never used in themock_registrymodule.Suggested fix
- use soroban_sdk::{contract, contractimpl, Address, Env, Symbol}; + use soroban_sdk::{contract, contractimpl, Address, Env};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` at line 22, The import list includes Symbol but it is unused in the mock_registry test module; remove Symbol from the use statement that reads use soroban_sdk::{contract, contractimpl, Address, Env, Symbol}; (or if Symbol will be needed, reference it where required), ensuring the mock_registry-related tests compile without unused-import warnings by keeping only the actually used symbols (contract, contractimpl, Address, Env).
5-5: Unused import.
Symbolis imported but not used in the test file itself (it's only used inside themock_registrymodule which has its own import).Suggested fix
use soroban_sdk::{ testutils::{Address as _, Ledger}, - token, Address, Env, Symbol, + token, Address, Env, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` at line 5, The import list in this test file includes an unused Symbol (`token, Address, Env, Symbol`) which is only used inside the mock_registry module; remove Symbol from the top-level import to avoid the unused-import warning. Update the use/import statement to `token, Address, Env` (or delete only `Symbol`) so that the test file imports only symbols it actually uses while leaving mock_registry's own Symbol import intact.
🤖 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/payment-vault-contract/src/test.rs`:
- Around line 71-72: Remove the duplicate registry creation: there are two
consecutive calls to create_mock_registry(&env) assigning to the same variable
`registry`; keep a single call to create_mock_registry(&env) (as in
`test_initialization`) and delete the redundant line so `registry` is only
created once.
- Around line 50-51: The test contains a duplicate call to
create_mock_registry(&env) that assigns to the same variable registry twice;
remove the redundant line so registry is only created once (keep the second/used
instance) and ensure any subsequent references use that single registry variable
(look for the create_mock_registry function call and the registry variable in
the test).
- Around line 20-34: The current mock (mock_registry::MockRegistry with
is_verified) always returns true so the negative path isn't tested; add either a
second mock (e.g., mock_registry_unverified::MockRegistryUnverified implementing
is_verified -> false) or modify MockRegistry to be parameterizable, then
register the unverified mock via env.register and add a test (e.g.,
test_book_session_fails_for_unverified_expert) that initializes the contract
client (call client.init with the registry), sets the expert rate
(client.set_my_rate), mints tokens for the user, calls
client.try_book_session(&user, &expert, &amount) and asserts the call fails with
the ExpertNotVerified error.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Line 29: The function is_verified currently takes an unused parameter expert
which triggers a compiler warning; rename the parameter to _expert (or prefix
with underscore) in the is_verified function signature to silence the unused
parameter warning while keeping the API shape (update any internal references if
present), i.e., change the parameter from expert: Address to _expert: Address in
the is_verified declaration.
- Line 22: The import list includes Symbol but it is unused in the mock_registry
test module; remove Symbol from the use statement that reads use
soroban_sdk::{contract, contractimpl, Address, Env, Symbol}; (or if Symbol will
be needed, reference it where required), ensuring the mock_registry-related
tests compile without unused-import warnings by keeping only the actually used
symbols (contract, contractimpl, Address, Env).
- Line 5: The import list in this test file includes an unused Symbol (`token,
Address, Env, Symbol`) which is only used inside the mock_registry module;
remove Symbol from the top-level import to avoid the unused-import warning.
Update the use/import statement to `token, Address, Env` (or delete only
`Symbol`) so that the test file imports only symbols it actually uses while
leaving mock_registry's own Symbol import intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7be10a2-f56a-435f-9221-78bb97b82042
📒 Files selected for processing (5)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/error.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
|
Hi @Bosun-Josh121 pr sent kindly review |
|
@KAMALDEEN333 fix CI errors |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/payment-vault-contract/src/test.rs (2)
1127-1148:⚠️ Potential issue | 🔴 CriticalScale & Pagination tests also missing
registryparameter.The following tests in the "Scale & Pagination Tests" section need the same fix:
test_scale_50_bookings_single_user_with_pagination(line 1148)test_pagination_isolation_between_users(line 1212)test_expert_pagination_50_bookings(line 1258)🐛 Proposed fix for test_scale_50_bookings_single_user_with_pagination
let admin = Address::generate(&env); let user = Address::generate(&env); let expert = Address::generate(&env); let oracle = Address::generate(&env); + let registry = create_mock_registry(&env); let token_admin = Address::generate(&env); let token = create_token_contract(&env, &token_admin); // Mint enough tokens: rate=1, duration=1 per booking, 50 bookings = 50 tokens token.mint(&user, &50_000); let client = create_client(&env); - client.init(&admin, &token.address, &oracle); + client.init(&admin, &token.address, &oracle, ®istry);Apply the same pattern to
test_pagination_isolation_between_usersandtest_expert_pagination_50_bookings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 1127 - 1148, The three scale & pagination tests (test_scale_50_bookings_single_user_with_pagination, test_pagination_isolation_between_users, test_expert_pagination_50_bookings) are missing the registry argument when initializing the client; update each test to create or obtain the registry Address (similar to other tests) and pass that registry into client.init(...) alongside &admin, &token.address, &oracle so the init call includes the registry parameter; ensure you follow the same pattern used in other tests for creating the registry Address and passing it to client.init in each of the three named test functions.
690-706:⚠️ Potential issue | 🔴 CriticalTests missing required
registryparameter will fail compilation.The
initfunction signature was updated to require 4 arguments, but multiple tests in the "Key Rotation Tests" section still use the old 3-argument signature. This will cause compilation errors.Affected tests in this section:
test_transfer_admin_success(line 701)test_new_admin_can_pause_after_transfer(line 719)test_old_admin_loses_privileges_after_transfer(line 738)test_set_oracle_success(line 767)test_non_admin_cannot_transfer_admin(line 793)test_non_admin_cannot_set_oracle(line 813)🐛 Proposed fix for test_transfer_admin_success
let admin_a = Address::generate(&env); let admin_b = Address::generate(&env); let token = Address::generate(&env); let oracle = Address::generate(&env); + let registry = create_mock_registry(&env); let client = create_client(&env); - client.init(&admin_a, &token, &oracle); + client.init(&admin_a, &token, &oracle, ®istry);Apply the same pattern to all affected tests in this section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 690 - 706, The failing tests call client.init with three args but init now requires four; update each affected test (e.g., test_transfer_admin_success, test_new_admin_can_pause_after_transfer, test_old_admin_loses_privileges_after_transfer, test_set_oracle_success, test_non_admin_cannot_transfer_admin, test_non_admin_cannot_set_oracle) to generate a registry address (let registry = Address::generate(&env);) and pass ®istry as the fourth argument to client.init(&admin_a, &token, &oracle, ®istry) (use the same pattern where create_client and other setup occur).
🧹 Nitpick comments (1)
contracts/payment-vault-contract/src/test.rs (1)
902-931: Good test coverage for the verification failure path.The test correctly configures the mock registry to return
falseand verifies thatbook_sessionfails. Consider strengthening the assertion to verify the specific error variant:💡 Suggested improvement to verify specific error
let res = client.try_book_session(&user, &expert, &max_duration); - assert!(res.is_err()); + assert_eq!( + res.unwrap_err().unwrap(), + crate::error::VaultError::ExpertNotVerified + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 902 - 931, The test test_book_session_fails_if_expert_not_verified only asserts that try_book_session returned an error; update it to assert the specific error variant (e.g., ContractError::ExpertNotVerified) so the failure is unambiguous. Replace the generic assert!(res.is_err()) with a check that unwraps or matches the error from client.try_book_session(&user, &expert, &max_duration) and compares it to the ExpertNotVerified variant (or use matches! to assert the variant) to ensure the exact error is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 1127-1148: The three scale & pagination tests
(test_scale_50_bookings_single_user_with_pagination,
test_pagination_isolation_between_users, test_expert_pagination_50_bookings) are
missing the registry argument when initializing the client; update each test to
create or obtain the registry Address (similar to other tests) and pass that
registry into client.init(...) alongside &admin, &token.address, &oracle so the
init call includes the registry parameter; ensure you follow the same pattern
used in other tests for creating the registry Address and passing it to
client.init in each of the three named test functions.
- Around line 690-706: The failing tests call client.init with three args but
init now requires four; update each affected test (e.g.,
test_transfer_admin_success, test_new_admin_can_pause_after_transfer,
test_old_admin_loses_privileges_after_transfer, test_set_oracle_success,
test_non_admin_cannot_transfer_admin, test_non_admin_cannot_set_oracle) to
generate a registry address (let registry = Address::generate(&env);) and pass
®istry as the fourth argument to client.init(&admin_a, &token, &oracle,
®istry) (use the same pattern where create_client and other setup occur).
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 902-931: The test test_book_session_fails_if_expert_not_verified
only asserts that try_book_session returned an error; update it to assert the
specific error variant (e.g., ContractError::ExpertNotVerified) so the failure
is unambiguous. Replace the generic assert!(res.is_err()) with a check that
unwraps or matches the error from client.try_book_session(&user, &expert,
&max_duration) and compares it to the ExpertNotVerified variant (or use matches!
to assert the variant) to ensure the exact error is asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c7ce93-9a4f-4f54-a37f-cccccedbad6a
📒 Files selected for processing (4)
contracts/payment-vault-contract/src/contract.rscontracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/payment-vault-contract/src/contract.rs
- contracts/payment-vault-contract/src/storage.rs
…MALDEEN333/SkillSphere-Contracts into Enforce-Identity-Verification
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/payment-vault-contract/src/storage.rs`:
- Around line 10-16: The DataKey enum currently contains duplicate variants
(Booking, BookingCounter, UserBookings, ExpertBookings, IsPaused, ExpertRate)
which breaks compilation; remove the first set of those variants (the initial
Booking, BookingCounter, UserBookings(Address), ExpertBookings(Address),
IsPaused, ExpertRate(Address) entries) and keep the later, intended
composite-key variants (e.g., BookingCounter, UserBooking, ExpertBooking,
UserBookings, ExpertBookings, IsPaused, ExpertRate) so the enum has only one
definition per variant name; after removing the duplicates, run a build and
update any references to the removed names to use the retained composite
variants (look for usages of DataKey::Booking, DataKey::UserBookings,
DataKey::ExpertBookings, DataKey::IsPaused, DataKey::ExpertRate, and
DataKey::BookingCounter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a3ea6d3-5ba4-46d9-9979-57a3ac28c937
📒 Files selected for processing (1)
contracts/payment-vault-contract/src/storage.rs
| RegistryAddress, | ||
| Booking(u64), // Booking ID -> BookingRecord | ||
| BookingCounter, // Counter for generating unique booking IDs | ||
| UserBookings(Address), // User Address -> Vec<u64> of booking IDs | ||
| ExpertBookings(Address), // Expert Address -> Vec<u64> of booking IDs | ||
| IsPaused, // Circuit breaker flag | ||
| ExpertRate(Address), // Expert Address -> rate per second (i128) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: detect duplicate enum variants in DataKey.
python - <<'PY'
import re
from collections import Counter
from pathlib import Path
p = Path("contracts/payment-vault-contract/src/storage.rs")
src = p.read_text()
m = re.search(r'pub\s+enum\s+DataKey\s*\{(?P<body>.*?)\n\}', src, re.S)
if not m:
print("DataKey enum not found")
raise SystemExit(1)
body = m.group("body")
names = []
for raw in body.splitlines():
line = raw.split("//", 1)[0].strip().rstrip(",")
if not line:
continue
mm = re.match(r'([A-Za-z_][A-Za-z0-9_]*)', line)
if mm:
names.append(mm.group(1))
dups = {k: v for k, v in Counter(names).items() if v > 1}
print("Duplicate variants:", dups if dups else "none")
PYRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Read the full DataKey enum definition from storage.rs
cat -n contracts/payment-vault-contract/src/storage.rs | head -50Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 2362
Remove duplicate DataKey enum variants to fix compilation.
The enum defines Booking, BookingCounter, IsPaused, and ExpertRate twice (lines 11–16 and lines 17–27). Duplicate variant identifiers are invalid in Rust and will cause a compile error.
Keep the second set of variants (lines 17–27), which include the new composite-key structure (UserBooking, ExpertBooking, etc.), and remove the first occurrence of each duplicate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/storage.rs` around lines 10 - 16, The
DataKey enum currently contains duplicate variants (Booking, BookingCounter,
UserBookings, ExpertBookings, IsPaused, ExpertRate) which breaks compilation;
remove the first set of those variants (the initial Booking, BookingCounter,
UserBookings(Address), ExpertBookings(Address), IsPaused, ExpertRate(Address)
entries) and keep the later, intended composite-key variants (e.g.,
BookingCounter, UserBooking, ExpertBooking, UserBookings, ExpertBookings,
IsPaused, ExpertRate) so the enum has only one definition per variant name;
after removing the duplicates, run a build and update any references to the
removed names to use the retained composite variants (look for usages of
DataKey::Booking, DataKey::UserBookings, DataKey::ExpertBookings,
DataKey::IsPaused, DataKey::ExpertRate, and DataKey::BookingCounter).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/payment-vault-contract/src/test.rs (1)
932-937:⚠️ Potential issue | 🟠 MajorAssert the exact
ExpertNotVerifiederror, not justis_err().This test currently passes on any failure reason, so it does not fully validate the acceptance criterion for the identity gate.
Suggested fix
- assert!(res.is_err()); + assert!(matches!( + res, + Err(Ok(crate::error::VaultError::ExpertNotVerified)) + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 932 - 937, The test currently only checks that booking failed; update it to assert the exact ExpertNotVerified error instead of using is_err(). Replace the final assertion with a precise check on the returned error from client.try_book_session(&user, &expert, &max_duration) — e.g. unwrap_err() compared to the ExpertNotVerified variant of your error enum (or use a pattern match like matches!(res, Err(ContractError::ExpertNotVerified))) so the test verifies the identity-gate rejection explicitly.
🧹 Nitpick comments (1)
contracts/payment-vault-contract/src/test.rs (1)
699-703: Prefercreate_mock_registry(&env)over randomAddress::generate(&env)in init tests.Using a real registered contract address keeps setup realistic and avoids brittle failures if init-time registry validation is added later.
Also applies to: 718-722, 738-742, 795-799, 816-820
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/payment-vault-contract/src/test.rs` around lines 699 - 703, Replace uses of Address::generate(&env) with create_mock_registry(&env) in the init tests so the registry is a realistic registered contract; specifically update the registry assignment used before client.init(&admin_a, &token, &oracle, ®istry) to let registry = create_mock_registry(&env) and do the same for the other occurrences noted (around the init calls at the other ranges). This ensures the registry address is a proper mock contract address the test environment recognizes instead of a random generated Address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 932-937: The test currently only checks that booking failed;
update it to assert the exact ExpertNotVerified error instead of using is_err().
Replace the final assertion with a precise check on the returned error from
client.try_book_session(&user, &expert, &max_duration) — e.g. unwrap_err()
compared to the ExpertNotVerified variant of your error enum (or use a pattern
match like matches!(res, Err(ContractError::ExpertNotVerified))) so the test
verifies the identity-gate rejection explicitly.
---
Nitpick comments:
In `@contracts/payment-vault-contract/src/test.rs`:
- Around line 699-703: Replace uses of Address::generate(&env) with
create_mock_registry(&env) in the init tests so the registry is a realistic
registered contract; specifically update the registry assignment used before
client.init(&admin_a, &token, &oracle, ®istry) to let registry =
create_mock_registry(&env) and do the same for the other occurrences noted
(around the init calls at the other ranges). This ensures the registry address
is a proper mock contract address the test environment recognizes instead of a
random generated Address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b7b3569-ec92-4e68-9847-02f181156b11
📒 Files selected for processing (2)
contracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/payment-vault-contract/src/storage.rs
🧠 SkillSphere Pull Request 🌐
Implemented the Enforce Identity Verification on Booking
Description
Collaborator
Target Contract: payment-vault-contract
Labels: phase-2-security, cross-contract
Description
Currently, book_session accepts any Address as an expert. The Payment Vault must synchronously ask the Identity Registry if the expert is actually verified before accepting the deposit.
Tasks
Storage (src/storage.rs):
Add DataKey::RegistryAddress to store the Identity Registry's contract address.
Update initialize_vault to accept registry: Address and save it.
Logic (src/contract.rs):
In book_session, retrieve the registry address from storage.
Execute a cross-contract call using env.invoke_contract::(®istry_address, &soroban_sdk::Symbol::new(env, "is_verified"), soroban_sdk::vec![env, expert.to_val()]).
Validation:
If the cross-contract call returns false, immediately return a new error VaultError::ExpertNotVerified.
Tests (src/test.rs):
Deploy a mock Registry Contract in the test environment.
Assert that book_session succeeds when the mock registry returns true.
Assert that book_session fails with ExpertNotVerified when the mock returns false.
Acceptance Criteria
Cross-contract call successfully reads the Identity Registry state.
Users are strictly prevented from booking unverified experts.
Target Contract: payment-vault-contract
Labels: phase-2-security, cross-contract
Description
Currently, book_session accepts any Address as an expert. The Payment Vault must synchronously ask the Identity Registry if the expert is actually verified before accepting the deposit.
Tasks
Closes #26
cargo test(All tests passed)📌 Type of Change
📝 Changes Description
(Briefly describe what you changed and why. If this modifies a Smart Contract, explain the impact on storage or gas.)
📸 Evidence
(A photo, log output, or Soroban CLI output is required as evidence. If this is a smart contract change, please include the test results or invocation logs.)
🌌 Comments
(Any extra notes for the reviewer? Are there any specific edge cases they should test?)
Thank you for contributing to SkillSphere! 🌍
We are glad you have chosen to help us democratize access to knowledge on the Stellar network. Your contribution brings us one step closer to a trustless, peer-to-peer consulting economy. Let's build the future together! 🚀
Summary by CodeRabbit
New Features
Bug Fixes
Tests