Implement Batch Update Expert Profiles#49
Merged
Bosun-Josh121 merged 2 commits intoLightForgeHub:mainfrom Mar 26, 2026
Merged
Conversation
Add batch_update_profiles function for admins to update multiple expert metadata URIs in a single transaction, optimizing gas usage during migrations. - Implement batch_update_profiles in contract.rs with admin auth - Support updates of address, URI, and status (as u32) - Limit batch size to 20 to prevent DoS - Validate URI length (max 64 chars) - Add comprehensive tests (5 updates, max vec, URI length validation)
📝 WalkthroughWalkthroughAdds an admin-only Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (caller)
participant Contract as IdentityRegistryContract
participant Storage as storage::set_expert_record
Admin->>Contract: batch_update_profiles(updates)
Contract->>Contract: check admin auth
alt not admin
Contract-->>Admin: Err(Unauthorized)
else admin
Contract->>Contract: verify updates.len() <= 20
alt too large
Contract-->>Admin: Err(ExpertVecMax)
else ok
loop for each update
Contract->>Contract: validate uri length (<=64)
alt uri too long
Contract-->>Admin: Err(UriTooLong) and abort
else valid uri
Contract->>Contract: map status_u32 -> ExpertStatus
alt invalid status
Contract-->>Admin: Err(NotVerified) and abort
else valid status
Contract->>Storage: set_expert_record(address, uri, status)
Storage-->>Contract: Ok
end
end
end
Contract-->>Admin: Ok(())
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
🤖 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/contract.rs`:
- Around line 184-189: The match that maps status_u32 to ExpertStatus currently
returns RegistryError::NotVerified on the wildcard arm, conflating malformed
input with a domain state; change the wildcard branch to return a new, dedicated
error variant (e.g., RegistryError::InvalidStatus or
RegistryError::MalformedStatus) instead of NotVerified, add that variant to the
RegistryError enum (with any required Display/From impls), and update any call
sites/tests that expect the old error so callers can distinguish client-side bad
input from an actual "not verified" domain result; the change should be applied
where status_u32 is converted to status (the match producing status) and in the
RegistryError type definition and conversions.
- Around line 175-193: The batch loop currently calls storage::set_expert_record
which only overwrites storage and skips directory indexing and event emission;
update the loop to reuse the same single-record mutation path that maintains the
directory index and emits the status_change/profile_updated events (the same
logic used by the single-record flow) instead of calling
storage::set_expert_record directly, so get_total_experts/get_expert_by_index
and event-driven consumers remain consistent — alternatively, if batch must be
limited to URI-only edits, explicitly validate and restrict this endpoint to URI
changes and document that it will not modify index or emit events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9935f85-bea8-4ab5-b81a-d1fd15f6d4a5
📒 Files selected for processing (3)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/test.rs
Comment on lines
+175
to
+193
| for update in updates { | ||
| let (expert, new_uri, status_u32) = update; | ||
|
|
||
| // Validate URI length | ||
| if new_uri.len() > 64 { | ||
| return Err(RegistryError::UriTooLong); | ||
| } | ||
|
|
||
| // Convert u32 to ExpertStatus | ||
| let status = match status_u32 { | ||
| 0 => ExpertStatus::Unverified, | ||
| 1 => ExpertStatus::Verified, | ||
| 2 => ExpertStatus::Banned, | ||
| _ => return Err(RegistryError::NotVerified), // Invalid status value | ||
| }; | ||
|
|
||
| // Update the expert record | ||
| storage::set_expert_record(env, &expert, status, new_uri); | ||
| } |
There was a problem hiding this comment.
Don't bypass the registry side effects in the batch path.
On Line 192, this loop only calls storage::set_expert_record, and that helper just overwrites the stored record (contracts/identity-registry-contract/src/storage.rs:46-64). If one of these updates promotes an address to Verified, the expert is never added to the directory index, and none of the status_change / profile_updated events used by the single-record flows are emitted. That leaves get_total_experts / get_expert_by_index and any event-driven sync consumer out of step with the state written by this entrypoint. Please preserve the same indexing/event behavior here, or explicitly restrict this API to URI-only edits.
🤖 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 175 - 193,
The batch loop currently calls storage::set_expert_record which only overwrites
storage and skips directory indexing and event emission; update the loop to
reuse the same single-record mutation path that maintains the directory index
and emits the status_change/profile_updated events (the same logic used by the
single-record flow) instead of calling storage::set_expert_record directly, so
get_total_experts/get_expert_by_index and event-driven consumers remain
consistent — alternatively, if batch must be limited to URI-only edits,
explicitly validate and restrict this endpoint to URI changes and document that
it will not modify index or emit events.
Comment on lines
+184
to
+189
| let status = match status_u32 { | ||
| 0 => ExpertStatus::Unverified, | ||
| 1 => ExpertStatus::Verified, | ||
| 2 => ExpertStatus::Banned, | ||
| _ => return Err(RegistryError::NotVerified), // Invalid status value | ||
| }; |
There was a problem hiding this comment.
Return a dedicated error for malformed status values.
Line 188 maps an out-of-range status_u32 to RegistryError::NotVerified, which is indistinguishable from a real domain error. Callers cannot tell “bad request” from “expert is not verified,” so retries and client-side validation become ambiguous. A separate InvalidStatus-style error would make this API much safer to consume.
🤖 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 184 - 189,
The match that maps status_u32 to ExpertStatus currently returns
RegistryError::NotVerified on the wildcard arm, conflating malformed input with
a domain state; change the wildcard branch to return a new, dedicated error
variant (e.g., RegistryError::InvalidStatus or RegistryError::MalformedStatus)
instead of NotVerified, add that variant to the RegistryError enum (with any
required Display/From impls), and update any call sites/tests that expect the
old error so callers can distinguish client-side bad input from an actual "not
verified" domain result; the change should be applied where status_u32 is
converted to status (the match producing status) and in the RegistryError type
definition and conversions.
Collaborator
|
@Josue19-08 pls resolve the conflicts |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
contracts/identity-registry-contract/src/contract.rs (2)
183-189:⚠️ Potential issue | 🟡 MinorReturn a dedicated error for invalid status values.
Line 188 maps out-of-range
status_u32toRegistryError::NotVerified, which is indistinguishable from a domain error. Callers cannot differentiate "bad request" from "expert is not verified." A dedicatedInvalidStatuserror variant would improve API clarity.,
🤖 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 183 - 189, The match converting status_u32 into ExpertStatus currently maps out-of-range values to RegistryError::NotVerified, conflating an invalid input with a domain state; add a new RegistryError variant (e.g., InvalidStatus) and update the conversion branch to return Err(RegistryError::InvalidStatus) for the _ case instead of NotVerified, adjusting any places that construct or pattern-match RegistryError accordingly so callers can distinguish bad input from an unverified expert when using status_u32, ExpertStatus, and RegistryError.
175-193:⚠️ Potential issue | 🟠 MajorDirectory indexing and event emission are still missing.
This loop only calls
storage::set_expert_record, bypassing the index management and events used by other flows (batch_add_experts,verify_expert,ban_expert). If this batch promotes an address toVerified, the expert won't appear inget_total_experts/get_expert_by_index, and event-driven consumers will be out of sync.Either reuse the same logic path that maintains index/events, or explicitly restrict this API to URI-only edits and document the limitation.
,
🤖 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 175 - 193, The loop currently calls storage::set_expert_record directly and therefore skips index updates and event emission used by batch_add_experts / verify_expert / ban_expert, so update the logic in the loop inside contract.rs to either (A) reuse the existing flows that maintain indices and emit events by delegating to the same helper functions (e.g., call the verification/ban/add helper used by verify_expert, ban_expert, batch_add_experts when status transitions occur) so that get_total_experts/get_expert_by_index stay consistent, or (B) explicitly forbid status changes in this API (only allow URI edits) by checking that status_u32 matches the existing stored status and returning an error if it would change, and document that limitation; ensure you reference storage::set_expert_record only for pure URI updates and invoke the same event-emitting/index-managing functions when promoting/demoting an expert.
🧹 Nitpick comments (3)
contracts/identity-registry-contract/src/contract.rs (1)
161-166: Docstring understates the function's capability.The docstring says "update multiple expert metadata URIs" but the function also changes status without any transition validation. Unlike
verify_expertandban_expertwhich guard against no-op transitions (AlreadyVerified,AlreadyBanned), this function silently allows any status change—including reverting a banned expert to verified.Consider either:
- Adding transition guards for consistency with single-record flows, or
- Updating the docstring to clarify this is an admin override that bypasses normal guards.
🤖 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 161 - 166, The docstring for batch_update_profiles is misleading and the function currently allows arbitrary status changes (including reverting banned experts) without transition checks; either add the same no-op guards used by verify_expert and ban_expert (check and return RegistryError::AlreadyVerified or RegistryError::AlreadyBanned when appropriate before applying updates) or explicitly change the docstring to state this is an admin override that bypasses normal transition guards; update references to the function name batch_update_profiles and error variants AlreadyVerified/AlreadyBanned (and RegistryError) so reviewers can verify the guard logic or the clarified docstring.contracts/identity-registry-contract/src/test.rs (2)
705-705: Missing test coverage for invalid status values.There's no test verifying behavior when
status_u32is outside the valid range (0, 1, 2). Adding a test liketest_batch_update_profiles_invalid_statuswould ensure the error path (currently returningNotVerified) is exercised.🧪 Suggested test
#[test] fn test_batch_update_profiles_invalid_status() { 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); client.init(&admin); let expert = Address::generate(&env); let uri = String::from_str(&env, "ipfs://test"); // status_u32 = 99 is invalid let updates = vec![&env, (expert.clone(), uri, 99u32)]; let result = client.try_batch_update_profiles(&updates); // Currently returns NotVerified; ideally would be InvalidStatus assert!(result.is_err()); }🤖 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` at line 705, Add a unit test named test_batch_update_profiles_invalid_status that calls IdentityRegistryContractClient::try_batch_update_profiles with an update containing an out-of-range status_u32 (e.g., 99) and asserts it returns an Err; locate the client helper IdentityRegistryContractClient and the contract method try_batch_update_profiles to construct the call, use Env::default(), env.mock_all_auths(), register the contract, init the admin, create an expert Address and uri, build updates as vec![&env, (expert.clone(), uri, 99u32)] and assert result.is_err() to exercise the invalid-status error path (currently returning NotVerified).
622-629: Test only covers URI updates, not actual status transitions.All updates use
status_u32 = 1on already-Verified experts, so this test doesn't exercise status changes. Consider adding a test case that changes an expert fromVerifiedtoBanned(or vice versa) to validate the status mapping works and to surface any index/event consistency issues.🤖 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 622 - 629, The test currently builds `updates` with only `status_u32 = 1` for experts (see the `updates` vec) so it never exercises status transitions; modify the test in test.rs to include at least one entry that flips an expert's status (e.g., change `expert3` or `expert4` from `1` to the numeric value for `Banned` and another from `Banned` back to `Verified`) and assert post-update state, index consistency, and emitted events; locate the `updates` vec and add entries with the alternate status_u32 values, then extend assertions that check the expert's status mapping, storage index updates, and any emitted events/indices for those specific experts (refer to `updates`, `expert1`..`expert5`, and `status_u32`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 183-189: The match converting status_u32 into ExpertStatus
currently maps out-of-range values to RegistryError::NotVerified, conflating an
invalid input with a domain state; add a new RegistryError variant (e.g.,
InvalidStatus) and update the conversion branch to return
Err(RegistryError::InvalidStatus) for the _ case instead of NotVerified,
adjusting any places that construct or pattern-match RegistryError accordingly
so callers can distinguish bad input from an unverified expert when using
status_u32, ExpertStatus, and RegistryError.
- Around line 175-193: The loop currently calls storage::set_expert_record
directly and therefore skips index updates and event emission used by
batch_add_experts / verify_expert / ban_expert, so update the logic in the loop
inside contract.rs to either (A) reuse the existing flows that maintain indices
and emit events by delegating to the same helper functions (e.g., call the
verification/ban/add helper used by verify_expert, ban_expert, batch_add_experts
when status transitions occur) so that get_total_experts/get_expert_by_index
stay consistent, or (B) explicitly forbid status changes in this API (only allow
URI edits) by checking that status_u32 matches the existing stored status and
returning an error if it would change, and document that limitation; ensure you
reference storage::set_expert_record only for pure URI updates and invoke the
same event-emitting/index-managing functions when promoting/demoting an expert.
---
Nitpick comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 161-166: The docstring for batch_update_profiles is misleading and
the function currently allows arbitrary status changes (including reverting
banned experts) without transition checks; either add the same no-op guards used
by verify_expert and ban_expert (check and return RegistryError::AlreadyVerified
or RegistryError::AlreadyBanned when appropriate before applying updates) or
explicitly change the docstring to state this is an admin override that bypasses
normal transition guards; update references to the function name
batch_update_profiles and error variants AlreadyVerified/AlreadyBanned (and
RegistryError) so reviewers can verify the guard logic or the clarified
docstring.
In `@contracts/identity-registry-contract/src/test.rs`:
- Line 705: Add a unit test named test_batch_update_profiles_invalid_status that
calls IdentityRegistryContractClient::try_batch_update_profiles with an update
containing an out-of-range status_u32 (e.g., 99) and asserts it returns an Err;
locate the client helper IdentityRegistryContractClient and the contract method
try_batch_update_profiles to construct the call, use Env::default(),
env.mock_all_auths(), register the contract, init the admin, create an expert
Address and uri, build updates as vec![&env, (expert.clone(), uri, 99u32)] and
assert result.is_err() to exercise the invalid-status error path (currently
returning NotVerified).
- Around line 622-629: The test currently builds `updates` with only `status_u32
= 1` for experts (see the `updates` vec) so it never exercises status
transitions; modify the test in test.rs to include at least one entry that flips
an expert's status (e.g., change `expert3` or `expert4` from `1` to the numeric
value for `Banned` and another from `Banned` back to `Verified`) and assert
post-update state, index consistency, and emitted events; locate the `updates`
vec and add entries with the alternate status_u32 values, then extend assertions
that check the expert's status mapping, storage index updates, and any emitted
events/indices for those specific experts (refer to `updates`,
`expert1`..`expert5`, and `status_u32`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66b0d6c9-dec6-4768-8950-4fe709cf0999
📒 Files selected for processing (3)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/identity-registry-contract/src/lib.rs
Contributor
Author
Done @Bosun-Josh121 |
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
This PR implements batch profile updates for the identity-registry-contract, allowing administrators to sync metadata URIs for multiple experts in a single transaction, significantly reducing gas costs during data migrations.
Changes:
batch_update_profiles(env, updates: Vec<(Address, String, u32)>)function incontract.rslib.rs)Impact: Administrators can now efficiently bulk-modify expert records without causing network congestion, making data migrations and bulk updates much more cost-effective.
📸 Evidence
All tests pass including the new batch update tests:
🌌 Comments
The implementation includes several safety features:
The function signature uses
u32for status to match Soroban's serialization format (#[repr(u32)]), making it easier for clients to interact with the contract.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