Merged
Conversation
Adds RegistryError::NotBanned (code 10) to handle the case where unban_expert is called on an expert who is not currently banned. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Adds unban_expert function that requires admin auth, validates the expert is currently Banned, reverts status to Verified while preserving the existing data_uri, and emits a status_change event (Banned -> Verified). Total expert count is not incremented since the expert is already indexed. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Adds pub fn unban_expert(env: Env, expert: Address) -> Result<(), RegistryError> to the IdentityRegistryContract public interface. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Tests the full ban -> unban lifecycle: verifies status transitions (Verified -> Banned -> Verified), data_uri preservation, that total expert count does not increment on unban, and that NotBanned is returned when unbanning a non-banned expert.
📝 WalkthroughWalkthroughAdded an admin-only Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
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.
🧹 Nitpick comments (2)
contracts/identity-registry-contract/src/contract.rs (1)
125-133: Minor optimization opportunity: reduce storage reads.Both
get_expert_status(line 125) andget_expert_record(line 132) read from storage for the same expert. You could read the record once and extract the status from it. However, this matches the existing pattern inban_expert(lines 99, 106), so consistency is maintained.♻️ Optional refactor to reduce storage reads
pub fn unban_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 { + let existing = storage::get_expert_record(env, expert); + + if existing.status != ExpertStatus::Banned { return Err(RegistryError::NotBanned); } - // Preserve existing data_uri when unbanning - let existing = storage::get_expert_record(env, expert); - storage::set_expert_record(env, expert, ExpertStatus::Verified, existing.data_uri); + storage::set_expert_record(env, expert, ExpertStatus::Verified, existing.data_uri.clone()); events::emit_status_change( env, expert.clone(), - current_status, + existing.status, ExpertStatus::Verified, admin, ); Ok(()) }This same optimization could be applied to
ban_expertas well for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/contract.rs` around lines 125 - 133, Read the expert record once via storage::get_expert_record and derive the status from that record instead of calling storage::get_expert_status again; replace the initial get_expert_status call with a single get_expert_record call, check record.status (or equivalent) against ExpertStatus::Banned, then call storage::set_expert_record with ExpertStatus::Verified and existing.data_uri as before; apply the same single-read pattern to ban_expert (replace its two storage reads with one get_expert_record) to reduce redundant storage access.contracts/identity-registry-contract/src/test.rs (1)
785-788: Consider adding edge case tests for completeness.The core functionality is well-tested. For parity with
ban_experttest coverage, consider adding:
test_unban_expert_unauthorized- verifying unauthorized callers cannot unbantest_unban_before_contract_initialized- verifyingNotInitializederror when contract not initializedThese follow the existing patterns from
test_ban_expert_unauthorizedandtest_ban_before_contract_initialized.🧪 Optional additional tests
#[test] #[should_panic] fn test_unban_expert_unauthorized() { let env = Env::default(); let contract_id = env.register(IdentityRegistryContract, ()); let client = IdentityRegistryContractClient::new(&env, &contract_id); let admin = Address::generate(&env); let expert = Address::generate(&env); client.init(&admin); env.mock_all_auths(); let data_uri = String::from_str(&env, "ipfs://unauth-unban"); client.add_expert(&expert, &data_uri); client.ban_expert(&expert); // Clear auth mocking env.mock_auths(&[]); // This should panic due to missing authorization client.unban_expert(&expert); } #[test] fn test_unban_before_contract_initialized() { let env = Env::default(); let contract_id = env.register(IdentityRegistryContract, ()); let client = IdentityRegistryContractClient::new(&env, &contract_id); let expert = Address::generate(&env); env.mock_all_auths(); // Try to unban without initializing (should fail) let result = client.try_unban_expert(&expert); assert_eq!(result, Err(Ok(RegistryError::NotInitialized))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/test.rs` around lines 785 - 788, Add two edge-case tests paralleling the ban tests: implement test_unban_expert_unauthorized using IdentityRegistryContractClient where you init the contract with client.init(&admin), add and ban an expert (client.add_expert, client.ban_expert) while auths are mocked, then clear mocks (env.mock_auths(&[])) and call client.unban_expert(&expert) expecting a panic (#[should_panic]) due to missing authorization; and implement test_unban_before_contract_initialized which creates a client without calling client.init, mocks auths (env.mock_all_auths()), calls client.try_unban_expert(&expert) and asserts Err(Ok(RegistryError::NotInitialized)), mirroring test_ban_expert_unauthorized and test_ban_before_contract_initialized patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 125-133: Read the expert record once via
storage::get_expert_record and derive the status from that record instead of
calling storage::get_expert_status again; replace the initial get_expert_status
call with a single get_expert_record call, check record.status (or equivalent)
against ExpertStatus::Banned, then call storage::set_expert_record with
ExpertStatus::Verified and existing.data_uri as before; apply the same
single-read pattern to ban_expert (replace its two storage reads with one
get_expert_record) to reduce redundant storage access.
In `@contracts/identity-registry-contract/src/test.rs`:
- Around line 785-788: Add two edge-case tests paralleling the ban tests:
implement test_unban_expert_unauthorized using IdentityRegistryContractClient
where you init the contract with client.init(&admin), add and ban an expert
(client.add_expert, client.ban_expert) while auths are mocked, then clear mocks
(env.mock_auths(&[])) and call client.unban_expert(&expert) expecting a panic
(#[should_panic]) due to missing authorization; and implement
test_unban_before_contract_initialized which creates a client without calling
client.init, mocks auths (env.mock_all_auths()), calls
client.try_unban_expert(&expert) and asserts
Err(Ok(RegistryError::NotInitialized)), mirroring test_ban_expert_unauthorized
and test_ban_before_contract_initialized patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b388b30-c5cb-4e4f-8453-c46ae7908355
📒 Files selected for processing (4)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/error.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/test.rs
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
2 participants
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.
🧠 SkillSphere Pull Request 🌐
Mark with an
xall the checkboxes that apply (like[x])cargo test(All tests passed)📌 Type of Change
📝 Changes Description
Adds
unban_expertto theidentity-registry-contract, allowing admins to reinstate a banned expert without requiring a new wallet.Files changed:
src/error.rs— AddedNotBanned = 10error variant for whenunban_expertis called on a non-banned expertsrc/contract.rs— Implementedunban_expert: requires admin auth, validates status isBanned, reverts toVerifiedpreserving the existingdata_uri, and emits astatus_changeevent (Banned → Verified)src/lib.rs— Exposedpub fn unban_expert(env: Env, expert: Address) -> Result<(), RegistryError>in#[contractimpl]src/test.rs— Addedtest_unban_expertcovering the full lifecycleStorage impact: No new storage keys. The existing expert record is updated in-place (
set_expert_record). The total expert counter is intentionally not incremented since the expert is already in the index.Gas impact: Minimal — one storage read (
get_expert_record), one storage write (set_expert_record), and one event emission.📸 Evidence
running 25 tests
test test::test_ban_before_contract_initialized ... ok
test test::test_batch_update_profiles_max_vec - should panic ... ok
test test::test_add_expert_unauthorized - should panic ... ok
test test::test_ban_unverified_expert ... ok
test test::test_batch_verification_no_admin - should panic ... ok
test test::test_batch_verification_max_vec - should panic ... ok
test test::test_batch_update_profiles_uri_too_long ... ok
test test::test_ban_expert_unauthorized - should panic ... ok
test test::test_add_expert ... ok
test test::test_ban_expert ... ok
test test::test_data_uri_persisted_on_verify ... ok
test test::test_complete_expert_lifecycle ... ok
test test::test_expert_directory_no_duplicates_on_reverify ... ok
test test::test_expert_status_changed_event ... ok
test test::test_initialization ... ok
test test::test_ban_expert_workflow ... ok
test test::test_expert_directory_via_batch_add ... ok
test test::test_batch_verification_check_status ... ok
test test::test_getters ... ok
test test::test_update_profile_rejections ... ok
test test::test_expert_directory_enumeration ... ok
test test::test_unban_expert ... ok
test test::test_update_profile_updates_uri_and_emits_event ... ok
test test::test_batch_update_profiles ... ok
test test::test_expert_pagination ... ok
test result: ok. 25 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.28s
Closes #42
🌌 Comments
Edge cases covered in
test_unban_expert:unban_experton an expert who isVerified(not banned) returnsNotBanneddata_uriis fully preserved after the ban/unban cycleget_total_experts()does not change after unban — the expert was already in the indexReviewers should note that
unban_expertintentionally does not calladd_expert_to_index, since the expert retains their original index position from initial verification.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
Tests