Implement Batch Verification #8
Conversation
📝 WalkthroughWalkthroughAdds a new admin-gated API Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
actor Admin
end
rect rgba(200,255,200,0.5)
participant Contract as IdentityRegistryContract
end
rect rgba(255,200,200,0.5)
participant Storage
participant Events
end
Admin->>Contract: batch_add_experts(experts: Vec<Address>)
Contract->>Contract: if experts.len() > 20 -> Err(ExpertVecMax)
alt length OK
Contract->>Storage: get_admin()
Contract->>Contract: admin.require_auth()
alt not admin
Contract-->>Admin: Err(Unauthorized)
else admin authorized
loop for each expert
Contract->>Storage: get_expert_status(addr)
alt status == Verified
Contract-->>Admin: Err(AlreadyVerified)
else
Contract->>Storage: set_expert_status(addr, Verified)
Contract->>Events: emit StatusChanged(addr, prev -> Verified, admin)
end
end
Contract-->>Admin: Ok(())
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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: 3
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/lib.rs`:
- Around line 32-34: Replace the panic caused by unwrap() when fetching the
admin in batch_add_experts with a proper contract error: call
storage::get_admin(&env) and if it returns None return
Err(ContractError::NotInitialized) (or the crate's NotInitialized variant)
instead of unwrapping; then call admin.require_auth() on the Some(admin) value.
Update any function signature/return types for batch_add_experts if needed to
propagate the ContractError/Result so callers receive NotInitialized rather than
panicking. Ensure references: storage::get_admin, admin.require_auth,
batch_add_experts, init, and NotInitialized.
In `@contracts/identity-registry-contract/src/test.rs`:
- Around line 24-38: Update the test_batch_verification test to meet the
acceptance criteria by creating five expert addresses instead of three, calling
client.batch_add_experts with that five-entry vector, and adding assertions
after the call that verify each address has the expected expert status (use the
contract read/query method that returns expert/verified status for an Address,
e.g., client.is_expert or the relevant getter) and any expected overall batch
result; keep the same setup (Env::default, env.mock_all_auths,
register_contract, IdentityRegistryContractClient::new, client.init(&admin)) but
replace the experts vec to contain five Address::generate(&env) entries and add
per-address assertions checking their verification/status via the contract
client methods.
- Around line 40-52: The test currently panics due to unwrap on get_admin() and
bypasses auth because env.mock_all_auths() is used; remove env.mock_all_auths(),
create an admin address and call client.init(&admin) (using the admin as the
signer) so the contract has an admin, then call client.batch_add_experts(...)
without authenticating as that admin so the panic comes from the require_auth()
check; reference the test name test_batch_verification_no_admin and the methods
client.init, client.batch_add_experts, env.mock_all_auths, get_admin, and
require_auth to locate and fix the code.
🧹 Nitpick comments (1)
contracts/identity-registry-contract/src/test.rs (1)
84-110: Consider asserting the error viatry_*instead ofshould_panic.
The panic string can be brittle across soroban-sdk versions; explicit error checking is more stable.♻️ Possible adjustment (verify client error behavior)
-#[test] -#[should_panic(expected = "Error(Contract, `#7`)")] +#[test] fn test_batch_verification_max_vec() { let env = Env::default(); env.mock_all_auths(); @@ - client.batch_add_experts(&experts); + let res = client.try_batch_add_experts(&experts); + assert_eq!(res, Err(RegistryError::ExpertVecMax)); }
| #[test] | ||
| fn test_batch_verification() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let contract_id = env.register_contract(None, IdentityRegistryContract); | ||
| let client = IdentityRegistryContractClient::new(&env, &contract_id); | ||
|
|
||
| let admin = soroban_sdk::Address::generate(&env); | ||
| client.init(&admin); | ||
|
|
||
| let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)]; | ||
|
|
||
| client.batch_add_experts(&experts); | ||
| } |
There was a problem hiding this comment.
Align this test with the acceptance criteria (5 experts + status assertions).
Currently it uses 3 experts and doesn’t assert verification. Consider either updating this test or renaming/using test_batch_verification_check_status as the required “test_batch_add”.
🧩 Possible update (one option)
-#[test]
-fn test_batch_verification() {
+#[test]
+fn test_batch_add() {
let env = Env::default();
env.mock_all_auths();
@@
- let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)];
+ let e1 = Address::generate(&env);
+ let e2 = Address::generate(&env);
+ let e3 = Address::generate(&env);
+ let e4 = Address::generate(&env);
+ let e5 = Address::generate(&env);
+ let experts = vec![&env, e1.clone(), e2.clone(), e3.clone(), e4.clone(), e5.clone()];
client.batch_add_experts(&experts);
+
+ env.as_contract(&contract_id, ||{
+ assert_eq!(storage::get_expert_status(&env, &e1), ExpertStatus::Verified);
+ assert_eq!(storage::get_expert_status(&env, &e2), ExpertStatus::Verified);
+ assert_eq!(storage::get_expert_status(&env, &e3), ExpertStatus::Verified);
+ assert_eq!(storage::get_expert_status(&env, &e4), ExpertStatus::Verified);
+ assert_eq!(storage::get_expert_status(&env, &e5), ExpertStatus::Verified);
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_batch_verification() { | |
| let env = Env::default(); | |
| env.mock_all_auths(); | |
| let contract_id = env.register_contract(None, IdentityRegistryContract); | |
| let client = IdentityRegistryContractClient::new(&env, &contract_id); | |
| let admin = soroban_sdk::Address::generate(&env); | |
| client.init(&admin); | |
| let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)]; | |
| client.batch_add_experts(&experts); | |
| } | |
| #[test] | |
| fn test_batch_add() { | |
| let env = Env::default(); | |
| env.mock_all_auths(); | |
| let contract_id = env.register_contract(None, IdentityRegistryContract); | |
| let client = IdentityRegistryContractClient::new(&env, &contract_id); | |
| let admin = soroban_sdk::Address::generate(&env); | |
| client.init(&admin); | |
| let e1 = Address::generate(&env); | |
| let e2 = Address::generate(&env); | |
| let e3 = Address::generate(&env); | |
| let e4 = Address::generate(&env); | |
| let e5 = Address::generate(&env); | |
| let experts = vec![&env, e1.clone(), e2.clone(), e3.clone(), e4.clone(), e5.clone()]; | |
| client.batch_add_experts(&experts); | |
| env.as_contract(&contract_id, ||{ | |
| assert_eq!(storage::get_expert_status(&env, &e1), ExpertStatus::Verified); | |
| assert_eq!(storage::get_expert_status(&env, &e2), ExpertStatus::Verified); | |
| assert_eq!(storage::get_expert_status(&env, &e3), ExpertStatus::Verified); | |
| assert_eq!(storage::get_expert_status(&env, &e4), ExpertStatus::Verified); | |
| assert_eq!(storage::get_expert_status(&env, &e5), ExpertStatus::Verified); | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/test.rs` around lines 24 - 38,
Update the test_batch_verification test to meet the acceptance criteria by
creating five expert addresses instead of three, calling
client.batch_add_experts with that five-entry vector, and adding assertions
after the call that verify each address has the expected expert status (use the
contract read/query method that returns expert/verified status for an Address,
e.g., client.is_expert or the relevant getter) and any expected overall batch
result; keep the same setup (Env::default, env.mock_all_auths,
register_contract, IdentityRegistryContractClient::new, client.init(&admin)) but
replace the experts vec to contain five Address::generate(&env) entries and add
per-address assertions checking their verification/status via the contract
client methods.
| #[test] | ||
| #[should_panic] | ||
| fn test_batch_verification_no_admin() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let contract_id = env.register_contract(None, IdentityRegistryContract); | ||
| let client = IdentityRegistryContractClient::new(&env, &contract_id); | ||
|
|
||
| let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)]; | ||
|
|
||
| client.batch_add_experts(&experts); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files contracts/identity-registry-contract/src/ | head -20Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 431
🏁 Script executed:
find contracts -name "test.rs" -o -name "lib.rs" | head -10Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 264
🏁 Script executed:
find . -type f -name "*.rs" | grep -E "(test|contract)" | head -20Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 687
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/test.rs | head -80Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 3395
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/contract.rs | head -100Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 487
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/contract.rs | grep -A 30 "batch_add_experts"Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/contract.rs | grep -A 30 "pub fn batch"Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
wc -l contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 134
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 487
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/lib.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1679
This test doesn't actually verify admin auth.
The panic occurs from the .unwrap() on line 32 of batch_add_experts() (when get_admin() returns None due to missing initialization), not from auth enforcement. Additionally, env.mock_all_auths() bypasses the require_auth() check entirely, so the test would never catch an actual auth failure.
Remove env.mock_all_auths(), initialize the admin with client.init(&admin), and let the test fail on the proper auth check:
Suggested fix
#[test]
#[should_panic]
fn test_batch_verification_no_admin() {
let env = Env::default();
- env.mock_all_auths();
let contract_id = env.register_contract(None, IdentityRegistryContract);
let client = IdentityRegistryContractClient::new(&env, &contract_id);
+ let admin = Address::generate(&env);
+ client.init(&admin);
+
let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)];
client.batch_add_experts(&experts);
}🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/test.rs` around lines 40 - 52, The
test currently panics due to unwrap on get_admin() and bypasses auth because
env.mock_all_auths() is used; remove env.mock_all_auths(), create an admin
address and call client.init(&admin) (using the admin as the signer) so the
contract has an admin, then call client.batch_add_experts(...) without
authenticating as that admin so the panic comes from the require_auth() check;
reference the test name test_batch_verification_no_admin and the methods
client.init, client.batch_add_experts, env.mock_all_auths, get_admin, and
require_auth to locate and fix the code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/lib.rs`:
- Around line 35-42: Loop fetching each expert's current status via
storage::get_expert_status stores it in status but the event emission hardcodes
ExpertStatus::Unverified; update the call to events::emit_status_change to pass
the previously read status variable (status) as the "from" state instead of
ExpertStatus::Unverified so the event reflects the actual previous state; keep
storage::set_expert_record(&env, &expert, ExpertStatus::Verified) and the rest
of the call parameters (expert, ExpertStatus::Verified, admin.clone())
unchanged.
♻️ Duplicate comments (1)
contracts/identity-registry-contract/src/test.rs (1)
40-51: Test name and purpose mismatch: tests initialization, not authorization.This test verifies that calling
batch_add_expertson an uninitialized contract fails (returningNotInitialized), not that an unauthorized caller is rejected. Consider either:
- Renaming to
test_batch_verification_not_initialized, or- Adding a separate test that initializes the contract, then calls
batch_add_expertswithout mocking auth for the admin.🧪 Add a test for actual admin authorization
#[test] #[should_panic] fn test_batch_verification_unauthorized() { let env = Env::default(); // Do NOT mock auth - this is intentional let contract_id = env.register_contract(None, IdentityRegistryContract); let client = IdentityRegistryContractClient::new(&env, &contract_id); let admin = Address::generate(&env); // Mock auth only for init env.mock_all_auths(); client.init(&admin); // Clear auth mocks so batch_add_experts fails on require_auth() // Note: In Soroban, you may need to create a new Env or use specific auth testing utilities let experts = vec![&env, Address::generate(&env)]; // This should panic due to missing admin authorization client.batch_add_experts(&experts); }
🧹 Nitpick comments (1)
contracts/identity-registry-contract/src/lib.rs (1)
11-11: Remove unused import.The
core::ops::Addimport is not used anywhere in this file.🧹 Suggested fix
-use core::ops::Add; -
| for expert in experts { | ||
| let status = storage::get_expert_status(&env, &expert); | ||
| if status == ExpertStatus::Verified { | ||
| return Err(RegistryError::AlreadyVerified); | ||
| } | ||
| storage::set_expert_record(&env, &expert, ExpertStatus::Verified); | ||
| events::emit_status_change(&env, expert, ExpertStatus::Unverified, ExpertStatus::Verified, admin.clone()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ExpertStatus enum definition
ast-grep --pattern 'enum ExpertStatus {
$$$
}'Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Also search for ExpertStatus definition more broadly
rg -A 10 'enum ExpertStatus' contracts/identity-registry-contract/src/Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 847
🏁 Script executed:
#!/bin/bash
# Check the file structure and understand the broader context
head -100 contracts/identity-registry-contract/src/lib.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1386
Use the actual status variable for event emission, as ExpertStatus has three variants.
The code retrieves the expert's status on line 36 but hardcodes ExpertStatus::Unverified in the event on line 41. Since ExpertStatus has three variants—Unverified, Verified, and Banned—the event will incorrectly report Unverified → Verified when a Banned expert is re-verified. Replace the hardcoded value with the status variable to accurately reflect the previous state:
for expert in experts {
let status = storage::get_expert_status(&env, &expert);
if status == ExpertStatus::Verified {
return Err(RegistryError::AlreadyVerified);
}
storage::set_expert_record(&env, &expert, ExpertStatus::Verified);
- events::emit_status_change(&env, expert, ExpertStatus::Unverified, ExpertStatus::Verified, admin.clone());
+ events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone());🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/lib.rs` around lines 35 - 42, Loop
fetching each expert's current status via storage::get_expert_status stores it
in status but the event emission hardcodes ExpertStatus::Unverified; update the
call to events::emit_status_change to pass the previously read status variable
(status) as the "from" state instead of ExpertStatus::Unverified so the event
reflects the actual previous state; keep storage::set_expert_record(&env,
&expert, ExpertStatus::Verified) and the rest of the call parameters (expert,
ExpertStatus::Verified, admin.clone()) unchanged.
There was a problem hiding this comment.
please look into this
There was a problem hiding this comment.
@Bosun-Josh121, I've reviewed the code again and can confirm the issue. Here's the problem:
Current behavior: The event always reports Unverified → Verified, regardless of the expert's actual previous state.
Concrete example:
- If an expert is currently
Bannedand an admin callsbatch_add_expertsto verify them:- Line 36 retrieves
status = ExpertStatus::Banned - Line 37 check passes (since
Banned != Verified) - Line 40 sets the expert to
Verified - Line 41 emits event:
Unverified → Verified❌ (should beBanned → Verified)
- Line 36 retrieves
Why this matters:
Events provide an audit trail for state changes. Inaccurate events can break off-chain indexers, analytics, and compliance tracking that rely on knowing the actual state transitions.
The fix:
Replace the hardcoded ExpertStatus::Unverified with the status variable that's already retrieved on line 36:
events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone());This ensures the event accurately reflects the actual state transition, whether it's Unverified → Verified or Banned → Verified.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all calls to emit_status_change to check for similar issues
rg -n 'emit_status_change' contracts/identity-registry-contract/
Length of output: 250
🏁 Script executed:
#!/bin/bash
# Look for the verify_expert or similar single-expert verification logic to compare
rg -nP 'fn\s+(verify_expert|add_expert|ban_expert)' contracts/identity-registry-contract/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the emit_status_change function definition to understand its signature
ast-grep --pattern 'fn emit_status_change($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the test file to see what's being tested for batch_add_experts
cat contracts/identity-registry-contract/src/test.rs
Length of output: 3688
🏁 Script executed:
#!/bin/bash
# Search for any test that checks event emissions or status changes
rg -n 'status_change|emit|event' contracts/identity-registry-contract/src/test.rs
Length of output: 0
@Bosun-Josh121, I've conducted a thorough investigation of the codebase. Here's what I found:
Evidence from the Codebase
1. Event Function Signature (events.rs:15-21):
pub fn emit_status_change(
env: &Env,
expert: Address,
old_status: ExpertStatus, // ← Parameter expects the ACTUAL old status
new_status: ExpertStatus,
admin: Address
)2. Current Implementation Issue (lib.rs:35-41):
for expert in experts {
let status = storage::get_expert_status(&env, &expert); // ← Retrieved but not used
if status == ExpertStatus::Verified {
return Err(RegistryError::AlreadyVerified);
}
storage::set_expert_record(&env, &expert, ExpertStatus::Verified);
events::emit_status_change(&env, expert, ExpertStatus::Unverified, /* ← HARDCODED */ ExpertStatus::Verified, admin.clone());
}3. Why Tests Didn't Catch This:
The test file only verifies final status, not event accuracy:
assert_eq!(storage::get_expert_status(&env, &e1), ExpertStatus::Verified);No tests check event emissions or handle the Banned → Verified scenario.
The Problem
When verifying a Banned expert:
- ✅ Line 36: Retrieves
status = ExpertStatus::Banned - ✅ Line 37: Check passes (Banned ≠ Verified)
- ✅ Line 40: Sets expert to Verified
- ❌ Line 41: Emits event as
Unverified → Verified(should beBanned → Verified)
This breaks audit trails and off-chain indexers that rely on accurate state transitions.
The Fix
Replace line 41:
events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone());Would you like me to implement this fix for you? I can update the code and add a test case for the Banned → Verified scenario.
🧬 Code Graph Analysis Results
File: contracts/identity-registry-contract/src/contract.rs
- Relevant snippet (lines 5-13):
pub fn initialize_registry(env: &Env, admin: &Address) -> Result<(), RegistryError> {
if storage::has_admin(env) {
return Err(RegistryError::AlreadyInitialized);
}
storage::set_admin(env, admin);
Ok(())
}
File: contracts/identity-registry-contract/src/events.rs
- Relevant snippet (lines 15-31):
pub fn emit_status_change(
env: &Env,
expert: Address,
old_status: ExpertStatus,
new_status: ExpertStatus,
admin: Address
) {
let event = ExpertStatusChangedEvent {
expert,
old_status,
new_status,
admin,
};
// We publish with the topic "status_change" so indexers can find it easily
env.events().publish((Symbol::new(env, "status_change"),), event);
}
File: contracts/identity-registry-contract/src/storage.rs
- Relevant snippet (lines 34-36):
pub fn get_admin(env: &Env) -> Option<Address> {
env.storage().instance().get(&DataKey::Admin)
}
- Relevant snippet (lines 40-59):
pub fn set_expert_record(env: &Env, expert: &Address, status: ExpertStatus) {
let key = DataKey::Expert(expert.clone());
let record = ExpertRecord {
status,
updated_at: env.ledger().timestamp(),
};
// 1. Save the data
env.storage().persistent().set(&key, &record);
// 2. Extend the TTL
// This tells the network: "If this data is going to die in less than 2 months,
// extend its life to 1 full year from now."
env.storage().persistent().extend_ttl(
&key,
LEDGERS_THRESHOLD,
LEDGERS_EXTEND_TO
);
}
- Relevant snippet (lines 83-85):
pub fn get_expert_status(env: &Env, expert: &Address) -> ExpertStatus {
get_expert_record(env, expert).status
}
File: contracts/identity-registry-contract/src/identity-registry-contract/src/lib.rs (already shown in file; no extra snippet)
|
Put function logic in contract.rs and use in lib.rs then resolve conflicts. Thanks @hakymulla |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 17-34: The function batch_add_experts currently lacks a final
Ok(()) return which prevents it from compiling; update the function
batch_add_experts to return Ok(()) at the end (after the for loop) so the
signature Result<(), RegistryError> is satisfied, leaving all existing early
Err(...) returns and side-effects (storage::set_expert_record,
events::emit_status_change, admin.require_auth, etc.) unchanged.
- Around line 1-5: Remove the duplicate import of ExpertStatus (the second "use
crate::types::ExpertStatus;") and drop the unused soroban_sdk items that belong
in lib.rs: remove "contract", "contractimpl", and "Vec" from the soroban_sdk
import list; keep only the actually used imports such as Address and Env and the
crate items like storage, events, error::RegistryError and types::ExpertStatus
so the file compiles cleanly.
In `@contracts/identity-registry-contract/src/test.rs`:
- Line 99: The function test_add_expert is missing the #[test] attribute so it
won't be executed by cargo test; add the #[test] attribute immediately above the
fn test_add_expert() definition (placing it on the line before the fn) to
register it as a unit test so the test runner picks it up.
♻️ Duplicate comments (1)
contracts/identity-registry-contract/src/test.rs (1)
28-39: Test panics fromNotInitialized, not from auth failure.This test will panic because
client.init(&admin)is never called, soget_admin()returnsNoneand the function returnsNotInitializederror. It does not test therequire_auth()check as the test name suggests.To properly test admin authorization failure, initialize the contract but don't mock auth:
🛠️ Suggested fix to test actual auth failure
#[test] #[should_panic] fn test_batch_verification_no_admin() { let env = Env::default(); - + // Don't call env.mock_all_auths() to test real auth failure + let contract_id = env.register_contract(None, IdentityRegistryContract); let client = IdentityRegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + client.init(&admin); + let experts = vec![&env, Address::generate(&env), Address::generate(&env), Address::generate(&env)]; + // Should panic on require_auth() failure, not NotInitialized client.batch_add_experts(&experts); }
🧹 Nitpick comments (3)
contracts/identity-registry-contract/src/contract.rs (1)
26-33:AlreadyVerifiederror aborts the entire batch on any duplicate.If one expert in the middle of the vector is already verified, the function returns an error and none of the subsequent experts are processed. This may be intentional, but it differs from typical batch semantics where partial success or skip-and-continue is expected.
Consider whether:
- Skipping already-verified experts silently (continue instead of error) would be more user-friendly
- The current strict behavior is desired (document this clearly)
♻️ Alternative: skip already-verified experts
for expert in experts { let status = storage::get_expert_status(&env, &expert); if status == ExpertStatus::Verified { - return Err(RegistryError::AlreadyVerified); + continue; // Skip already verified experts } storage::set_expert_record(&env, &expert, ExpertStatus::Verified); events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone()); }contracts/identity-registry-contract/src/lib.rs (1)
24-27: Inconsistent indentation in doc comment.Line 24 uses 2-space indentation while line 25 uses 4-space indentation. This is a minor formatting inconsistency.
♻️ Suggested fix
- /// Batch Add an expert to the whitelist (Admin only) + /// Batch Add experts to the whitelist (Admin only) pub fn batch_add_experts(env: Env, experts: Vec<Address>) -> Result<(), RegistryError> { contract::batch_add_experts(env, experts) }contracts/identity-registry-contract/src/test.rs (1)
71-97: Test may fail withAlreadyVerifiedbefore reachingExpertVecMaxcheck.The vector contains 24 elements but reuses only 4 unique addresses (
e1-e4). When the batch is processed, after the first 4 experts are verified, the 5th element (e1.clone()) will triggerAlreadyVerifiederror before the vector length check matters.Since the length check (>20) happens before iteration in
batch_add_experts, this test should work. However, the test would be clearer and more robust if it used 21+ unique addresses.♻️ Suggested fix using unique addresses
#[test] #[should_panic(expected = "Error(Contract, `#7`)")] fn test_batch_verification_max_vec() { let env = Env::default(); env.mock_all_auths(); let contract_id = env.register_contract(None, IdentityRegistryContract); let client = IdentityRegistryContractClient::new(&env, &contract_id); let admin = soroban_sdk::Address::generate(&env); client.init(&admin); - let e1 = Address::generate(&env); - let e2 = Address::generate(&env); - let e3 = Address::generate(&env); - let e4 = Address::generate(&env); - - let experts = vec![&env, e1.clone(), e2.clone(), e3.clone(), e4.clone(), - e1.clone(), e2.clone(), e3.clone(), e4.clone(), - e1.clone(), e2.clone(), e3.clone(), e4.clone(), - e1.clone(), e2.clone(), e3.clone(), e4.clone(), - e1.clone(), e2.clone(), e3.clone(), e4.clone(), - e1.clone(), e2.clone(), e3.clone(), e4.clone() - ]; + // Generate 21 unique addresses to exceed the limit of 20 + let mut experts = Vec::new(&env); + for _ in 0..21 { + experts.push_back(Address::generate(&env)); + } client.batch_add_experts(&experts); }
| use soroban_sdk::{contract, contractimpl, Address, Env, Vec}; | ||
| use crate::storage; | ||
| use crate::error::RegistryError; | ||
| use crate::types::ExpertStatus; | ||
| use crate::events; | ||
| use crate::{error::RegistryError, types::ExpertStatus}; | ||
| use crate::types::ExpertStatus; |
There was a problem hiding this comment.
Duplicate import and unused imports will cause compilation errors.
Line 4 imports ExpertStatus via crate::types::ExpertStatus, and line 5 imports it again from crate::types::ExpertStatus. This is a duplicate import that will fail to compile.
Additionally, contract, contractimpl, and Vec from soroban_sdk are imported but not used in this file (they belong in lib.rs).
🛠️ Proposed fix
-use soroban_sdk::{contract, contractimpl, Address, Env, Vec};
+use soroban_sdk::{Address, Env, Vec};
use crate::storage;
use crate::events;
-use crate::{error::RegistryError, types::ExpertStatus};
-use crate::types::ExpertStatus;
+use crate::error::RegistryError;
+use crate::types::ExpertStatus;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use soroban_sdk::{contract, contractimpl, Address, Env, Vec}; | |
| use crate::storage; | |
| use crate::error::RegistryError; | |
| use crate::types::ExpertStatus; | |
| use crate::events; | |
| use crate::{error::RegistryError, types::ExpertStatus}; | |
| use crate::types::ExpertStatus; | |
| use soroban_sdk::{Address, Env, Vec}; | |
| use crate::storage; | |
| use crate::events; | |
| use crate::error::RegistryError; | |
| use crate::types::ExpertStatus; |
🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/contract.rs` around lines 1 - 5,
Remove the duplicate import of ExpertStatus (the second "use
crate::types::ExpertStatus;") and drop the unused soroban_sdk items that belong
in lib.rs: remove "contract", "contractimpl", and "Vec" from the soroban_sdk
import list; keep only the actually used imports such as Address and Env and the
crate items like storage, events, error::RegistryError and types::ExpertStatus
so the file compiles cleanly.
2 participants
🧠 SkillSphere Pull Request 🌐
Mark with an
xall the checkboxes that apply (like[x])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
✏️ Tip: You can customize this high-level summary in your review settings.