diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..904ef62 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,27 @@ +name: CI + +# Scoped to `leaderboard` only for now, not the whole workspace. `cargo test +# --workspace` currently fails for reasons that have nothing to do with any +# one PR: `referral_registry/src/lib.rs` was emptied by a bad merge (see +# issue tracker / PR #185 discussion) and `prediction_market`'s test suite +# depends on it as a dev-dependency. Gate the whole workspace here once that +# is repaired — until then a workspace-wide job would be red on every PR +# regardless of what it actually changes, which trains everyone to ignore CI +# rather than trust it. + +on: + push: + branches: [main] + pull_request: + +jobs: + test-leaderboard: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # No explicit toolchain action: ubuntu-latest ships rustup, and it + # picks up this repo's rust-toolchain.toml (channel = "1.91.0") + # automatically on the first cargo invocation. + - run: rustup show + - uses: Swatinem/rust-cache@v2 + - run: cargo test -p leaderboard --verbose diff --git a/leaderboard/src/lib.rs b/leaderboard/src/lib.rs index 5a2135f..5fca2b0 100644 --- a/leaderboard/src/lib.rs +++ b/leaderboard/src/lib.rs @@ -279,6 +279,42 @@ impl LeaderboardContract { Ok(()) } + // ── Issue #24: an explicit way to reduce a player's points ─────────────── + // A loss no longer has to be point-positive. This is decay-aware (the + // deduction lands on the player's current, decayed score, not a stale + // stored one) and saturates at zero rather than underflowing. Deliberately + // scoped to the market contract only, mirroring reward()/add_pts() — the + // caller who can award points is the one trusted to take them away. + // + // Safe with respect to the top-list invariants: get_top_players and + // get_rank already recompute rank from live (decayed) values on every + // call instead of trusting stored order (see entry_points_now + + // selection-sort in get_top_players, and the exhaustive scan in + // get_rank), and recompute_min is an unconditional full scan. So a score + // moving down through update_top_players' existing "already listed" + // branch is exactly as safe as it moving up: bubble_up simply no-ops when + // the entry no longer beats its predecessor, and the cached min is + // recomputed correctly regardless of direction. A penalized player who + // isn't currently ranked never touches the top list at all — see + // debit_points — so a penalty can never insert an unranked player into + // the leaderboard or bump a ranked one out on their behalf. + pub fn penalize( + env: Env, + caller: Address, + user: Address, + pts: u64, + ) -> Result<(), LeaderboardError> { + Self::require_not_paused(&env)?; + Self::require_market_contract(&env, &caller)?; + caller.require_auth(); + if pts == 0 { + return Err(LeaderboardError::InvalidPoints); + } + Self::require_not_banned(&env, &user)?; + Self::debit_points(&env, &user, pts); + Ok(()) + } + // ── Pull-based reward flow (issue #86) ─────────────────────────────────── /// Queue a settled-bet reward for later claim. A banned player is rejected @@ -412,39 +448,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(()) } @@ -963,6 +966,31 @@ impl LeaderboardContract { ); } + /// Reduce a player's (decay-forwarded) points by `pts`, saturating at + /// zero. Deliberately does not touch won_bets/lost_bets/bonus_bets — + /// those are activity counters, not points, and the loss itself is + /// already recorded by whichever add_pts/reward call reported it. + /// + /// Only reconciles the top list if the player is currently ranked. An + /// unranked player's Stats just get a lower number; penalizing them must + /// never be the reason they newly appear in (or displace someone from) + /// the top list, so update_top_players is skipped entirely when they + /// aren't already in it — mirroring how a penalty can't create rank, only + /// remove it. + fn debit_points(env: &Env, user: &Address, pts: u64) { + let mut s = Self::stats_for_update(env, user); + s.points = s.points.saturating_sub(pts); + Self::commit_stats(env, user, &s); + if Self::top_slot_entry(env, user).is_some() { + Self::update_top_players(env, user.clone(), s.points); + } + env.storage().instance().extend_ttl(TTL_BUMP, TTL_HIGH); + env.events().publish( + (Symbol::new(&env, "leaderboard_penalized"), user.clone()), + s.points, + ); + } + fn accumulate_pending( env: &Env, user: &Address, @@ -1321,4 +1349,6 @@ mod tests; #[cfg(test)] mod ttl_tests; #[cfg(test)] -mod admin_tests; \ No newline at end of file +mod admin_tests; +#[cfg(test)] +mod penalty_tests; \ No newline at end of file diff --git a/leaderboard/src/penalty_tests.rs b/leaderboard/src/penalty_tests.rs new file mode 100644 index 0000000..6640d01 --- /dev/null +++ b/leaderboard/src/penalty_tests.rs @@ -0,0 +1,262 @@ +// ── Issue #24: an explicit way to reduce points ─────────────────────────────── +// +// Decay (issue #69, see decay_tests.rs) fixes the *staleness* half of #24: an +// idle leader's score shrinks over time, so an active newcomer can catch up +// without ever out-earning them in absolute terms. It does not touch the +// other half of the complaint — "no penalty for losses (losers still gain +// LOSE_POINTS)" — because decay only ever erodes a score passively, on a +// schedule; nothing in that model lets a caller take points away for a +// specific event (a loss) the moment it happens. +// +// penalize() is that missing primitive. These tests prove it does the one +// thing it's supposed to (move a score down, decay-aware, saturating at +// zero) and, just as importantly, prove it *doesn't* do anything it isn't +// supposed to: it must never be the reason an unranked player enters the top +// list, and it must never touch the activity counters (won/lost/bonus) that +// a separate add_pts/reward call already recorded. + +use super::*; +use soroban_sdk::{ + testutils::{Address as _, Events, Ledger as _}, + Env, Symbol, TryFromVal, Val, +}; + +fn setup() -> ( + Env, + LeaderboardContractClient<'static>, + Address, + Address, + Address, +) { + let env = Env::default(); + env.mock_all_auths(); + env.cost_estimate().budget().reset_unlimited(); + + let contract_id = env.register(LeaderboardContract, ()); + let client = LeaderboardContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let market = Address::generate(&env); + let referral = Address::generate(&env); + + client.initialize(&admin, &market, &referral); + (env, client, admin, market, referral) +} + +/// Move the ledger forward by whole decay periods (same helper as +/// decay_tests.rs — kept local so this file has no cross-module dependency). +fn advance_periods(env: &Env, periods: u32) { + let seq = env.ledger().sequence(); + env.ledger() + .set_sequence_number(seq + periods * DECAY_PERIOD_LEDGERS); +} + +#[test] +fn test_penalize_reduces_points() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &100_u64, &true); + client.penalize(&market, &user, &30_u64); + assert_eq!(client.get_points(&user), 70); +} + +#[test] +fn test_penalize_saturates_at_zero_instead_of_underflowing() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &10_u64, &true); + // Deducting more than the balance must floor to 0, not panic or wrap. + client.penalize(&market, &user, &1_000_u64); + assert_eq!(client.get_points(&user), 0); +} + +#[test] +fn test_penalize_a_never_credited_player_stays_at_zero() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + // Never called add_pts/reward for this user at all. + client.penalize(&market, &user, &50_u64); + assert_eq!(client.get_points(&user), 0); +} + +#[test] +fn test_penalize_rejects_non_market_caller() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + let rando = Address::generate(&env); + client.add_pts(&market, &user, &50_u64, &true); + let result = client.try_penalize(&rando, &user, &10_u64); + assert!(result.is_err(), "penalize should reject a non-market caller"); + // The player's balance must be untouched by the rejected call. + assert_eq!(client.get_points(&user), 50); +} + +#[test] +fn test_penalize_rejects_zero_points() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &50_u64, &true); + let result = client.try_penalize(&market, &user, &0_u64); + assert!(result.is_err(), "penalize should reject a zero-point deduction"); +} + +#[test] +fn test_penalize_rejects_banned_player() { + let (env, client, admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &50_u64, &true); + client.ban_player(&admin, &user); + let result = client.try_penalize(&market, &user, &10_u64); + assert!(result.is_err(), "penalize must reject a banned player like every other accrual path"); +} + +#[test] +fn test_penalize_rejects_while_paused() { + let (env, client, admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &50_u64, &true); + client.pause(&admin); + let result = client.try_penalize(&market, &user, &10_u64); + assert!(result.is_err(), "penalize must respect the pause switch"); +} + +#[test] +fn test_penalize_does_not_touch_activity_counters() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &100_u64, &true); // 1 win + client.add_pts(&market, &user, &20_u64, &false); // 1 loss + let before = client.get_stats(&user); + + client.penalize(&market, &user, &15_u64); + + let after = client.get_stats(&user); + assert_eq!(after.points, before.points - 15); + assert_eq!(after.won_bets, before.won_bets, "penalize must not touch won_bets"); + assert_eq!(after.lost_bets, before.lost_bets, "penalize must not touch lost_bets"); + assert_eq!(after.total_bets, before.total_bets, "penalize must not touch total_bets"); +} + +#[test] +fn test_penalize_is_decay_aware() { + // The deduction must land on the player's *current* (decayed) score, not + // the stale value that was last written — otherwise a penalty applied + // long after the fact would double-count decay that already happened. + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &1_000_u64, &true); + + advance_periods(&env, 3); + let decayed_before_penalty = client.get_points(&user); + assert!(decayed_before_penalty < 1_000, "score should have decayed by now"); + + client.penalize(&market, &user, &50_u64); + + assert_eq!(client.get_points(&user), decayed_before_penalty - 50); +} + +#[test] +fn test_penalize_drops_a_leader_below_a_weaker_ranked_player() { + let (env, client, _admin, market, _referral) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.add_pts(&market, &alice, &100_u64, &true); + client.add_pts(&market, &bob, &60_u64, &true); + assert_eq!(client.get_rank(&alice), 1); + assert_eq!(client.get_rank(&bob), 2); + + client.penalize(&market, &alice, &50_u64); // alice: 100 -> 50, now behind bob's 60 + + assert_eq!(client.get_points(&alice), 50); + assert_eq!(client.get_rank(&bob), 1, "bob should now lead"); + assert_eq!(client.get_rank(&alice), 2); + + let top = client.get_top_players(&0_u32, &2_u32); + assert_eq!(top.get(0).unwrap().address, bob); + assert_eq!(top.get(1).unwrap().address, alice); +} + +#[test] +fn test_penalize_min_cache_stays_correct_after_reordering() { + 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, &300_u64, &true); + client.add_pts(&market, &b, &200_u64, &true); + client.add_pts(&market, &c, &100_u64, &true); + // The min cache is only rigorously maintained once the list has been + // full at least once (or an existing entry has been updated) — with 3 + // of 50 slots filled nothing has forced a recompute yet, so prime it via + // the permissionless keeper, same as a real integrator would. + client.reconcile_top_slots(); + assert_eq!(client.get_min_points(), 100); + + // Penalize the current leader below the current minimum. The min cache + // must be recomputed to reflect the new weakest entry, not left stale. + client.penalize(&market, &a, &250_u64); // a: 300 -> 50, now the weakest + + assert_eq!(client.get_points(&a), 50); + assert_eq!(client.get_min_points(), 50, "min cache must track the new weakest entry"); +} + +#[test] +fn test_penalize_never_inserts_an_unranked_player_into_the_top_list() { + let (env, client, _admin, market, _referral) = setup(); + let ranked = Address::generate(&env); + let never_ranked = Address::generate(&env); + client.add_pts(&market, &ranked, &50_u64, &true); + let count_before = client.get_top_player_count(); + + // never_ranked has no Stats record and is not in the top list. Penalizing + // them must not be the reason they newly appear in it — there is plenty + // of room (the list is nowhere near MAX_TOP_PLAYERS), so the ordinary + // "insert if there's room" path in update_top_players would happily add + // them if it ever ran for this call, which is exactly what must not + // happen for a penalty. + client.penalize(&market, &never_ranked, &10_u64); + + assert_eq!(client.get_top_player_count(), count_before); + assert_eq!(client.get_rank(&never_ranked), UNRANKED_RANK); +} + +#[test] +fn test_penalize_never_evicts_a_ranked_player_on_an_unranked_players_behalf() { + // Same guarantee as above, but with the list full: penalizing an + // unranked player must not trigger the "full list, evict the weakest" + // branch either. + 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); + } + assert_eq!(client.get_top_player_count(), MAX_TOP_PLAYERS); + let min_before = client.get_min_points(); + + let never_ranked = Address::generate(&env); + client.penalize(&market, &never_ranked, &10_u64); + + assert_eq!(client.get_top_player_count(), MAX_TOP_PLAYERS); + assert_eq!(client.get_min_points(), min_before, "a penalty on an outsider must not touch the list"); + assert_eq!(client.get_rank(&never_ranked), UNRANKED_RANK); +} + +#[test] +fn test_penalize_emits_leaderboard_penalized_event() { + let (env, client, _admin, market, _referral) = setup(); + let user = Address::generate(&env); + client.add_pts(&market, &user, &100_u64, &true); + client.penalize(&market, &user, &40_u64); + + // Same event-inspection pattern as + // tests::test_add_pts_emits_leaderboard_updated — `env.events().all()` + // returns a `ContractEvents` in soroban-sdk 26, exposed as an XDR slice + // rather than an indexable Vec of (address, topics, data) tuples. + let events = env.events().all(); + let emitted = events.events(); + assert!(!emitted.is_empty(), "penalize emitted no event"); + let soroban_sdk::xdr::ContractEventBody::V0(body) = &emitted.last().unwrap().body; + let topic0 = Val::try_from_val(&env, &body.topics[0]).unwrap(); + let name = Symbol::try_from_val(&env, &topic0).unwrap(); + assert_eq!(name, Symbol::new(&env, "leaderboard_penalized")); +} diff --git a/leaderboard/src/tests.rs b/leaderboard/src/tests.rs index df6cbee..3047158 100644 --- a/leaderboard/src/tests.rs +++ b/leaderboard/src/tests.rs @@ -795,14 +795,10 @@ fn test_add_pts_emits_leaderboard_updated() { } #[test] -fn test_add_pts_always_rejected() { +fn test_add_pts_rejects_non_market_caller() { 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), - } + let result = client.try_add_pts(&rando, &user, &10_u64, &true); + assert!(result.is_err(), "add_pts should reject a non-market caller"); } diff --git a/leaderboard/src/ttl_tests.rs b/leaderboard/src/ttl_tests.rs index 7834b2a..941ea6e 100644 --- a/leaderboard/src/ttl_tests.rs +++ b/leaderboard/src/ttl_tests.rs @@ -156,14 +156,18 @@ fn test_min_points_and_min_slot_survive_ttl_refresh_cycle() { client.get_min_points() >= weakest, "min cache regressed after eviction" ); - let top = client.get_top_players(&0_u32, &MAX_TOP_PLAYERS); + // get_top_players pages are capped at MAX_PAGE_SIZE (20), so a single + // call cannot return all 50 slots — fetch the head and tail pages + // separately, same as the other tests in this suite do. + let head = client.get_top_players(&0_u32, &1_u32); assert_eq!( - top.get(0).unwrap().address, + head.get(0).unwrap().address, newcomer, "a fresh score must lead a list of decayed incumbents" ); + let tail = client.get_top_players(&(MAX_TOP_PLAYERS - 20), &20_u32); assert_eq!( - top.get(MAX_TOP_PLAYERS - 1).unwrap().points, + tail.get(19).unwrap().points, client.get_min_points(), "min cache must agree with the weakest ranked entry" ); diff --git a/prediction_market/src/lib.rs b/prediction_market/src/lib.rs index 036f7db..b093f3b 100644 --- a/prediction_market/src/lib.rs +++ b/prediction_market/src/lib.rs @@ -467,14 +467,18 @@ impl PredictionMarketContract { .instance() .set(&DataKey::PinnedHashes, &pending.hashes); env.storage().instance().remove(&DataKey::PendingConfig); + // Matches the documented event schema at the top of this file: + // config_changed (admin) Config. `caller` is the governor who + // executed the change (the only actor actually in scope here); + // `pending.cfg` is the live Config, matching the documented data + // type. This replaces two merge-corrupted publishes that used to sit + // here: a duplicate "cfg_act" event, and this event referencing + // `admin`/`token_contract`/`referral_contract`/`leaderboard_contract` + // /`xlm_sac` — none of which are in scope in this function. env.events().publish( - (Symbol::new(&env, "cfg_act"), caller), + (Symbol::new(&env, "config_changed"), caller), pending.cfg, ); - env.events().publish( - (Symbol::new(&env, "config_changed"), admin), - (token_contract, referral_contract, leaderboard_contract, xlm_sac), - ); Ok(()) } @@ -595,6 +599,8 @@ impl PredictionMarketContract { .instance() .get(&DataKey::GovernorCount) .unwrap_or(0) + } + /// The cross-contract ABI version this deployment implements (issue #84). pub fn interface_version(_env: Env) -> u32 { INTERFACE_VERSION @@ -1618,15 +1624,19 @@ impl PredictionMarketContract { Self::ensure_fee_ledger_migrated(&env); } - /// Remaining TTL (ledgers) of the Market key. 0 means missing/expired — - /// integrators can warn before funds become unrecoverable (issue #54). - pub fn get_market_ttl(env: Env, market_id: u64) -> u32 { - let key = DataKey::Market(market_id); - if !env.storage().persistent().has(&key) { - return 0; - } - env.storage().persistent().get_ttl(&key) - } + // get_market_ttl (issue #54) removed: it called + // env.storage().persistent().get_ttl(&key), which only exists on + // soroban_sdk::testutils::storage::Persistent — backed by + // env.host().get_contract_data_live_until_ledger(...), a local + // test-sandbox introspection capability, not something a real deployed + // contract can call on a live network. This function could not compile + // for a production build; there is no supported way to query another + // key's remaining TTL from within contract code in this SDK version. A + // real fix would need the contract to track its own expected expiry + // (e.g. record target_ledger = current_sequence + extend_to on every + // extend_ttl of Market-related keys) — new state and its own tests, not + // a merge repair. Left for a dedicated follow-up if this integrator + // safety feature is still wanted. /// Permissionless keeper: anyone may pay to extend this market's /// Market/Bet/Payout/bettor-index keys. Does not resurrect expired entries. diff --git a/prediction_market/src/tests.rs b/prediction_market/src/tests.rs index 8cc0965..d5aaa09 100644 --- a/prediction_market/src/tests.rs +++ b/prediction_market/src/tests.rs @@ -1550,15 +1550,14 @@ fn test_cancel_refund_rebumps_ttl_entries() { assert!(ttl(&market_key) > market_before); } -// ── #54: permissionless refresh + per-market expiry tracking + migration ───── - -#[test] -fn test_get_market_ttl_tracks_live_entry() { - let t = setup(); - assert_eq!(t.client.get_market_ttl(&99_u64), 0); - let id = create_test_market(&t); - assert!(t.client.get_market_ttl(&id) >= TTL_BUMP); -} +// ── #54: permissionless refresh + expiry tracking + migration ──────────────── +// +// get_market_ttl (a public read of another key's remaining TTL) was removed: +// it called an SDK method that only exists in the local test sandbox, not on +// a real deployed contract — see the comment at its former call site in +// lib.rs. These tests read TTL directly via the same +// testutils::storage::Persistent mechanism the contract itself cannot use in +// production, which is exactly why that was never a viable public API. #[test] fn test_refresh_market_ttl_rebumps_bet_and_market() { @@ -1584,7 +1583,6 @@ fn test_refresh_market_ttl_rebumps_bet_and_market() { assert_eq!(t.client.refresh_market_ttl(&id), 1); assert!(ttl(&bet_key) > bet_before); assert!(ttl(&market_key) > market_before); - assert!(t.client.get_market_ttl(&id) > market_before); } #[test] @@ -1597,12 +1595,18 @@ fn test_refresh_markets_migrates_existing_entries() { t.client.place_bet(&user, &a, &true, &100_0000000_i128); t.client.place_bet(&user, &b, &true, &100_0000000_i128); + let market_contract = t.client.address.clone(); + let ttl = |key: &DataKey| -> u32 { + t.env + .as_contract(&market_contract, || t.env.storage().persistent().get_ttl(key)) + }; + advance_ledgers(&t.env, 6_000_000); - let before_a = t.client.get_market_ttl(&a); + let before_a = ttl(&DataKey::Market(a)); let bumped = t.client.refresh_markets(&1_u64, &20_u32); assert_eq!(bumped, 2); - assert!(t.client.get_market_ttl(&a) > before_a); - assert!(t.client.get_market_ttl(&b) >= TTL_BUMP); + assert!(ttl(&DataKey::Market(a)) > before_a); + assert!(ttl(&DataKey::Market(b)) >= TTL_BUMP); } #[test]