refactor(storage): fix unbounded vector growth with O(1) indexed storage (#24)#46
Merged
Bosun-Josh121 merged 1 commit intoLightForgeHub:mainfrom Mar 26, 2026
Conversation
…storage - Remove UserBookings and ExpertBookings Vec storage (unbounded growth risk) - Introduce composite keys: UserBooking, UserBookingCount, ExpertBooking, ExpertBookingCount - Refactor add_booking_* to constant-time writes (no full vector rewrite) - Add paginated getters for user and expert bookings - Update contract interface to support pagination (start_index, limit) - Add booking count getters for efficient frontend querying - Add tests for 50-booking scale and pagination correctness Fixes LightForgeHub#24
📝 WalkthroughWalkthroughThe PR refactors booking list storage from unbounded vectors to indexed keys with O(1) write semantics. Storage now uses composite keys for individual booking entries paired with counters, eliminating gas-expensive vector rewriting. Pagination APIs replace bulk retrieval to support safe frontend queries across large booking histories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🤖 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/payment-vault-contract/src/storage.rs`:
- Around line 126-149: The functions get_user_bookings_paginated and
get_expert_bookings_paginated allow an unbounded caller-controlled limit and can
overflow when computing start_index + limit; define a contract-level constant
(e.g. MAX_PAGE_SIZE) and clamp the incoming limit to that (limit =
limit.min(MAX_PAGE_SIZE)), then compute the end using a safe add (e.g.
start_index.saturating_add(limit) or
start_index.checked_add(limit).unwrap_or_max()) before taking min(count) to
avoid overflow; apply these changes to the functions named
get_user_bookings_paginated and get_expert_bookings_paginated and ensure the
clamped limit is used when iterating and pushing booking ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e95b68f0-7c71-4527-8a41-dfc32bc3cea1
📒 Files selected for processing (3)
contracts/payment-vault-contract/src/lib.rscontracts/payment-vault-contract/src/storage.rscontracts/payment-vault-contract/src/test.rs
Comment on lines
+126
to
149
| pub fn get_user_bookings_paginated( | ||
| env: &Env, | ||
| user: &Address, | ||
| start_index: u32, | ||
| limit: u32, | ||
| ) -> soroban_sdk::Vec<u64> { | ||
| let count = get_user_booking_count(env, user); | ||
| let mut result = soroban_sdk::Vec::new(env); | ||
|
|
||
| let end = (start_index + limit).min(count); | ||
| let mut i = start_index; | ||
| while i < end { | ||
| if let Some(booking_id) = env | ||
| .storage() | ||
| .persistent() | ||
| .get::<DataKey, u64>(&DataKey::UserBooking(user.clone(), i)) | ||
| { | ||
| result.push_back(booking_id); | ||
| } | ||
| i += 1; | ||
| } | ||
|
|
||
| result | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the current pagination math and whether release overflow checks are configured.
rg -n 'get_(user|expert)_bookings_paginated|start_index\s*\+\s*limit|saturating_add|checked_add|MAX_BOOKINGS_PAGE_SIZE' contracts/payment-vault-contract/src
fd '^Cargo.toml$' | while read -r file; do
echo "--- $file"
sed -n '/\[profile.release\]/,/^\[/p' "$file"
doneRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1503
Implement explicit pagination size bounds.
The limit parameter is fully caller-controlled, allowing requests for arbitrarily large pages. While start_index + limit arithmetic will panic on overflow in release builds (overflow-checks enabled in Cargo.toml), relying on panics is not idiomatic defense—explicit bounds should be enforced instead. Clamp limit to a contract-defined maximum to prevent DoS and excessive resource consumption.
This applies to both get_user_bookings_paginated (line 126–149) and get_expert_bookings_paginated (line 174–197).
🛠️ Proposed fix
+const MAX_BOOKINGS_PAGE_SIZE: u32 = 100;
+
pub fn get_user_bookings_paginated(
env: &Env,
user: &Address,
start_index: u32,
limit: u32,
) -> soroban_sdk::Vec<u64> {
let count = get_user_booking_count(env, user);
let mut result = soroban_sdk::Vec::new(env);
- let end = (start_index + limit).min(count);
+ let end = start_index
+ .saturating_add(limit.min(MAX_BOOKINGS_PAGE_SIZE))
+ .min(count);
let mut i = start_index;
while i < end {
if let Some(booking_id) = env
.storage()
.persistent()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/payment-vault-contract/src/storage.rs` around lines 126 - 149, The
functions get_user_bookings_paginated and get_expert_bookings_paginated allow an
unbounded caller-controlled limit and can overflow when computing start_index +
limit; define a contract-level constant (e.g. MAX_PAGE_SIZE) and clamp the
incoming limit to that (limit = limit.min(MAX_PAGE_SIZE)), then compute the end
using a safe add (e.g. start_index.saturating_add(limit) or
start_index.checked_add(limit).unwrap_or_max()) before taking min(count) to
avoid overflow; apply these changes to the functions named
get_user_bookings_paginated and get_expert_bookings_paginated and ensure the
clamped limit is used when iterating and pushing booking ids.
9 tasks
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.
🛠 Fix: Unbounded Vector Growth (Gas Optimization)
This PR resolves issue #24 by replacing Vec-based booking storage with a scalable, gas-efficient indexed storage model.
🚨 Problem
Previously, user and expert bookings were stored as:
UserBookings(Address) -> Vec<u64>ExpertBookings(Address) -> Vec<u64>Each new booking required:
This results in O(N) storage operations, which:
✅ Solution
🔹 Storage Refactor (O(1) Writes)
Replaced vector storage with indexed keys:
UserBooking(Address, u32)UserBookingCount(Address)ExpertBooking(Address, u32)ExpertBookingCount(Address)Now:
🔹 Pagination Support
Updated interface:
Added:
get_user_booking_countget_expert_booking_countThis allows safe querying of large datasets.
🔹 Tests Added
🎯 Result
📌 Files Changed
storage.rs— core refactorlib.rs— interface updatetest.rs— new tests🔗 Issue
Closes #24
Summary by CodeRabbit
New Features
Documentation
Refactor