feat(contracts): implement puzzle reward verification, slashing logic, and move-sequence test coverage#665
Merged
gabito1451 merged 3 commits intoApr 23, 2026
Conversation
…, and move-sequence test coverage
…project-requirements Co-authored-by: Copilot <copilot@github.com>
|
@Kaylahray 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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request adds game timeout settlement logic to the smart contract, including three new public methods ( Changes
Sequence DiagramsequenceDiagram
participant Admin
participant GameContract
participant Player1
participant Player2
Admin->>GameContract: configure_timeout(duration: 1000)
GameContract->>GameContract: Store timeout duration
Player1->>GameContract: create_game(wager)
GameContract->>GameContract: Initialize game with last_move_at=0
Player2->>GameContract: submit_move(game_id)
GameContract->>GameContract: Update last_move_at timestamp
Note over Player1,Player2: Wait for timeout window
Player1->>GameContract: claim_timeout_win(game_id)
GameContract->>GameContract: Check: elapsed > timeout duration
GameContract->>GameContract: Validate claimant is waiting player
GameContract->>GameContract: Mark game Completed, set winner
GameContract->>GameContract: process_payout(winner)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Kaylahray
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/game_contract/src/lib.rs (4)
672-674:⚠️ Potential issue | 🔴 CriticalProtect max-stake configuration with admin auth.
set_max_stakemutates a global game limit without authorization, so any caller can raise caps or set the limit to block new games.🛡️ Proposed fix
- pub fn set_max_stake(env: Env, new_limit: i128) { + pub fn set_max_stake(env: Env, admin: Address, new_limit: i128) { + let current_admin: Address = env + .storage() + .instance() + .get(&CONTRACT_ADMIN) + .expect("Not initialized"); + current_admin.require_auth(); + + if admin != current_admin { + panic!("Unauthorized admin address"); + } + if new_limit < 0 { + panic!("Max stake must be non-negative"); + } env.storage().instance().set(&MAX_STAKE, &new_limit); }🤖 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 672 - 674, The setter set_max_stake currently allows any caller to change MAX_STAKE; add an authorization check that only the contract admin/owner can call it (e.g., verify against the stored OWNER/ADMIN key or role before mutating MAX_STAKE), and return/error if the caller is not authorized; update set_max_stake to fetch the caller identity from Env, compare it to the stored owner/admin identifier (the same storage key used elsewhere in the contract), and only call env.storage().instance().set(&MAX_STAKE, &new_limit) after the check passes.
760-762:⚠️ Potential issue | 🔴 CriticalPrevent reward amount aliasing before signature verification.
reward_amount as i64truncates out-of-rangei128values before hashing. A signature for one low-64-bit amount can validate a different on-chaini128amount. Reject negative/out-of-range values or encode the fulli128canonically.🛡️ Proposed fix
+ if reward_amount < 0 || reward_amount > i64::MAX as i128 { + return Err(ContractError::Unauthorized); + } + // Append reward_amount as little-endian i64 bytes let amount_le: [u8; 8] = (reward_amount as i64).to_le_bytes(); payload_bytes.append(&Bytes::from_slice(&env, &amount_le));🤖 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 760 - 762, The code currently casts reward_amount to i64 (reward_amount as i64) then appends its 8-byte LE representation (amount_le), which allows aliasing of distinct i128 on-chain values to the same 64-bit signature input; either enforce and reject out-of-range/negative reward_amount values before signature hashing (check reward_amount fits within i64 and error/abort if not) or encode the full i128 canonical bytes instead (convert reward_amount to i128 and append its 16-byte little-endian representation) so payload_bytes includes the full integer; update the logic around reward_amount, amount_le, and payload_bytes accordingly and ensure the signature verification uses the same canonical encoding.
593-603:⚠️ Potential issue | 🔴 CriticalValidate escrow before debiting payout stakes.
process_payoutsubtracts wagers without checking escrow balances. Repeatedpayoutcalls or stale dispute resolution can drive escrow negative and transfer from unrelated contract funds if liquidity exists.🛡️ Proposed fix
// Deduct both stakes first (clean state, prevents double-spend) let player1_escrow = escrow.get(game.player1.clone()).unwrap_or(0); + if player1_escrow < game.wager_amount { + return Err(ContractError::InsufficientFunds); + } escrow.set(game.player1.clone(), player1_escrow - game.wager_amount); let player2 = game.player2.as_ref().ok_or(ContractError::GameFull)?; let player2_escrow = escrow.get(player2.clone()).unwrap_or(0); + if player2_escrow < game.wager_amount { + return Err(ContractError::InsufficientFunds); + } escrow.set(player2.clone(), player2_escrow - game.wager_amount);🤖 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 593 - 603, Before mutating escrow, validate each player's balance in process_payout: read player1_escrow and player2_escrow and if either is less than game.wager_amount return an appropriate ContractError (e.g., InsufficientEscrow) instead of subtracting; only call escrow.set(...) to decrement after both checks pass. Also use checked arithmetic (or ensure non-negative) when computing new balances and when crediting the winner (winner_escrow + payout) to avoid overflow. Reference symbols: process_payout, game.player1, game.player2, escrow.get, escrow.set, game.wager_amount, winner, payout.
1230-1233:⚠️ Potential issue | 🟠 MajorMake snapshot signing keys deterministic.
SigningKey::generate(&mut OsRng)at line 1231 changes the serializedADMIN_KEYvalue in test snapshots every run, making these snapshot fixtures flaky. The threetest_claim_puzzle_reward_*snapshots show different admin keys becausesetup()generates a new random key each time.🧪 Proposed fix
- let signing_key = SigningKey::generate(&mut OsRng); + let signing_key = SigningKey::from_bytes(&[7u8; 32]);🤖 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 1230 - 1233, The test snapshots are flaky because SigningKey::generate(&mut OsRng) in setup() produces a new random ADMIN_KEY each run; replace the nondeterministic generator with a deterministic one (e.g., construct the SigningKey from a fixed seed or use a seeded RNG such as StdRng::seed_from_u64(0) or ChaCha20Rng::from_seed) so that signing_key (and therefore verifying_key_bytes and admin_key) are stable across runs; update the code around Address::generate(env), SigningKey::generate(&mut OsRng), verifying_key_bytes and admin_key to use the deterministic key source.
🧹 Nitpick comments (1)
contracts/game_contract/test_snapshots/tests/test_file_dispute_success.1.json (1)
730-735: Includegame_idin thedispute/filedevent payload.The dispute state stores
game_id, but the event only emits[dispute_id, filer]. Includinggame_idkeeps the event self-describing and avoids an extraget_disputeread for indexers.Also applies to: 2461-2485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/game_contract/test_snapshots/tests/test_file_dispute_success.1.json` around lines 730 - 735, The dispute/filed event currently emits only [dispute_id, filer] but the dispute state includes game_id; update the event payload to include that game_id so indexers don't need an extra get_dispute read. In the code path that creates a dispute and emits the "dispute/filed" event (the function handling dispute filing where dispute_id and filer are assembled), fetch the dispute.game_id and include it in the emitted payload (e.g., emit ["dispute/filed", [dispute_id, filer, game_id]]), and ensure the event schema/serialization for "dispute/filed" is updated accordingly so listeners can consume [dispute_id, filer, game_id].
🤖 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 885-887: The current check only rejects GameState::Created but
allows disputes on Completed, Drawn, or Forfeited games; change the condition in
the dispute entry point so disputes are allowed only when game.state ==
GameState::Active (i.e., return Err(ContractError::NotDisputable) if game.state
!= GameState::Active). Locate the check referencing GameState and
ContractError::NotDisputable in lib.rs (the disputed-game validation logic) and
replace the existing Created-only guard with an Active-only guard.
- Around line 1064-1087: Before applying dispute settlement, re-fetch and verify
the game's current state is still Disputed to avoid double-settling: after
loading games and obtaining game (the Map<u64, Game> `games`, the `game` from
`games.get(dispute.game_id)` and the `dispute`/`winner` values), check that
game.state == GameState::Disputed and return an appropriate ContractError (e.g.,
ContractError::InvalidGameState or a new NotDisputable error) if it is not; only
then proceed with the existing match branch that sets
GameState::Completed/Drawn, assigns game.winner, calls Self::process_payout or
Self::process_draw_payout, and persist the modified `game` back into `games` and
storage so settlement cannot be applied twice.
In
`@contracts/game_contract/test_snapshots/tests/test_resolve_dispute_winner_takes_all.1.json`:
- Around line 835-865: The ESCROW map is left with the winner's payout amount
after the contract transfers tokens, causing double-counting; update the payout
paths so ESCROW entries are reconciled with actual token transfers: in
process_payout, process_draw_payout, and payout_tournament locate the logic that
credits ESCROW[winner] and performs the token transfer (symbols: process_payout,
process_draw_payout, payout_tournament, ESCROW) and either 1) deduct the
winner's payout from the ESCROW map before performing the physical token
transfer or 2) perform the transfer first and then set ESCROW[winner] (and any
loser ESCROW entries) to zero immediately after, and audit all branches to
ensure no code path leaves a stale ESCROW balance (clear or update ESCROW for
draws and tournament payouts consistently).
---
Outside diff comments:
In `@contracts/game_contract/src/lib.rs`:
- Around line 672-674: The setter set_max_stake currently allows any caller to
change MAX_STAKE; add an authorization check that only the contract admin/owner
can call it (e.g., verify against the stored OWNER/ADMIN key or role before
mutating MAX_STAKE), and return/error if the caller is not authorized; update
set_max_stake to fetch the caller identity from Env, compare it to the stored
owner/admin identifier (the same storage key used elsewhere in the contract),
and only call env.storage().instance().set(&MAX_STAKE, &new_limit) after the
check passes.
- Around line 760-762: The code currently casts reward_amount to i64
(reward_amount as i64) then appends its 8-byte LE representation (amount_le),
which allows aliasing of distinct i128 on-chain values to the same 64-bit
signature input; either enforce and reject out-of-range/negative reward_amount
values before signature hashing (check reward_amount fits within i64 and
error/abort if not) or encode the full i128 canonical bytes instead (convert
reward_amount to i128 and append its 16-byte little-endian representation) so
payload_bytes includes the full integer; update the logic around reward_amount,
amount_le, and payload_bytes accordingly and ensure the signature verification
uses the same canonical encoding.
- Around line 593-603: Before mutating escrow, validate each player's balance in
process_payout: read player1_escrow and player2_escrow and if either is less
than game.wager_amount return an appropriate ContractError (e.g.,
InsufficientEscrow) instead of subtracting; only call escrow.set(...) to
decrement after both checks pass. Also use checked arithmetic (or ensure
non-negative) when computing new balances and when crediting the winner
(winner_escrow + payout) to avoid overflow. Reference symbols: process_payout,
game.player1, game.player2, escrow.get, escrow.set, game.wager_amount, winner,
payout.
- Around line 1230-1233: The test snapshots are flaky because
SigningKey::generate(&mut OsRng) in setup() produces a new random ADMIN_KEY each
run; replace the nondeterministic generator with a deterministic one (e.g.,
construct the SigningKey from a fixed seed or use a seeded RNG such as
StdRng::seed_from_u64(0) or ChaCha20Rng::from_seed) so that signing_key (and
therefore verifying_key_bytes and admin_key) are stable across runs; update the
code around Address::generate(env), SigningKey::generate(&mut OsRng),
verifying_key_bytes and admin_key to use the deterministic key source.
---
Nitpick comments:
In
`@contracts/game_contract/test_snapshots/tests/test_file_dispute_success.1.json`:
- Around line 730-735: The dispute/filed event currently emits only [dispute_id,
filer] but the dispute state includes game_id; update the event payload to
include that game_id so indexers don't need an extra get_dispute read. In the
code path that creates a dispute and emits the "dispute/filed" event (the
function handling dispute filing where dispute_id and filer are assembled),
fetch the dispute.game_id and include it in the emitted payload (e.g., emit
["dispute/filed", [dispute_id, filer, game_id]]), and ensure the event
schema/serialization for "dispute/filed" is updated accordingly so listeners can
consume [dispute_id, filer, game_id].
🪄 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: 450dbd6a-aa12-449f-9c01-73e91d66314a
📒 Files selected for processing (18)
contracts/emergency_circuit_breaker/src/lib.rscontracts/game_contract/src/lib.rscontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_invalid_sig.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_replay_rejected.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_puzzle_reward_valid_sig.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_not_reached.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_rejects_current_turn_player.1.jsoncontracts/game_contract/test_snapshots/tests/test_claim_timeout_win_success.1.jsoncontracts/game_contract/test_snapshots/tests/test_configure_timeout.1.jsoncontracts/game_contract/test_snapshots/tests/test_fee_redirection_2_percent.1.jsoncontracts/game_contract/test_snapshots/tests/test_fee_redirection_2_percent_large.1.jsoncontracts/game_contract/test_snapshots/tests/test_file_dispute_success.1.jsoncontracts/game_contract/test_snapshots/tests/test_get_timeout_remaining.1.jsoncontracts/game_contract/test_snapshots/tests/test_reject_dispute_refund_fee.1.jsoncontracts/game_contract/test_snapshots/tests/test_resolve_dispute_winner_takes_all.1.jsoncontracts/game_contract/test_snapshots/tests/test_submit_move_rejects_out_of_turn_and_empty_moves.1.jsoncontracts/game_contract/test_snapshots/tests/test_submit_move_sequence_updates_turn_and_history.1.jsoncontracts/game_contract/test_snapshots/tests/test_usdc_staking_workflow.1.json
Comment on lines
+885
to
+887
| if game.state == GameState::Created { | ||
| return Err(ContractError::NotDisputable); | ||
| } |
Contributor
There was a problem hiding this comment.
Only allow disputes for active games.
This rejects Created, but still accepts Completed, Drawn, and Forfeited games, charging dispute fees for already-settled games.
🛡️ Proposed fix
- if game.state == GameState::Created {
+ if game.state != GameState::InProgress {
return Err(ContractError::NotDisputable);
}🤖 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 885 - 887, The current check
only rejects GameState::Created but allows disputes on Completed, Drawn, or
Forfeited games; change the condition in the dispute entry point so disputes are
allowed only when game.state == GameState::Active (i.e., return
Err(ContractError::NotDisputable) if game.state != GameState::Active). Locate
the check referencing GameState and ContractError::NotDisputable in lib.rs (the
disputed-game validation logic) and replace the existing Created-only guard with
an Active-only guard.
Comment on lines
+835
to
+865
| { | ||
| "key": { | ||
| "symbol": "ESCROW" | ||
| }, | ||
| "val": { | ||
| "map": [ | ||
| { | ||
| "key": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 200 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAK3IM" | ||
| }, | ||
| "val": { | ||
| "i128": { | ||
| "hi": 0, | ||
| "lo": 0 | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect payout and escrow-update paths for dispute and timeout completion.
# Expected: after a winner payout, both players' active escrow entries are cleared/decremented,
# or the contract has a documented invariant explaining why ESCROW may exceed token balance.
rg -n -C4 'ESCROW|escrow|resolve_dispute|claim_timeout_win|transfer|payout|winner|Completed' contracts/game_contract/src/lib.rsRepository: NOVUS-X/XLMate
Length of output: 21719
🏁 Script executed:
# Search for all references to ESCROW after payout operations
rg -n 'ESCROW|escrow' contracts/game_contract/src/lib.rs | head -60
# Also check if there are any invariant comments or documentation
rg -n 'invariant|balance|accounting' contracts/game_contract/src/lib.rsRepository: NOVUS-X/XLMate
Length of output: 4967
🏁 Script executed:
# Look for any "claim" or "withdraw" functions that might consume ESCROW
rg -n 'fn (claim|withdraw|redeem)' contracts/game_contract/src/lib.rs
# Check if ESCROW is read anywhere after process_payout
rg -n -B5 -A5 'get.*ESCROW' contracts/game_contract/src/lib.rs | head -100Repository: NOVUS-X/XLMate
Length of output: 2544
Clear escrow after winner payout.
process_payout (lines 595–603) deducts both players' wagers from ESCROW and credits the winner's payout, then line 619 transfers tokens to the winner. This leaves ESCROW[winner] with the payout amount after the contract has already transferred those tokens out (line 2479 in the snapshot shows contract balance=0). The snapshot at line 845 records ESCROW[player1] = 200 after the transfer, creating redundant accounting: the tokens exist both in the winner's balance and in ESCROW. Either ESCROW should be cleared after transfer, or the physical token transfer should not happen.
Verify and fix all payout paths: process_payout (line 569), process_draw_payout (line 534), and payout_tournament (line 430) must either deduct ESCROW after transfer or document an invariant explaining the separation.
Also applies to: 1182–1252, 2450–2540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@contracts/game_contract/test_snapshots/tests/test_resolve_dispute_winner_takes_all.1.json`
around lines 835 - 865, The ESCROW map is left with the winner's payout amount
after the contract transfers tokens, causing double-counting; update the payout
paths so ESCROW entries are reconciled with actual token transfers: in
process_payout, process_draw_payout, and payout_tournament locate the logic that
credits ESCROW[winner] and performs the token transfer (symbols: process_payout,
process_draw_payout, payout_tournament, ESCROW) and either 1) deduct the
winner's payout from the ESCROW map before performing the physical token
transfer or 2) perform the transfer first and then set ESCROW[winner] (and any
loser ESCROW entries) to zero immediately after, and audit all branches to
ensure no code path leaves a stale ESCROW balance (clear or update ESCROW for
draws and tournament payouts consistently).
Co-authored-by: Copilot <copilot@github.com>
This was referenced Apr 24, 2026
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.
Summary
This PR completes the contract work for the following issues:
What Changed
Puzzle reward verification
Slashing logic for disconnected players
Move-sequence test coverage
Why
These issues all target critical on-chain settlement and staking behavior in
contracts/game_contract/src. This PR closes the remaining gap in disconnect handling by making timeout claims turn-aware, while also expanding unit coverage around move flow and preserving the existing puzzle reward verification path.Validation
Ran the contract test suite with cargo.
Result:
16 passed0 failedNotes
Summary by CodeRabbit
New Features
Bug Fixes
Tests