feat: implement persistent storage migration for player profile#705
Open
Fayvor22 wants to merge 2 commits into
Open
feat: implement persistent storage migration for player profile#705Fayvor22 wants to merge 2 commits into
Fayvor22 wants to merge 2 commits into
Conversation
|
@Fayvor22 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds persistent on-chain player profiles to the game contract, updates profiles across settlement flows (claim_draw, claim_win, forfeit, claim_timeout_win, resolve_dispute) with fixed rating deltas (+32 win, -16 loss, +8 draw), and exposes APIs to query, set, and paginate profiles; includes tests. Changes
Sequence DiagramsequenceDiagram
participant Player as Player
participant Settlement as Game Settlement
participant ProfileLogic as Player Profile Logic
participant Storage as Persistent Storage
Player->>Settlement: submit settlement action (claim_win/draw/forfeit/timeout/dispute)
Settlement->>ProfileLogic: request profile updates for involved players
ProfileLogic->>Storage: fetch profile(s) by Address
Storage-->>ProfileLogic: return existing or default profile(s)
ProfileLogic->>ProfileLogic: apply outcome deltas (+32/-16/+8), update counters, highest_rating, last_updated
ProfileLogic->>Storage: write updated profile(s)
Storage-->>ProfileLogic: persist confirmation
ProfileLogic-->>Settlement: return updated profile(s)
Settlement-->>Player: finalize settlement (including profile updates)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/game_contract/src/lib.rs (1)
3309-3343: Add missing edge-case tests forset_player_rating.Current coverage validates only the happy path. Please add tests for unauthorized caller and negative rating rejection (
InvalidAmount) to fully cover the API contract.✅ Suggested test additions
+ #[test] + fn test_set_player_rating_rejects_non_admin() { + let env = Env::default(); + env.mock_all_auths(); + // ... setup omitted for brevity (same as existing test) + let not_admin = Address::generate(&env); + let result = client.try_set_player_rating(¬_admin, &player1, &1500i32); + assert_eq!(result, Err(Ok(ContractError::Unauthorized))); + } + + #[test] + fn test_set_player_rating_rejects_negative_rating() { + let env = Env::default(); + env.mock_all_auths(); + // ... setup omitted for brevity (same as existing test) + let result = client.try_set_player_rating(&admin, &player1, &-1i32); + assert_eq!(result, Err(Ok(ContractError::InvalidAmount))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/src/lib.rs` around lines 3309 - 3343, Add two edge-case tests for set_player_rating: one that calls GameContractClient::set_player_rating from a non-admin address and asserts the call fails with an authorization error (unauthorized caller), and another that attempts to set a negative rating (e.g., -1) and asserts the contract returns the InvalidAmount error; locate usage around client.set_player_rating and client.get_player_profile in the test module and mirror the existing test setup (initialize_token, initialize_puzzle_rewards, mint) but change the caller and rating value and assert the expected failure results.
🤖 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/game_contract/src/lib.rs`:
- Around line 1874-1900: The code currently loads and writes a single
Map<Address, PlayerRating> under PLAYER_PROFILES which serializes the entire map
on each write; replace this pattern with per-player persistent keys so you only
read/write a single PlayerRating for a given player. Specifically, stop using
Map<Address, PlayerRating> profiles and calls like
env.storage().persistent().get(&PLAYER_PROFILES) / set(&PLAYER_PROFILES,
&profiles); instead derive a unique key per player (e.g., combine
PLAYER_PROFILES with the player's Address) and implement
get_player_profile(player) and set_player_profile(player, profile) helpers that
call env.storage().persistent().get(key) and set(key, profile) to load/update
only that player’s record; apply the same change for the other occurrences
referenced (around the PlayerRating create/update sites and any functions
handling pagination) so you avoid full-map serialization and linear scans.
- Around line 121-126: The constant PLAYER_PROFILES uses
symbol_short!("PLAYER_PROF") which exceeds the 9-character limit; change the
symbol to a <=9-character identifier (e.g., "PLAYERPF" or "PLRPROF") in the
PLAYER_PROFILES declaration and update any code that references PLAYER_PROFILES
or that exact symbol string so the storage key remains consistent; locate the
PLAYER_PROFILES constant and replace the symbol_short! argument with the shorter
string and run a compile to ensure no other references use the old 11-char name.
- Around line 1950-1963: The current addition profile.rating + rating_delta can
overflow an i32 before .max(0) is applied; change the update to perform the
arithmetic in a wider integer (e.g., cast profile.rating and rating_delta to
i64), compute let new_rating_i64 = (profile.rating as i64) + (rating_delta as
i64), clamp it to the valid range 0..=i32::MAX, then assign profile.rating =
new_rating_i64 as i32. Update the block that currently computes rating_delta and
sets profile.rating, and ensure this logic is used alongside set_player_rating
and anywhere profile.rating is mutated to avoid overflow/wraparound.
---
Nitpick comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 3309-3343: Add two edge-case tests for set_player_rating: one that
calls GameContractClient::set_player_rating from a non-admin address and asserts
the call fails with an authorization error (unauthorized caller), and another
that attempts to set a negative rating (e.g., -1) and asserts the contract
returns the InvalidAmount error; locate usage around client.set_player_rating
and client.get_player_profile in the test module and mirror the existing test
setup (initialize_token, initialize_puzzle_rewards, mint) but change the caller
and rating value and assert the expected failure results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30981581-5b18-4395-b378-99dc6c703e40
📒 Files selected for processing (1)
contracts/game_contract/src/lib.rs
Comment on lines
+121
to
+126
| // Persistent Player Profiles (#521) – PERSISTENT storage for long-term availability | ||
| // This key stores all player profile data in Persistent storage rather than Instance storage. | ||
| // This ensures that player statistics and ratings survive contract upgrades and are permanently | ||
| // available on-chain. Each player is keyed by their Address. | ||
| const PLAYER_PROFILES: Symbol = symbol_short!("PLAYER_PROF"); // Map<Address, PlayerProfile> in Persistent storage | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum character length allowed by soroban_sdk's symbol_short! macro?
💡 Result:
The soroban_sdk::symbol_short! macro allows symbol string literals of maximum 9 characters. This is stated in the official soroban_sdk docs for the macro (“A short symbol’s maximum length is 9 characters”).
Citations:
symbol_short! macro requires symbol ≤9 characters; "PLAYER_PROF" has 11.
Line 125 uses symbol_short!("PLAYER_PROF"), which exceeds the 9-character maximum enforced by the soroban_sdk macro and will fail compilation.
Proposed fix
-const PLAYER_PROFILES: Symbol = symbol_short!("PLAYER_PROF"); // Map<Address, PlayerProfile> in Persistent storage
+const PLAYER_PROFILES: Symbol = symbol_short!("PLYRPROF"); // Map<Address, PlayerRating> in Persistent storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 121 - 126, The constant
PLAYER_PROFILES uses symbol_short!("PLAYER_PROF") which exceeds the 9-character
limit; change the symbol to a <=9-character identifier (e.g., "PLAYERPF" or
"PLRPROF") in the PLAYER_PROFILES declaration and update any code that
references PLAYER_PROFILES or that exact symbol string so the storage key
remains consistent; locate the PLAYER_PROFILES constant and replace the
symbol_short! argument with the shorter string and run a compile to ensure no
other references use the old 11-char name.
Comment on lines
+1874
to
+1900
| let mut profiles: Map<Address, PlayerRating> = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&PLAYER_PROFILES) | ||
| .unwrap_or(Map::new(env)); | ||
|
|
||
| // Return existing profile if it exists | ||
| if let Some(profile) = profiles.get(player.clone()) { | ||
| return profile; | ||
| } | ||
|
|
||
| // Create new profile with standard starting rating (1200 in chess) | ||
| let new_profile = PlayerRating { | ||
| address: player.clone(), | ||
| rating: 1200, | ||
| games_played: 0, | ||
| wins: 0, | ||
| losses: 0, | ||
| draws: 0, | ||
| highest_rating: 1200, | ||
| last_updated: env.ledger().sequence() as u64, | ||
| }; | ||
|
|
||
| profiles.set(player.clone(), new_profile.clone()); | ||
| env.storage() | ||
| .persistent() | ||
| .set(&PLAYER_PROFILES, &profiles); |
Contributor
There was a problem hiding this comment.
Single-map persistent storage will scale poorly in gas/CPU.
Storing all profiles in one Map<Address, PlayerRating> under one key causes full-map serialization on each profile write and linear scans for pagination. This will get expensive quickly as player count grows.
♻️ Refactor direction (per-player keys)
+#[contracttype]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum PersistentKey {
+ PlayerProfile(Address),
+}- let mut profiles: Map<Address, PlayerRating> = env.storage().persistent().get(&PLAYER_PROFILES).unwrap_or(Map::new(env));
- let mut profile = Self::initialize_player_profile(env, player);
+ let key = PersistentKey::PlayerProfile(player.clone());
+ let mut profile: PlayerRating = env.storage().persistent().get(&key).unwrap_or(PlayerRating {
+ address: player.clone(),
+ rating: 1200,
+ games_played: 0,
+ wins: 0,
+ losses: 0,
+ draws: 0,
+ highest_rating: 1200,
+ last_updated: env.ledger().sequence() as u64,
+ });
...
- profiles.set(player.clone(), profile);
- env.storage().persistent().set(&PLAYER_PROFILES, &profiles);
+ env.storage().persistent().set(&key, &profile);Also applies to: 1938-1943, 1972-1975, 2005-2024, 2038-2057
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1874 - 1900, The code
currently loads and writes a single Map<Address, PlayerRating> under
PLAYER_PROFILES which serializes the entire map on each write; replace this
pattern with per-player persistent keys so you only read/write a single
PlayerRating for a given player. Specifically, stop using Map<Address,
PlayerRating> profiles and calls like
env.storage().persistent().get(&PLAYER_PROFILES) / set(&PLAYER_PROFILES,
&profiles); instead derive a unique key per player (e.g., combine
PLAYER_PROFILES with the player's Address) and implement
get_player_profile(player) and set_player_profile(player, profile) helpers that
call env.storage().persistent().get(key) and set(key, profile) to load/update
only that player’s record; apply the same change for the other occurrences
referenced (around the PlayerRating create/update sites and any functions
handling pagination) so you avoid full-map serialization and linear scans.
Comment on lines
+1950
to
+1963
| let rating_delta = if is_draw { | ||
| profile.draws += 1; | ||
| 8 // Draw: +8 rating | ||
| } else if is_win { | ||
| profile.wins += 1; | ||
| 32 // Win: +32 rating | ||
| } else { | ||
| profile.losses += 1; | ||
| -16 // Loss: -16 rating | ||
| }; | ||
|
|
||
| // Update rating (ensure it doesn't go below 0) | ||
| profile.rating = (profile.rating + rating_delta).max(0); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current arithmetic site and admin-settable rating path
rg -n "profile\.rating = \(profile\.rating \+ rating_delta\)\.max\(0\);" contracts/game_contract/src/lib.rs
rg -n "pub fn set_player_rating\(" contracts/game_contract/src/lib.rs -A30Repository: NOVUS-X/XLMate
Length of output: 1189
🏁 Script executed:
#!/bin/bash
# Check bounds of rating system and any other protections
rg -n "rating" contracts/game_contract/src/lib.rs | head -40
# Look for PlayerRating struct definition to understand rating field type
rg -n "struct PlayerRating" contracts/game_contract/src/lib.rs -A10
# Check if there are any other places where rating is updated
rg -n "profile\.rating\s*=" contracts/game_contract/src/lib.rs -B2 -A2Repository: NOVUS-X/XLMate
Length of output: 2943
i32 rating arithmetic can overflow before clamping.
Line 1962 adds rating_delta directly to i32. The set_player_rating function allows admins to set ratings up to i32::MAX without an upper bound check. If rating approaches i32::MAX, adding a positive delta (+32 for win, +8 for draw) overflows before .max(0) is applied. In debug mode this panics; in release mode it wraps to a negative value, which then gets incorrectly clamped to 0.
🛡️ Proposed fix
- profile.rating = (profile.rating + rating_delta).max(0);
+ let updated = (profile.rating as i64) + (rating_delta as i64);
+ profile.rating = updated.clamp(0, i32::MAX as i64) as i32;📝 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.
Suggested change
| let rating_delta = if is_draw { | |
| profile.draws += 1; | |
| 8 // Draw: +8 rating | |
| } else if is_win { | |
| profile.wins += 1; | |
| 32 // Win: +32 rating | |
| } else { | |
| profile.losses += 1; | |
| -16 // Loss: -16 rating | |
| }; | |
| // Update rating (ensure it doesn't go below 0) | |
| profile.rating = (profile.rating + rating_delta).max(0); | |
| let rating_delta = if is_draw { | |
| profile.draws += 1; | |
| 8 // Draw: +8 rating | |
| } else if is_win { | |
| profile.wins += 1; | |
| 32 // Win: +32 rating | |
| } else { | |
| profile.losses += 1; | |
| -16 // Loss: -16 rating | |
| }; | |
| // Update rating (ensure it doesn't go below 0) | |
| let updated = (profile.rating as i64) + (rating_delta as i64); | |
| profile.rating = updated.clamp(0, i32::MAX as i64) as i32; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/game_contract/src/lib.rs` around lines 1950 - 1963, The current
addition profile.rating + rating_delta can overflow an i32 before .max(0) is
applied; change the update to perform the arithmetic in a wider integer (e.g.,
cast profile.rating and rating_delta to i64), compute let new_rating_i64 =
(profile.rating as i64) + (rating_delta as i64), clamp it to the valid range
0..=i32::MAX, then assign profile.rating = new_rating_i64 as i32. Update the
block that currently computes rating_delta and sets profile.rating, and ensure
this logic is used alongside set_player_rating and anywhere profile.rating is
mutated to avoid overflow/wraparound.
Contributor
Author
|
@gabito1451 gm boss |
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
1 participant
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.
Closes #521
Summary by CodeRabbit