feat(identity-registry): implement Expert Directory enumeration (#25)#28
Merged
Bosun-Josh121 merged 4 commits intoLightForgeHub:mainfrom Feb 24, 2026
Conversation
Add VerifiedExpertIndex(u64) and TotalVerifiedCount data keys to support on-chain iteration over verified experts. Implement: - add_expert_to_index: appends address at current count index, increments counter (instance storage), persists index entry with TTL - get_total_experts: returns total count from instance storage - get_expert_by_index: retrieves address at a given numeric index Closes LightForgeHub#25
Call add_expert_to_index in verify_expert and batch_add_experts only when the expert was not previously Verified, preventing duplicates in the enumerable directory. Expose get_total_experts and get_expert_by_index as contract-level functions. Closes LightForgeHub#25
…ndex Add two public contract methods to the IdentityRegistryContract interface so the frontend can enumerate the full verified expert directory by iterating from 0 to total_experts - 1. Closes LightForgeHub#25
Three new test cases covering the enumerable expert directory: - test_expert_directory_enumeration: verifies 3 experts via add_expert, asserts total == 3 and correct address at indices 0, 1, 2 - test_expert_directory_no_duplicates_on_reverify: confirms AlreadyVerified error and that total stays at 1 when re-verifying the same expert - test_expert_directory_via_batch_add: same index assertions through the batch_add_experts code path Closes LightForgeHub#25
📝 WalkthroughWalkthroughThe PR implements an enumerable directory of verified experts by introducing indexed storage for experts, index management functions, and public getters that allow querying total expert count and retrieving experts by sequential index position. Changes
Sequence DiagramsequenceDiagram
participant Frontend as Frontend/Client
participant Contract as Contract Logic
participant Storage as Storage Layer
participant Index as Index Store
Frontend->>Contract: verify_expert(expert_addr)
Contract->>Storage: Check expert status
alt Expert not yet verified
Storage-->>Contract: Status = Pending
Contract->>Storage: Update status to Verified
Storage-->>Contract: Status updated
Contract->>Storage: add_expert_to_index(expert_addr)
Storage->>Index: Get current total count
Index-->>Storage: count = N
Storage->>Index: Store expert at VerifiedExpertIndex(N)
Storage->>Index: Increment TotalVerifiedCount to N+1
Index-->>Storage: Index updated
Storage-->>Contract: Index updated
else Expert already verified
Storage-->>Contract: AlreadyVerified error
end
Contract-->>Frontend: Success/Error
Frontend->>Contract: get_total_experts()
Contract->>Storage: Retrieve TotalVerifiedCount
Storage->>Index: Fetch count
Index-->>Storage: count = N
Storage-->>Contract: N
Contract-->>Frontend: N
Frontend->>Contract: get_expert_by_index(index: 0)
Contract->>Storage: Retrieve expert at index
Storage->>Index: Fetch VerifiedExpertIndex(0)
Index-->>Storage: Address
Storage-->>Contract: Address
Contract-->>Frontend: Address
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
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)
64-92:⚠️ Potential issue | 🔴 Critical
Banned→Verifiedre-verification creates a duplicate index entry.The guard at Line 71 only blocks
Verified→Verified. A banned expert can be re-verified, andadd_expert_to_indexat Line 81 will append them to the index a second time, breaking the uniqueness invariant.Only index the expert on their first verification (i.e., when transitioning from
Unverified):🐛 Proposed fix
storage::set_expert_record(env, expert, ExpertStatus::Verified, data_uri); - storage::add_expert_to_index(env, expert); + if current_status == ExpertStatus::Unverified { + storage::add_expert_to_index(env, expert); + }🤖 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 64 - 92, verify_expert currently only prevents Verified→Verified but will re-add a Banned expert to the index; update the logic in verify_expert (which calls storage::get_expert_status, storage::set_expert_record, storage::add_expert_to_index and events::emit_status_change) so that storage::add_expert_to_index is only called when the previous status is ExpertStatus::Unverified (i.e., index the expert only on their first transition from Unverified→Verified), leaving the Verified check/error intact and keeping the status update and event emission unchanged.
27-36:⚠️ Potential issue | 🔴 CriticalSame
Banned→Verifiedduplicate index bug inbatch_add_experts.The guard at Line 29 only blocks
Verifiedstatus. ABannedexpert in the batch will be re-verified and re-indexed. Apply the same fix — only index when transitioning fromUnverified.🐛 Proposed fix
storage::set_expert_record(&env, &expert, ExpertStatus::Verified, empty_uri); - storage::add_expert_to_index(&env, &expert); + if status == ExpertStatus::Unverified { + storage::add_expert_to_index(&env, &expert); + }🤖 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 27 - 36, The batch_add_experts loop currently only prevents re-adding Verified experts but will re-verify and re-index Banned experts; update the logic in the loop (where storage::get_expert_status, storage::set_expert_record, storage::add_expert_to_index, and events::emit_status_change are used) to: keep the early return for status == ExpertStatus::Verified, call storage::set_expert_record to set Verified as before, but only call storage::add_expert_to_index when the previous status equals ExpertStatus::Unverified (i.e., index only on Unverified→Verified transitions); still call events::emit_status_change with the original status and new ExpertStatus::Verified.
🤖 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-registry-contract/src/storage.rs`:
- Around line 126-131: get_expert_by_index omits the TTL extension performed in
get_expert_record, so index entries can expire while still referenced; to fix,
after retrieving the Address in get_expert_by_index
(DataKey::VerifiedExpertIndex(index)), re-apply the same TTL-extension logic
used by get_expert_record (i.e., use the same persistent storage update/helper
that refreshes the TTL for the key) so the index entry's TTL is extended on each
read and prevents out-of-bounds panics.
- Around line 96-115: The index is append-only and allows duplicates because
re-verifying a previously Banned expert will call add_expert_to_index
unconditionally and get_total_experts returns the append-only count; to fix, add
an indexed-flag or live-count and guard re-indexing: modify add_expert_to_index
to first check a per-address marker (e.g., a new DataKey like
VerifiedIndexed(Address) or change semantics to check current status via
get(&DataKey::ExpertStatus(Address))) and skip appending if already indexed, and
update verify_expert and batch_add_experts to only call add_expert_to_index when
the previous status was Unverified (not when Banned→Verified); also maintain a
separate DataKey::CurrentVerifiedCount (increment on Verified transition,
decrement on Ban transition) and change get_total_experts to return that live
count instead of the append-only TotalVerifiedCount so frontends won’t see
duplicates.
In `@contracts/identity-registry-contract/src/test.rs`:
- Around line 531-556: Add a new unit test that reproduces the Banned→Verified
lifecycle to catch the duplicate-index bug: create env, register
IdentityRegistryContract and client, init with admin, add_expert(&expert, &uri)
and assert get_total_experts() == 1, call ban_expert(&expert), then re-add via
add_expert(&expert, &uri) and assert get_total_experts() remains 1 and that
get_expert_by_index(&0u64) returns the original expert; name the test e.g.
test_expert_directory_no_duplicates_on_reverify_after_ban to mirror the existing
test and exercise the verify→ban→re-verify path.
---
Outside diff comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 64-92: verify_expert currently only prevents Verified→Verified but
will re-add a Banned expert to the index; update the logic in verify_expert
(which calls storage::get_expert_status, storage::set_expert_record,
storage::add_expert_to_index and events::emit_status_change) so that
storage::add_expert_to_index is only called when the previous status is
ExpertStatus::Unverified (i.e., index the expert only on their first transition
from Unverified→Verified), leaving the Verified check/error intact and keeping
the status update and event emission unchanged.
- Around line 27-36: The batch_add_experts loop currently only prevents
re-adding Verified experts but will re-verify and re-index Banned experts;
update the logic in the loop (where storage::get_expert_status,
storage::set_expert_record, storage::add_expert_to_index, and
events::emit_status_change are used) to: keep the early return for status ==
ExpertStatus::Verified, call storage::set_expert_record to set Verified as
before, but only call storage::add_expert_to_index when the previous status
equals ExpertStatus::Unverified (i.e., index only on Unverified→Verified
transitions); still call events::emit_status_change with the original status and
new ExpertStatus::Verified.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/storage.rscontracts/identity-registry-contract/src/test.rs
Comment on lines
+96
to
+115
| pub fn add_expert_to_index(env: &Env, expert: &Address) { | ||
| let count: u64 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&DataKey::TotalVerifiedCount) | ||
| .unwrap_or(0u64); | ||
|
|
||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::VerifiedExpertIndex(count), expert); | ||
| env.storage().persistent().extend_ttl( | ||
| &DataKey::VerifiedExpertIndex(count), | ||
| LEDGERS_THRESHOLD, | ||
| LEDGERS_EXTEND_TO, | ||
| ); | ||
|
|
||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::TotalVerifiedCount, &(count + 1)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for relevant state transition and indexing logic
rg -n 'Banned|add_expert_to_index|AlreadyVerified' contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 965
🏁 Script executed:
#!/bin/bash
# Get the verify_expert function with context around line 30-35
sed -n '20,45p' contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Get the verify_expert function
rg -B15 -A10 'pub fn verify_expert' contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Get the complete verify_expert function
rg -A25 'pub fn verify_expert' contracts/identity-registry-contract/src/contract.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Check if there's any removal/deletion logic for the index in storage.rs
rg -n 'remove|delete|pop|clear' contracts/identity-registry-contract/src/storage.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Double-check by looking at the complete ExpertStatus enum to confirm Banned exists
rg -A5 'enum ExpertStatus' contracts/identity-registry-contract/src/Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 459
Append-only index creates duplicate entries when banned experts are re-verified.
The index has no removal path and is purely append-only. More critically, the verification logic allows the state transition Banned → Verified without preventing re-indexing. Both verify_expert and batch_add_experts only guard against Verified → Verified (checking if current_status == ExpertStatus::Verified), but they do not block Banned → Verified. This means a banned expert can be re-verified, causing add_expert_to_index to append a duplicate entry.
Additionally, get_total_experts returns the total index count (including duplicates), not the count of currently-verified experts, which is misleading.
Frontends iterating the index will need to filter both banned experts and duplicate entries client-side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/identity-registry-contract/src/storage.rs` around lines 96 - 115,
The index is append-only and allows duplicates because re-verifying a previously
Banned expert will call add_expert_to_index unconditionally and
get_total_experts returns the append-only count; to fix, add an indexed-flag or
live-count and guard re-indexing: modify add_expert_to_index to first check a
per-address marker (e.g., a new DataKey like VerifiedIndexed(Address) or change
semantics to check current status via get(&DataKey::ExpertStatus(Address))) and
skip appending if already indexed, and update verify_expert and
batch_add_experts to only call add_expert_to_index when the previous status was
Unverified (not when Banned→Verified); also maintain a separate
DataKey::CurrentVerifiedCount (increment on Verified transition, decrement on
Ban transition) and change get_total_experts to return that live count instead
of the append-only TotalVerifiedCount so frontends won’t see duplicates.
Comment on lines
+126
to
+131
| pub fn get_expert_by_index(env: &Env, index: u64) -> Address { | ||
| env.storage() | ||
| .persistent() | ||
| .get(&DataKey::VerifiedExpertIndex(index)) | ||
| .expect("Index out of bounds") | ||
| } |
There was a problem hiding this comment.
Missing TTL extension on index entry reads — inconsistent with get_expert_record pattern.
get_expert_record (Line 72-76) extends TTL on reads to keep frequently-accessed entries alive. get_expert_by_index does not, meaning index entries can expire while the count still references them, leading to panics on valid indices.
♻️ Proposed fix
pub fn get_expert_by_index(env: &Env, index: u64) -> Address {
+ let key = DataKey::VerifiedExpertIndex(index);
+ if env.storage().persistent().has(&key) {
+ env.storage()
+ .persistent()
+ .extend_ttl(&key, LEDGERS_THRESHOLD, LEDGERS_EXTEND_TO);
+ }
env.storage()
.persistent()
- .get(&DataKey::VerifiedExpertIndex(index))
+ .get(&key)
.expect("Index out of bounds")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/identity-registry-contract/src/storage.rs` around lines 126 - 131,
get_expert_by_index omits the TTL extension performed in get_expert_record, so
index entries can expire while still referenced; to fix, after retrieving the
Address in get_expert_by_index (DataKey::VerifiedExpertIndex(index)), re-apply
the same TTL-extension logic used by get_expert_record (i.e., use the same
persistent storage update/helper that refreshes the TTL for the key) so the
index entry's TTL is extended on each read and prevents out-of-bounds panics.
Comment on lines
+531
to
+556
| #[test] | ||
| fn test_expert_directory_no_duplicates_on_reverify() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| 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); | ||
|
|
||
| let uri = String::from_str(&env, "ipfs://expert"); | ||
| client.add_expert(&expert, &uri); | ||
|
|
||
| // Total is 1 | ||
| assert_eq!(client.get_total_experts(), 1u64); | ||
|
|
||
| // Re-verifying an already verified expert returns AlreadyVerified | ||
| let result = client.try_add_expert(&expert, &uri); | ||
| assert_eq!(result, Err(Ok(RegistryError::AlreadyVerified))); | ||
|
|
||
| // Total remains 1 — no duplicate in the index | ||
| assert_eq!(client.get_total_experts(), 1u64); | ||
| } |
There was a problem hiding this comment.
Missing test for Banned→Verified re-verification — the scenario that triggers the duplicate index bug.
This test only covers the Verified→Verified path. Add a test that exercises the full lifecycle: verify → ban → re-verify, and asserts that the index count does not increase on re-verification. This is exactly the scenario where the duplicate-index bug identified in contract.rs would surface.
Suggested test
#[test]
fn test_expert_directory_no_duplicates_on_reverify_after_ban() {
let env = Env::default();
env.mock_all_auths();
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);
let uri = String::from_str(&env, "ipfs://expert");
client.add_expert(&expert, &uri);
assert_eq!(client.get_total_experts(), 1u64);
// Ban the expert
client.ban_expert(&expert);
// Re-verify — should NOT create a duplicate index entry
client.add_expert(&expert, &uri);
assert_eq!(client.get_total_experts(), 1u64);
assert_eq!(client.get_expert_by_index(&0u64), expert);
}🤖 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 531 - 556, Add
a new unit test that reproduces the Banned→Verified lifecycle to catch the
duplicate-index bug: create env, register IdentityRegistryContract and client,
init with admin, add_expert(&expert, &uri) and assert get_total_experts() == 1,
call ban_expert(&expert), then re-add via add_expert(&expert, &uri) and assert
get_total_experts() remains 1 and that get_expert_by_index(&0u64) returns the
original expert; name the test e.g.
test_expert_directory_no_duplicates_on_reverify_after_ban to mirror the existing
test and exercise the verify→ban→re-verify path.
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.
Closes #25
Summary
DataKey::VerifiedExpertIndex(u64)andDataKey::TotalVerifiedCounttostorage.rs. Implementedadd_expert_to_index(appends address at current count, increments counter),get_total_experts, andget_expert_by_indexhelpers with proper TTL management.verify_expertandbatch_add_expertsincontract.rsto calladd_expert_to_indexonly when the expert was not previouslyVerified, preventing duplicates. Addedget_total_expertsandget_expert_by_indexcontract-level wrapper functions.get_total_experts(env) -> u64andget_expert_by_index(env, index: u64) -> Addressas public contract methods inlib.rs.add_expert, no-duplicate enforcement on re-verification, and enumeration viabatch_add_experts. All 20 tests pass.Design notes
TotalVerifiedCountis stored in instance storage (cheap reads, same TTL as contract) since it's a single counter accessed on every write.VerifiedExpertIndex(u64)entries are stored in persistent storage with the same 1-year TTL used for expert records, keeping index and records consistent.AlreadyVerifiedguard in bothverify_expertandbatch_add_expertsfires beforeadd_expert_to_indexis ever called.Test plan
cargo clippy— no warningscargo fmt— no changescargo build— cleancargo test— 20/20 passingtest_expert_directory_enumeration— 3 experts added viaadd_expert, total == 3, indices 0/1/2 correcttest_expert_directory_no_duplicates_on_reverify— re-verify returnsAlreadyVerified, total stays 1test_expert_directory_via_batch_add— same assertions throughbatch_add_expertspathSummary by CodeRabbit