[Feat]:: Implemented "Ban Expert" Logic #10
Conversation
📝 WalkthroughWalkthroughAdds admin-managed registry initialization, admin-authorized expert verification and banning, batch expert addition, a public status getter, admin storage helpers (with TTL touches), status-change events, and expanded tests covering verification and banning lifecycles. Changes
Sequence Diagram(s)mermaid Admin->>Contract: initialize_registry(admin) / ban_expert(expert) / batch_add_experts(...) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 1
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 41-62: In verify_expert, add a guard to prevent re-verifying a
banned expert: fetch current_status via storage::get_expert_status(env, expert)
(same as ban_expert uses) and if current_status == ExpertStatus::Banned return
Err(RegistryError::AlreadyBanned) before the existing AlreadyVerified check;
this enforces that ban revokes verification (alternatively implement a separate
unban_expert with explicit authorization if unbanning is allowed).
🧹 Nitpick comments (3)
contracts/identity-registry-contract/src/lib.rs (1)
35-37: Consider adding a doc comment for API consistency.The other public methods (
init,add_expert,ban_expert) have doc comments, butget_statusdoes not. Adding one would maintain consistency in the public API documentation.📝 Suggested doc comment
+ /// Get the current status of an expert pub fn get_status(env: Env, expert: Address) -> ExpertStatus { contract::get_expert_status(&env, &expert) }contracts/identity-registry-contract/src/test.rs (2)
99-130: Consider adding explicit authorization verification forban_expert.The test validates the ban flow and error handling, but unlike
test_add_expert(lines 45-58), it doesn't explicitly verify thatban_expertrequires admin authorization viaAuthorizedInvocationcheck. Additionally, per PR objectives,ban_expertshould emit anExpertStatusChangedevent, but this isn't verified.♻️ Suggested additions to enhance test coverage
// Ban the expert (should succeed) client.ban_expert(&expert); + // Verify admin authorization was required + assert_eq!( + env.auths().last().unwrap(), + ( + admin.clone(), + AuthorizedInvocation { + function: AuthorizedFunction::Contract(( + contract_id.clone(), + Symbol::new(&env, "ban_expert"), + (expert.clone(),).into_val(&env) + )), + sub_invocations: std::vec![] + } + ) + ); + + // Verify ExpertStatusChanged event was emitted + let events = env.events().all(); + let event = events.last().unwrap(); + assert_eq!(event.0, contract_id); + let topic: Symbol = event.1.get(0).unwrap().try_into_val(&env).unwrap(); + assert_eq!(topic, Symbol::new(&env, "status_change")); + // Check that status is now Banned let status = client.get_status(&expert);
144-152: Simplify the authorization mocking setup.The sequence of
mock_all_auths_allowing_non_root_auth()followed bymock_auths(&[])is confusing. A simpler pattern, consistent withtest_add_expert_unauthorized, would be to avoid any auth mocking before callingban_expert:♻️ Simplified version
client.init(&admin); env.mock_all_auths(); client.add_expert(&expert); - env.mock_all_auths_allowing_non_root_auth(); - - env.mock_auths(&[]); + // Clear auth mocking - subsequent calls will require real authorization + env.mock_auths(&[]); client.ban_expert(&expert); }Alternatively, for maximum clarity and consistency with
test_add_expert_unauthorized, you could use a scoped approach where auth mocking only applies to theadd_expertcall.
| pub fn ban_expert(env: &Env, expert: &Address) -> Result<(), RegistryError> { | ||
| let admin = storage::get_admin(env).ok_or(RegistryError::NotInitialized)?; | ||
| admin.require_auth(); | ||
|
|
||
| let current_status = storage::get_expert_status(env, expert); | ||
|
|
||
| if current_status == ExpertStatus::Banned { | ||
| return Err(RegistryError::AlreadyBanned); | ||
| } | ||
|
|
||
| storage::set_expert_record(env, expert, ExpertStatus::Banned); | ||
|
|
||
| events::emit_status_change( | ||
| env, | ||
| expert.clone(), | ||
| current_status, | ||
| ExpertStatus::Banned, | ||
| admin, | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the verify_expert function implementation
cat -n contracts/identity-registry-contract/src/contract.rs | sed -n '1,80p'Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 2208
🏁 Script executed:
# Search for test files related to ban and verify operations
fd -e rs -path "*/test*" | head -20Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 247
🏁 Script executed:
# Look for any tests that mention ban_expert or the interaction between ban and verify
rg "ban_expert|Banned.*Verified|verify.*ban" -B 3 -A 3 --type rustRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 7289
🏁 Script executed:
# Check if there's a verify_expert function and see its implementation
ast-grep --pattern 'fn verify_expert($_) { $$$ }'Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
# Look for any comments, TODOs, or documentation about expert status handling
rg "TODO|FIXME|NOTE|preemptive|unbann" --type rust -iRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
# Search more thoroughly for tests that might cover re-verification of banned experts
cat -n contracts/identity-registry-contract/src/test.rs | head -300Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 9485
🏁 Script executed:
# Look for the complete test_ban_expert_workflow test
rg "test_ban_expert_workflow" -A 50 -B 5Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 4640
🏁 Script executed:
# Check ExpertStatus enum to understand all possible states
rg "enum ExpertStatus" -A 10Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 847
🏁 Script executed:
# Look for any documentation or comments about the intended behavior of ban/verify
rg "Ban|Verified|expert" contracts/identity-registry-contract/src/lib.rs -B 2 -A 2Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 600
🏁 Script executed:
# Check if there's any test for re-verifying a banned expert (verify after ban)
rg "ban_expert.*verify_expert|verify_expert.*ban_expert" -B 5 -A 5Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
# Search more comprehensively for any mention of re-verification after ban
rg "Verified.*Banned|Banned.*Verified" -B 3 -A 3Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
# Check if there's documentation or requirements about the design
find . -name "README*" -o -name "REQUIREMENTS*" -o -name "*.md" | head -10Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 125
🏁 Script executed:
# Look for any TODOs or FIXMEs related to ban/verify logic
rg "TODO|FIXME" contracts/identity-registry-contract/ -B 2 -A 2Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
# Check the README for design documentation or requirements
cat README.mdRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 8315
🏁 Script executed:
# Look specifically for the PR template to understand requirements
cat ./.github/pull_request_template.mdRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1480
🏁 Script executed:
# Check if there's any test attempting to re-verify a banned expert
rg "ban_expert" -A 20 contracts/identity-registry-contract/src/test.rs | rg -A 15 "verify_expert|add_expert"Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 1605
🏁 Script executed:
# Look at complete_expert_lifecycle test to confirm it doesn't test re-verification after ban
cat -n contracts/identity-registry-contract/src/test.rs | sed -n '238,261p'Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 971
Add a check in verify_expert to prevent re-verifying banned experts.
The implementation currently allows re-verifying a Banned expert back to Verified status. The verify_expert function only guards against AlreadyVerified but does not check Banned status. This conflicts with the documented semantics that banning "revokes" verification. Add: if current_status == ExpertStatus::Banned { return Err(RegistryError::AlreadyBanned); } before the existing check, or introduce an explicit unban_expert function if unbanning should require separate authorization.
🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/contract.rs` around lines 41 - 62,
In verify_expert, add a guard to prevent re-verifying a banned expert: fetch
current_status via storage::get_expert_status(env, expert) (same as ban_expert
uses) and if current_status == ExpertStatus::Banned return
Err(RegistryError::AlreadyBanned) before the existing AlreadyVerified check;
this enforces that ban revokes verification (alternatively implement a separate
unban_expert with explicit authorization if unbanning is allowed).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/storage.rs`:
- Around line 7-9: The admin record stored under DataKey::Admin currently never
extends its instance TTL, so modify has_admin, set_admin, and get_admin to call
the same extend_ttl() used for expert records: after successfully setting the
admin in set_admin call extend_ttl() to bump TTL; in get_admin and has_admin, if
the admin is found return the value and call extend_ttl() to refresh TTL (mirror
the expert get/set pattern). Locate DataKey::Admin and the functions has_admin,
set_admin, and get_admin and add the extend_ttl() invocation in the same spots
and order as the expert record code to ensure consistent TTL behavior.
♻️ Duplicate comments (1)
contracts/identity-registry-contract/src/contract.rs (1)
18-29: Block re‑verification of banned experts.
verify_expertonly guardsVerified, so aBannedexpert can be re‑verified, undermining the ban semantics. Add an explicitBannedcheck before the existing guard.🐛 Suggested guard in
verify_expertlet current_status = storage::get_expert_status(env, expert); + if current_status == ExpertStatus::Banned { + return Err(RegistryError::AlreadyBanned); + } if current_status == ExpertStatus::Verified { return Err(RegistryError::AlreadyVerified); }
| pub enum DataKey { | ||
| Admin, | ||
| Expert(Address), | ||
| Admin, | ||
| Expert(Address), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/storage.rs | head -50Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 214
🏁 Script executed:
cat contracts/identity-registry-contract/src/storage.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 2568
Extend admin instance TTL to prevent silent expiration.
Admin is stored in instance storage without TTL extension. Unlike expert records which correctly call extend_ttl() on both set and get operations, admin functions (has_admin, set_admin, get_admin) never bump the TTL. Once the admin expires, has_admin() silently returns false and the registry becomes unmanaged or can be re-initialized. Apply the same TTL extension pattern used for expert records:
TTL extension for admin storage
pub fn has_admin(env: &Env) -> bool {
- env.storage().instance().has(&DataKey::Admin)
+ let has = env.storage().instance().has(&DataKey::Admin);
+ if has {
+ env.storage().instance().extend_ttl(LEDGERS_THRESHOLD, LEDGERS_EXTEND_TO);
+ }
+ has
}
pub fn set_admin(env: &Env, admin: &Address) {
env.storage().instance().set(&DataKey::Admin, admin);
+ env.storage().instance().extend_ttl(LEDGERS_THRESHOLD, LEDGERS_EXTEND_TO);
}
pub fn get_admin(env: &Env) -> Option<Address> {
- env.storage().instance().get(&DataKey::Admin)
+ let admin = env.storage().instance().get(&DataKey::Admin);
+ if admin.is_some() {
+ env.storage().instance().extend_ttl(LEDGERS_THRESHOLD, LEDGERS_EXTEND_TO);
+ }
+ admin
}🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/storage.rs` around lines 7 - 9, The
admin record stored under DataKey::Admin currently never extends its instance
TTL, so modify has_admin, set_admin, and get_admin to call the same extend_ttl()
used for expert records: after successfully setting the admin in set_admin call
extend_ttl() to bump TTL; in get_admin and has_admin, if the admin is found
return the value and call extend_ttl() to refresh TTL (mirror the expert get/set
pattern). Locate DataKey::Admin and the functions has_admin, set_admin, and
get_admin and add the extend_ttl() invocation in the same spots and order as the
expert record code to ensure consistent TTL behavior.
|
Hello !!! can you please give me compliments !! |
|
kindly fix the conflicts then ready for merge! @ryzen-xp |
|
Hello @Bosun-Josh121 check it |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 1-5: The file imports ExpertStatus twice causing a duplicate
import error (E0252); remove the redundant use of ExpertStatus so only a single
import remains (keep the canonical import from crate::types::ExpertStatus and
delete the duplicate in the grouped crate import), ensuring functions/types that
reference ExpertStatus still compile and no other use statements are
accidentally removed.
In `@contracts/identity-registry-contract/src/test.rs`:
- Around line 5-8: The test import block contains a duplicate import of
ExpertStatus which causes E0252; remove the redundant import so ExpertStatus is
imported only once (for example keep crate::types::ExpertStatus and delete the
duplicate in the crate::{storage, types::ExpertStatus} or vice versa) and ensure
the remaining imports still include IdentityRegistryContract,
IdentityRegistryContractClient and storage so types and clients resolve
correctly.
♻️ Duplicate comments (1)
contracts/identity-registry-contract/src/contract.rs (1)
40-59: Prevent re-verifying banned experts (ban should be terminal unless explicitly unbanned).
verify_expertstill allows aBannedexpert to be set back toVerified, which undermines the “ban revokes verification” intent. Consider blockingBannedhere (and/or introduce a dedicated unban flow).🔧 Proposed fix
let current_status = storage::get_expert_status(env, expert); + if current_status == ExpertStatus::Banned { + return Err(RegistryError::AlreadyBanned); + } if current_status == ExpertStatus::Verified { return Err(RegistryError::AlreadyVerified); }
|
Hello @ryzen-xp, please makes sure there's no build errors |
|
Hello @Bosun-Josh121 can you recheck |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/identity-registry-contract/src/contract.rs (2)
17-37: Block re-verification of banned experts in batch flow.
batch_add_expertswill currently re-verifyBannedexperts. If ban is meant to be revocation, this should returnAlreadyBannedbefore setting status toVerified.✅ Proposed fix
for expert in experts { let status = storage::get_expert_status(&env, &expert); + if status == ExpertStatus::Banned { + return Err(RegistryError::AlreadyBanned); + } if status == ExpertStatus::Verified { return Err(RegistryError::AlreadyVerified); } storage::set_expert_record(&env, &expert, ExpertStatus::Verified); events::emit_status_change(&env, expert, status, ExpertStatus::Verified, admin.clone()); }
6-15: Addadmin.require_auth()to prevent admin squatting during initialization.Any caller can invoke
initialize_registryand designate themselves as the admin before the intended admin does. This is a critical vulnerability since the admin role controls all expert verification and banning. All other admin functions (batch_add_experts,verify_expert,ban_expert) properly calladmin.require_auth(); initialization must do the same. Note: the same vulnerability exists inpayment-vault-contract/src/contract.rsin theinitialize_vaultfunction.🔒 Proposed fix
pub fn initialize_registry(env: &Env, admin: &Address) -> Result<(), RegistryError> { if storage::has_admin(env) { return Err(RegistryError::AlreadyInitialized); } + admin.require_auth(); storage::set_admin(env, admin); Ok(()) }
🤖 Fix all issues with AI agents
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 17-19: Update the function doc comment to accurately describe
batch_add_experts: replace the singular "Verify an expert" line with a
description indicating this function batch-verifies multiple experts
(admin-only), e.g., note that batch_add_experts takes Env and Vec<Address> and
sets multiple experts' status to Verified, returning Result<(), RegistryError>;
keep mention of admin-only and Batch Verification.
♻️ Duplicate comments (1)
contracts/identity-registry-contract/src/contract.rs (1)
39-61: Prevent re-verifying banned experts (already flagged).Same concern as previous review: add a
Bannedguard before theAlreadyVerifiedcheck.
| /// Verify an expert by setting their status to Verified (Admin only) | ||
| /// Batch Verification | ||
| pub fn batch_add_experts(env:Env, experts: Vec<Address>) -> Result<(), RegistryError> { |
There was a problem hiding this comment.
Fix the batch function doc comment.
The doc comment says “Verify an expert” but this function batch-verifies. Consider updating for accuracy.
🤖 Prompt for AI Agents
In `@contracts/identity-registry-contract/src/contract.rs` around lines 17 - 19,
Update the function doc comment to accurately describe batch_add_experts:
replace the singular "Verify an expert" line with a description indicating this
function batch-verifies multiple experts (admin-only), e.g., note that
batch_add_experts takes Env and Vec<Address> and sets multiple experts' status
to Verified, returning Result<(), RegistryError>; keep mention of admin-only and
Batch Verification.
|
Check tests as well @ryzen-xp |
|
hi @Bosun-Josh121 , plz give me some compliments on drip . |
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
Implemented Ban Expert security feature to enable admins to revoke verification status from bad actors.
Changes:
ban_expert()function insrc/contract.rswith admin authban_expert()andget_status()insrc/lib.rsAlreadyBannederror variantStorage Impact: Single storage write + event emission (similar gas cost to
verify_expert)📸 Evidence
Tests include:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.