Implement Expert Directory Pagination#48
Conversation
Add get_experts_paginated function to efficiently fetch experts in batches. This addresses the performance issue of fetching large expert lists. - Implement get_experts_paginated in contract.rs with start_index and limit - Expose function in public API (lib.rs) - Add comprehensive test with 15 experts, validating pagination behavior
📝 WalkthroughWalkthroughA new pagination function is added to the identity registry contract to fetch expert addresses in batches. The implementation includes the core pagination logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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🧪 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: 1
🧹 Nitpick comments (1)
contracts/identity-registry-contract/src/test.rs (1)
585-625: Add an explicit out-of-range pagination assertion.This test validates the main paths well, but it does not lock the
start_index > totalbehavior. Adding that check here will prevent regressions on the empty-page contract.✅ Suggested test extension
fn test_expert_pagination() { @@ // Verify the last 5 experts match for i in 0..5 { assert_eq!(page2.get(i as u32).unwrap(), experts.get((i + 10) as u32).unwrap()); } + + // Fetch start beyond total (should return empty) + let page3 = client.get_experts_paginated(&20u64, &10u64); + assert_eq!(page3.len(), 0); }🤖 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 585 - 625, The test_expert_pagination lacks an assertion for the out-of-range start index case; extend the test_expert_pagination to call get_experts_paginated with a start index greater than client.get_total_experts() (e.g., start = total + 1) and assert the returned page is empty (len == 0) to lock expected start_index > total behavior and prevent regressions in get_experts_paginated.
🤖 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 168-172: The pagination end bound computation in
get_experts_paginated can overflow when computing start_index + limit; replace
the raw addition with saturating arithmetic (e.g.,
start_index.saturating_add(limit)) when computing end_index so the sum never
wraps, then compare that saturated sum to total to pick the min for end_index
(retain the existing variables end_index, start_index, limit, total and the same
branch logic but use saturating_add).
---
Nitpick comments:
In `@contracts/identity-registry-contract/src/test.rs`:
- Around line 585-625: The test_expert_pagination lacks an assertion for the
out-of-range start index case; extend the test_expert_pagination to call
get_experts_paginated with a start index greater than client.get_total_experts()
(e.g., start = total + 1) and assert the returned page is empty (len == 0) to
lock expected start_index > total behavior and prevent regressions in
get_experts_paginated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59fbd94b-125b-4a0f-a1ff-4b8366955336
📒 Files selected for processing (3)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/test.rs
| let end_index = if start_index + limit > total { | ||
| total | ||
| } else { | ||
| start_index + limit | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd contracts/identity-registry-contract && head -n 180 src/contract.rs | tail -n 30Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 1031
🏁 Script executed:
cd contracts/identity-registry-contract && cat -n src/contract.rs | sed -n '150,180p'Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 1306
🏁 Script executed:
rg -n "get_experts_paginated" contracts/identity-registry-contract/src/contract.rs -A 25Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 708
🏁 Script executed:
rg -n "get_experts_paginated" contracts/identity-registry-contract/src/ -B 2 -A 2Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 2085
🏁 Script executed:
rg -n "#\[contract\]|#\[derive_contract\]|pub fn get_experts" contracts/identity-registry-contract/src/contract.rs -B 1 -A 5Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 474
🏁 Script executed:
fd "contract.rs|lib.rs|mod.rs" contracts/identity-registry-contract/src/ -type fRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 249
🏁 Script executed:
cat -n contracts/identity-registry-contract/src/lib.rs | head -n 80Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 3309
🏁 Script executed:
rg -n "#\[contract\]" contracts/identity-registry-contract/src/Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 141
Prevent u64 overflow in pagination bound calculation.
get_experts_paginated is a public contract method that accepts arbitrary start_index and limit values from external callers. On lines 168 and 171, start_index + limit can overflow; in Soroban release builds, this wraps to an unexpected value and breaks the bounds check. Use saturating arithmetic to ensure predictable, safe behavior.
🔧 Proposed fix
- let end_index = if start_index + limit > total {
- total
- } else {
- start_index + limit
- };
+ let end_index = core::cmp::min(start_index.saturating_add(limit), total);📝 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.
| let end_index = if start_index + limit > total { | |
| total | |
| } else { | |
| start_index + limit | |
| }; | |
| let end_index = core::cmp::min(start_index.saturating_add(limit), total); |
🤖 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 168 - 172,
The pagination end bound computation in get_experts_paginated can overflow when
computing start_index + limit; replace the raw addition with saturating
arithmetic (e.g., start_index.saturating_add(limit)) when computing end_index so
the sum never wraps, then compare that saturated sum to total to pick the min
for end_index (retain the existing variables end_index, start_index, limit,
total and the same branch logic but use saturating_add).
2 participants
🧠 SkillSphere Pull Request 🌐
Mark with an
xall the checkboxes that apply (like[x])Closes #44
cargo test(All tests passed)📌 Type of Change
📝 Changes Description
This PR implements pagination for the expert directory in the identity-registry-contract. The frontend/indexer needed to fetch experts in batches instead of individual indexes for performance.
Changes:
get_experts_paginated(env, start_index, limit)function incontract.rsthat returns a vector of expert addresseslib.rs)Impact: This allows efficient batch fetching of large expert lists, significantly improving performance for the frontend and indexer.
📸 Evidence
All tests pass including the new pagination test:
🌌 Comments
The implementation handles edge cases properly:
start_index + limitexceeds totalstart_indexis beyond total expertsget_expert_by_indexThank 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