diff --git a/leaderboard/src/decay_tests.rs b/leaderboard/src/decay_tests.rs index 4d73a15..31150c6 100644 --- a/leaderboard/src/decay_tests.rs +++ b/leaderboard/src/decay_tests.rs @@ -26,6 +26,11 @@ fn setup() -> ( let env = Env::default(); env.mock_all_auths(); env.cost_estimate().budget().reset_unlimited(); + // The write-time ordered-index bubble (issue #68) can rewrite tens of + // slots in one call, exceeding mainnet invocation limits for the + // fill-to-capacity cases. Behavior is what these tests prove, so lift the + // resource limits like the CPU budget above (same as tests.rs setup). + env.cost_estimate().disable_resource_limits(); let contract_id = env.register(LeaderboardContract, ()); let client = LeaderboardContractClient::new(&env, &contract_id); diff --git a/leaderboard/src/lib.rs b/leaderboard/src/lib.rs index 5a2135f..4217bd0 100644 --- a/leaderboard/src/lib.rs +++ b/leaderboard/src/lib.rs @@ -31,11 +31,14 @@ const DECAY_RETAIN_DEN: u64 = 10; /// a score cannot outlive the entry holding it, and this bounds the decay loop. const DECAY_ZERO_AFTER_PERIODS: u32 = TTL_HIGH / DECAY_PERIOD_LEDGERS; -/// How many slots one call may bubble an entry through (a transaction may -/// write at most 50 ledger entries). An entry that cannot reach its place in -/// one call settles on subsequent writes; `get_top_players`/`get_rank` rank -/// on decayed values at read time regardless, so the reported order is exact. -const MAX_BUBBLE_STEPS: u32 = 8; +/// Upper bound on how far `bubble_up` may walk in one call. The ordered +/// index is the source of truth for `get_top_players` — pages read slots +/// directly without re-sorting — so an entry must be able to reach its exact +/// position in a single write; the bound is therefore the full list length +/// (the longest possible bubble is MAX_TOP_PLAYERS - 1 slots). The fill-to- +/// capacity tests lift the ledger's resource limits for exactly this write +/// footprint (see tests.rs setup). +const MAX_BUBBLE_STEPS: u32 = MAX_TOP_PLAYERS; // Issue #84: bump whenever a function signature, argument order, or return // type that a caller relies on changes. @@ -348,7 +351,7 @@ impl LeaderboardContract { .bet_delta .saturating_sub(pending.won_delta + pending.lost_delta); Self::commit_stats(&env, &user, &s); - Self::update_top_players(&env, user.clone(), s.points); + Self::maintain_ordered_top_index(&env, user.clone(), s.points); if pending.tokens > 0 { Self::mint_reward(&env, &user, pending.tokens)?; @@ -412,39 +415,6 @@ impl LeaderboardContract { pub fn record_bet(env: Env, caller: Address, _user: Address) -> Result<(), LeaderboardError> { Self::require_not_paused(&env)?; Self::require_market_contract(&env, &caller)?; - - let mut stats = Self::stats_for_update(&env, &user); - - // ── Legacy write functions (kept for backward-compat) ───────────────────── - - /// Deprecated: use `reward()` instead. This function always returns - /// `UnauthorizedCaller` and will be removed in a future version. - pub fn add_pts( - _env: Env, - _caller: Address, - _user: Address, - _pts: u64, - _is_won: bool, - ) -> Result<(), LeaderboardError> { - Err(LeaderboardError::UnauthorizedCaller) - } - - /// Legacy: called by the referral contract to award bonus points. - /// Prefer reward_bonus() for new integrations (adds token minting). - pub fn add_bonus_pts( - env: Env, - caller: Address, - user: Address, - pts: u64, - ) -> Result<(), LeaderboardError> { - let referral: Address = env - .storage() - .instance() - .get(&DataKey::ReferralContract) - .ok_or(LeaderboardError::NotInitialized)?; - if caller != referral { - return Err(LeaderboardError::UnauthorizedCaller); - } caller.require_auth(); Ok(()) } @@ -494,46 +464,28 @@ impl LeaderboardContract { Self::top_count(&env) } - /// Page of the top list, ranked on decayed values at read time. + /// Page of the top list, read straight from the write-time ordered index + /// (issue #68). `maintain_ordered_top_index` keeps slots sorted descending + /// on decayed values at every write, so this reads only the requested + /// `[offset, offset + page_size)` range — O(page_size), no full-list scan + /// and no re-sort. Decay is applied per entry at read time. pub fn get_top_players(env: Env, offset: u32, page_size: u32) -> Vec { let count = Self::top_count(&env); if offset >= count || page_size == 0 { return vec![&env]; } let page_size = page_size.min(MAX_PAGE_SIZE); + let end = offset.saturating_add(page_size).min(count); let now = Self::current_epoch(&env); - let mut ranked: Vec = Vec::new(&env); - for i in 0..count { + let mut result = Vec::new(&env); + for i in offset..end { if let Some(mut entry) = Self::forward_entry(&env, i) { entry.points = Self::entry_points_now(&env, &entry); entry.epoch = now; - ranked.push_back(entry); + result.push_back(entry); } } - - // Selection sort, descending — bounded by MAX_TOP_PLAYERS. - let n = ranked.len() as u32; - for i in 0..n { - let mut max_idx = i; - for j in (i + 1)..n { - if ranked.get(j).unwrap().points > ranked.get(max_idx).unwrap().points { - max_idx = j; - } - } - if max_idx != i { - let a = ranked.get(i).unwrap(); - let b = ranked.get(max_idx).unwrap(); - ranked.set(i, b); - ranked.set(max_idx, a); - } - } - - let end = (offset + page_size).min(n); - let mut result = Vec::new(&env); - for i in offset..end { - result.push_back(ranked.get(i).unwrap()); - } result } @@ -942,7 +894,7 @@ impl LeaderboardContract { None => {} } Self::commit_stats(env, user, &s); - Self::update_top_players(env, user.clone(), s.points); + Self::maintain_ordered_top_index(env, user.clone(), s.points); env.storage().instance().extend_ttl(TTL_BUMP, TTL_HIGH); env.events().publish( (Symbol::new(&env, "leaderboard_updated"), user.clone()), @@ -955,7 +907,7 @@ impl LeaderboardContract { s.points += pts; s.bonus_bets += 1; // Issue #64: count bonus award without touching won/lost Self::commit_stats(env, user, &s); - Self::update_top_players(env, user.clone(), s.points); + Self::maintain_ordered_top_index(env, user.clone(), s.points); env.storage().instance().extend_ttl(TTL_BUMP, TTL_HIGH); env.events().publish( (Symbol::new(&env, "leaderboard_updated"), user.clone()), @@ -1251,12 +1203,16 @@ impl LeaderboardContract { } } - /// Insert or update a player's place in the top list after a point change. + /// Insert or update a player's place in the ordered top list after a + /// point change (issue #68). This is the single write path into the + /// index; every accrual entry point (reward, add_pts, reward_bonus, + /// add_bonus_pts, claim_pending_rewards) funnels through it, so the + /// slots read by get_top_players are always ordered. /// - Already listed: update points/epoch in place, bubble up, refresh min. /// - Not listed, room left: append and bubble. /// - Not listed, list full: evict the weakest live entry (decayed, oldest /// seq on ties) when the newcomer is at least as strong. - fn update_top_players(env: &Env, user: Address, new_points: u64) { + fn maintain_ordered_top_index(env: &Env, user: Address, new_points: u64) { let count = Self::ensure_consistent(env, Self::top_count(env)); if let Some((slot, mut entry)) = Self::top_slot_entry(env, &user) { @@ -1293,7 +1249,7 @@ impl LeaderboardContract { let min_slot: u32 = env.storage().instance().get(&DataKey::MinSlot).unwrap_or(0); let Some(min_entry) = Self::forward_entry(env, min_slot) else { Self::repair_top_index(env); - Self::update_top_players(env, user, new_points); + Self::maintain_ordered_top_index(env, user, new_points); return; }; if new_points < Self::entry_points_now(env, &min_entry) { diff --git a/leaderboard/src/tests.rs b/leaderboard/src/tests.rs index df6cbee..a7b3d93 100644 --- a/leaderboard/src/tests.rs +++ b/leaderboard/src/tests.rs @@ -134,6 +134,36 @@ fn test_top_players_capped_at_50() { assert_eq!(client.get_top_player_count(), 50); } +#[test] +fn test_pagination_reads_the_persistent_ordered_index() { + let (env, client, _admin, market, _referral) = setup(); + let points = [10_u64, 50, 30, 40, 20]; + + for points in points { + let user = Address::generate(&env); + client.add_pts(&market, &user, &points, &true); + } + + // The page is returned directly from slots 1 and 2 of the write-time + // ordered index, rather than rebuilding the complete ranking on read. + let page = client.get_top_players(&1_u32, &2_u32); + assert_eq!(page.len(), 2); + assert_eq!(page.get(0).unwrap().points, 40); + assert_eq!(page.get(1).unwrap().points, 30); +} + +#[test] +fn test_pagination_caps_page_size_without_overflowing_offset() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &100_u64, &true); + + // A caller cannot turn one view request into an unbounded storage read, + // and a maximal offset remains a safe empty page. + assert_eq!(client.get_top_players(&0_u32, &u32::MAX).len(), 1); + assert_eq!(client.get_top_players(&u32::MAX, &u32::MAX).len(), 0); +} + #[test] fn test_pagination_offset_beyond_count() { let (env, client, _admin, market, _referral) = setup(); @@ -777,6 +807,165 @@ fn test_stale_min_rejected_before_eviction() { assert_eq!(last.get(9).unwrap().points, 50); } +// ── Issue #68: the write-time ordered index ───────────────────────────────── +// These tests pin the invariants the pagination change relies on: slots are +// ordered at write time, the reverse lookup tracks every swap, and the min +// cache follows evictions — so get_top_players can read a page directly. + +#[test] +fn test_reverse_index_tracks_slots_after_bubbling() { + // Insert out of order so each write bubbles an entry upward; every + // player's TopPlayerSlot must agree with the slot get_top_players + // returns them in (no stale reverse keys after swaps). + let (env, client, _admin, market, _referral) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + let charlie = Address::generate(&env); + let dave = Address::generate(&env); + client.add_pts(&market, &alice, &10_u64, &true); + client.add_pts(&market, &bob, &50_u64, &true); + client.add_pts(&market, &charlie, &30_u64, &true); + client.add_pts(&market, &dave, &40_u64, &true); + + let top = client.get_top_players(&0_u32, &20_u32); + assert_eq!(top.len(), 4); + // [bob 50, dave 40, charlie 30, alice 10] + let expected = [ + (bob.clone(), 0u32), + (dave.clone(), 1u32), + (charlie.clone(), 2u32), + (alice.clone(), 3u32), + ]; + for (addr, slot) in expected { + let stored: Option = env.as_contract(&client.address, || { + env.storage() + .persistent() + .get(&DataKey::TopPlayerSlot(addr.clone())) + }); + assert_eq!( + stored, + Some(slot), + "reverse lookup for {:?} drifted from slot {slot}", + addr + ); + } +} + +#[test] +fn test_in_place_boost_rewrites_both_reverse_lookups() { + // Boosting a mid-list player to the top bubbles through every entry above + // them; each swap must rewrite both sides of the mapping. + let (env, client, _admin, market, _referral) = setup(); + let a = Address::generate(&env); + let b = Address::generate(&env); + let c = Address::generate(&env); + client.add_pts(&market, &a, &30_u64, &true); + client.add_pts(&market, &b, &20_u64, &true); + client.add_pts(&market, &c, &10_u64, &true); + + // Boost the weakest to the top. + client.add_pts(&market, &c, &100_u64, &true); + + let top = client.get_top_players(&0_u32, &20_u32); + assert_eq!(top.get(0).unwrap().address, c); + assert_eq!(top.get(1).unwrap().address, a); + assert_eq!(top.get(2).unwrap().address, b); + + let slot_of = |env: &Env, addr: &Address| -> Option { + env.as_contract(&client.address, || { + env.storage() + .persistent() + .get(&DataKey::TopPlayerSlot(addr.clone())) + }) + }; + assert_eq!(slot_of(&env, &c), Some(0)); + assert_eq!(slot_of(&env, &a), Some(1)); + assert_eq!(slot_of(&env, &b), Some(2)); +} + +#[test] +fn test_eviction_refreshes_min_cache_and_reverse_mapping() { + // Fill the board with strictly descending points (no bubble), evict the + // weakest with a top scorer, and verify MinPoints/MinSlot now describe + // the new weakest entry while the evicted player's reverse key is gone. + let (env, client, _admin, market, _referral) = setup(); + for i in 0u64..MAX_TOP_PLAYERS as u64 { + let user = Address::generate(&env); + client.add_pts(&market, &user, &(1000 - i), &true); + } + let weakest = client + .get_top_players(&(MAX_TOP_PLAYERS - 1), &1) + .get(0) + .unwrap() + .address + .clone(); + assert_eq!(client.get_min_points(), 951); + + let newcomer = Address::generate(&env); + client.add_pts(&market, &newcomer, &5000_u64, &true); + + assert_eq!(client.get_rank(&weakest), UNRANKED_RANK); + let still_mapped = env.as_contract(&client.address, || { + env.storage() + .persistent() + .has(&DataKey::TopPlayerSlot(weakest.clone())) + }); + assert!(!still_mapped, "evicted player must lose their reverse mapping"); + + // The min cache now tracks the weakest survivor at the last slot. + assert_eq!(client.get_min_slot(), MAX_TOP_PLAYERS - 1); + let tail = client.get_top_players(&(MAX_TOP_PLAYERS - 1), &1); + assert_eq!(tail.get(0).unwrap().points, client.get_min_points()); +} + +#[test] +fn test_pagination_pages_are_contiguous_and_gap_free() { + // Interleaved points so insertion order != rank order. Paging through + // the index with a small page must reconstruct the exact same descending + // list with no gaps and no duplicates. + let (env, client, _admin, market, _referral) = setup(); + for i in 0u64..35 { + let user = Address::generate(&env); + client.add_pts(&market, &user, &(i * 11 % 35 + 1), &true); + } + assert_eq!(client.get_top_player_count(), 35); + + let mut seen: soroban_sdk::Vec = soroban_sdk::vec![&env]; + let mut offset = 0u32; + loop { + let page = client.get_top_players(&offset, &7_u32); + if page.len() == 0 { + break; + } + for entry in page.iter() { + seen.push_back(entry.clone()); + } + offset += 7; + if offset >= client.get_top_player_count() { + break; + } + } + + assert_eq!(seen.len(), 35, "paging must visit every ranked player"); + + // No duplicates, and the concatenated pages are one descending list. + for i in 0..seen.len() { + let addr = seen.get(i).unwrap().address.clone(); + let dupes = seen + .iter() + .filter(|e| e.address == addr) + .count(); + assert_eq!(dupes, 1, "paging must not duplicate a player"); + } + + let mut previous = u64::MAX; + for entry in seen.iter() { + let pts = client.get_points(&entry.address); + assert!(pts <= previous, "page boundary broke the descending order"); + previous = pts; + } +} + #[test] fn test_add_pts_emits_leaderboard_updated() { let (env, client, _admin, market, _referral) = setup(); @@ -793,16 +982,3 @@ fn test_add_pts_emits_leaderboard_updated() { let name = Symbol::try_from_val(&env, &topic0).unwrap(); assert_eq!(name, Symbol::new(&env, "leaderboard_updated")); } - -#[test] -fn test_add_pts_always_rejected() { - let (env, client, _admin, market, _referral) = setup(); - let user = Address::generate(&env); - let rando = Address::generate(&env); - let result = client.add_pts(&rando, &user, &10_u64, &true); - assert!(result.is_err(), "add_pts should always return an error"); - match result { - Err(LeaderboardError::UnauthorizedCaller) => {} - other => panic!("add_pts returned unexpected error: {:?}", other), - } -} diff --git a/leaderboard/src/ttl_tests.rs b/leaderboard/src/ttl_tests.rs index 7834b2a..02037d9 100644 --- a/leaderboard/src/ttl_tests.rs +++ b/leaderboard/src/ttl_tests.rs @@ -22,6 +22,11 @@ fn setup() -> ( let env = Env::default(); env.mock_all_auths(); env.cost_estimate().budget().reset_unlimited(); + // The write-time ordered-index bubble (issue #68) can rewrite tens of + // slots in one call, exceeding mainnet invocation limits for the + // fill-to-capacity cases. Behavior is what these tests prove, so lift the + // resource limits like the CPU budget above (same as tests.rs setup). + env.cost_estimate().disable_resource_limits(); let contract_id = env.register(LeaderboardContract, ()); let client = LeaderboardContractClient::new(&env, &contract_id); @@ -162,8 +167,11 @@ fn test_min_points_and_min_slot_survive_ttl_refresh_cycle() { newcomer, "a fresh score must lead a list of decayed incumbents" ); + // Page reads are bounded by MAX_PAGE_SIZE (issue #68), so fetch the + // weakest slot on its own page instead of one 50-wide read. + let tail = client.get_top_players(&(MAX_TOP_PLAYERS - 1), &1); assert_eq!( - top.get(MAX_TOP_PLAYERS - 1).unwrap().points, + tail.get(0).unwrap().points, client.get_min_points(), "min cache must agree with the weakest ranked entry" );