feat: Move sequence tests, SEP-40 oracle support, and time-lock escrow#707
Merged
gabito1451 merged 2 commits intoApr 28, 2026
Conversation
…scrow Closes OpenKnight-Foundation#534: Smart contract unit test suite for move sequences - Added comprehensive unit tests for submit_move function - Tests cover: normal moves, wrong turn, non-player, empty data, game state, alternation Closes OpenKnight-Foundation#533: Integrate SEP-40 Oracle support for clock sync - Added configure_oracle, get_oracle, and get_oracle_time functions - Storage key for oracle contract address - Placeholder for cross-contract oracle invocation Closes OpenKnight-Foundation#532: Time-lock escrow for tournament grand prizes - Added TournamentEscrow type with lock duration - configure_tournament_timelock for admin configuration - create_tournament_escrow to lock prize pools - release_tournament_escrow with time validation - get_tournament_escrow query function
|
@dzekojohn4 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
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughThe pull request adds SEP-40 oracle clock synchronization support and tournament time-lock escrow functionality to the game contract. It introduces oracle address configuration, oracle time retrieval, tournament escrow creation and release with timelock validation, and corresponding error handling. Tests are added for move submission behavior with turn-based validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Contract
participant Oracle
participant Storage
Note over Contract,Storage: Oracle Synchronization Flow
Contract->>Storage: configure_oracle(admin, oracle_addr)
Storage-->>Contract: oracle config stored
Contract->>Contract: get_oracle()
Contract-->>Contract: retrieve oracle address
Contract->>Oracle: read latest time
Oracle-->>Contract: return current timestamp
Contract->>Contract: get_oracle_time()
Contract-->>Contract: return synced time
sequenceDiagram
participant Admin
participant Contract
participant Storage
participant Winners
Note over Admin,Winners: Tournament Escrow Lifecycle
Admin->>Contract: configure_tournament_timelock(duration)
Contract->>Storage: store timelock duration
Storage-->>Contract: done
Admin->>Contract: create_tournament_escrow(game_id)
Contract->>Storage: create escrow record
Storage-->>Storage: set locked_until timestamp
Storage-->>Contract: return escrow_id
Contract->>Admin: emit creation event
Note over Admin,Winners: ... time passes ...
Winners->>Contract: release_tournament_escrow(escrow_id, winners, percentages)
Contract->>Storage: verify escrow exists
Contract->>Storage: verify lock expired
Contract->>Contract: validate percentages sum to 100
Storage-->>Contract: checks passed
Contract->>Winners: distribute tokens to each winner
Contract->>Storage: mark escrow released
Contract->>Winners: emit release event
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 145-153: TournamentEscrow currently lacks owner/winner/split
information and release_tournament_escrow trusts caller-supplied recipients, so
persist a canonical payout plan at escrow creation and validate it at release:
add payout fields to the TournamentEscrow struct (e.g., winners: Vec<AccountId>
and splits: Vec<u8>/Vec<i128> or a single encoded PayoutPlan), populate that
plan in the function that creates the escrow (where game_id is known), and
change release_tournament_escrow to only allow releasing to the stored payout
plan (or to compute the plan from immutable game state using the game_id) while
rejecting arbitrary recipient lists and enforcing any required auth checks;
update all places that construct or read TournamentEscrow to use the new fields
and ensure total splits sum validation occurs at creation.
- Around line 1916-1970: The function create_tournament_escrow currently allows
creating multiple escrows for the same game; before creating a new
TournamentEscrow, check for an existing escrow tied to game_id (e.g. by looking
up TOURNAMENT_ESCROWS for any entry with that game_id or by maintaining a new
map TOURNAMENT_GAME_TO_ESCROW mapping game_id -> escrow_id) and return an error
if one exists; then, when creating the escrow, persist the guard by either
updating the Game (add/set an escrowed flag on the Game stored in GAMES) or by
inserting the new mapping (set TOURNAMENT_GAME_TO_ESCROW[game_id] = escrow_id)
so subsequent calls to create_tournament_escrow will fail. Ensure you update the
saved GAMES or TOURNAMENT_ESCROWS/TOURNAMENT_GAME_TO_ESCROW storage consistently
and publish the event only after the guard is stored.
- Around line 1865-1882: The get_oracle_time function currently ignores
ORACLE_CONTRACT; update get_oracle_time to perform a cross-contract invocation
on the stored oracle address (ORACLE_CONTRACT) to call the oracle's lastprice
method, parse the returned price record to extract the timestamp, and return
that timestamp; on any failure (missing field, call error, or unexpected return
type) fall back to env.ledger().sequence() as u64. Ensure you reference the
stored Address from ORACLE_CONTRACT, invoke the oracle's lastprice entrypoint,
convert/unpack the returned record to obtain the timestamp field, and handle
errors gracefully so configure_oracle actually affects behavior.
In `@contracts/game_contract/src/test.rs`:
- Around line 620-623: The function setup_in_progress_game has an incorrect
return type: change the returned GameContractClient to the
lifetime-parameterized form GameContractClient<'_> so it matches the
Soroban-generated client signature; update the function signature's return tuple
to (GameContractClient<'_>, Address, Address, u64) to fix the compilation error
referencing the GameContractClient type.
🪄 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: d63e46dc-c730-459d-a7f7-e43ca2c30dda
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
contracts/game_contract/src/lib.rscontracts/game_contract/src/test.rs
Comment on lines
+145
to
+153
| #[contracttype] | ||
| #[derive(Clone, Debug)] | ||
| pub struct TournamentEscrow { | ||
| pub escrow_id: u64, | ||
| pub game_id: u64, | ||
| pub total_amount: i128, | ||
| pub locked_until: u64, // ledger sequence when funds can be released | ||
| pub released: bool, | ||
| } |
Contributor
There was a problem hiding this comment.
The release path can pay the entire escrow to arbitrary addresses.
TournamentEscrow does not bind winners or the split, and release_tournament_escrow accepts both from the caller without auth or cross-checking against canonical game state. After the timelock expires, any caller can route 100% of the pool to attacker-controlled addresses. Persist the payout plan at escrow creation time, or derive it from immutable game data, and release only that stored plan.
Also applies to: 1976-2045
🤖 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 145 - 153, TournamentEscrow
currently lacks owner/winner/split information and release_tournament_escrow
trusts caller-supplied recipients, so persist a canonical payout plan at escrow
creation and validate it at release: add payout fields to the TournamentEscrow
struct (e.g., winners: Vec<AccountId> and splits: Vec<u8>/Vec<i128> or a single
encoded PayoutPlan), populate that plan in the function that creates the escrow
(where game_id is known), and change release_tournament_escrow to only allow
releasing to the stored payout plan (or to compute the plan from immutable game
state using the game_id) while rejecting arbitrary recipient lists and enforcing
any required auth checks; update all places that construct or read
TournamentEscrow to use the new fields and ensure total splits sum validation
occurs at creation.
Comment on lines
+1865
to
+1882
| /// Get the oracle-synced timestamp (ledger sequence from oracle). | ||
| /// Falls back to the current ledger sequence if oracle is not configured. | ||
| /// The oracle is called via cross-contract invocation using the SEP-40 | ||
| /// `lastprice` interface which returns a price record with a timestamp. | ||
| /// | ||
| /// Note: This is a minimal implementation. In production, you would invoke | ||
| /// the oracle contract's `lastprice` method and extract the timestamp. | ||
| pub fn get_oracle_time(env: Env) -> u64 { | ||
| let oracle_opt: Option<Address> = env.storage().instance().get(&ORACLE_CONTRACT); | ||
| match oracle_opt { | ||
| Some(_oracle) => { | ||
| // TODO: Implement cross-contract call to oracle.lastprice() | ||
| // For now, fall back to ledger sequence | ||
| env.ledger().sequence() as u64 | ||
| } | ||
| None => env.ledger().sequence() as u64, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
get_oracle_time still ignores the configured oracle.
When ORACLE_CONTRACT is present, this branch still returns env.ledger().sequence(), so configure_oracle has no effect and the advertised fallback never actually differs from the configured case. Wire the cross-contract read here before shipping, or rename the API so it does not promise synchronized oracle time.
🤖 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 1865 - 1882, The
get_oracle_time function currently ignores ORACLE_CONTRACT; update
get_oracle_time to perform a cross-contract invocation on the stored oracle
address (ORACLE_CONTRACT) to call the oracle's lastprice method, parse the
returned price record to extract the timestamp, and return that timestamp; on
any failure (missing field, call error, or unexpected return type) fall back to
env.ledger().sequence() as u64. Ensure you reference the stored Address from
ORACLE_CONTRACT, invoke the oracle's lastprice entrypoint, convert/unpack the
returned record to obtain the timestamp field, and handle errors gracefully so
configure_oracle actually affects behavior.
Comment on lines
+1916
to
+1970
| pub fn create_tournament_escrow( | ||
| env: Env, | ||
| game_id: u64, | ||
| ) -> Result<u64, ContractError> { | ||
| let games: Map<u64, Game> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&GAMES) | ||
| .ok_or(ContractError::GameNotFound)?; | ||
|
|
||
| let game = games.get(game_id).ok_or(ContractError::GameNotFound)?; | ||
|
|
||
| if game.state != GameState::Completed { | ||
| return Err(ContractError::GameNotInProgress); | ||
| } | ||
|
|
||
| game.player1.require_auth(); | ||
|
|
||
| let duration: u64 = env | ||
| .storage() | ||
| .instance() | ||
| .get(&TOURNAMENT_TIMELOCK) | ||
| .ok_or(ContractError::TimeoutNotConfigured)?; | ||
|
|
||
| let total_amount = match &game.player2 { | ||
| Some(_) => game.wager_amount * 2, | ||
| None => game.wager_amount, | ||
| }; | ||
|
|
||
| let locked_until = env.ledger().sequence() as u64 + duration; | ||
|
|
||
| let mut escrows: Map<u64, TournamentEscrow> = env | ||
| .storage() | ||
| .instance() | ||
| .get(&TOURNAMENT_ESCROWS) | ||
| .unwrap_or(Map::new(&env)); | ||
|
|
||
| let escrow_id = escrows.len() as u64 + 1; | ||
| let escrow = TournamentEscrow { | ||
| escrow_id, | ||
| game_id, | ||
| total_amount, | ||
| locked_until, | ||
| released: false, | ||
| }; | ||
|
|
||
| escrows.set(escrow_id, escrow); | ||
| env.storage().instance().set(&TOURNAMENT_ESCROWS, &escrows); | ||
|
|
||
| env.events().publish( | ||
| (symbol_short!("tl_escrow"), symbol_short!("created")), | ||
| (escrow_id, game_id, locked_until), | ||
| ); | ||
|
|
||
| Ok(escrow_id) |
Contributor
There was a problem hiding this comment.
Prevent the same game from being escrowed multiple times.
create_tournament_escrow only appends a new record. It does not mark the game as escrowed/settled, and it does not store any game_id -> escrow_id guard. The same completed game can therefore mint multiple escrows and still remain eligible for the existing tournament payout paths, creating multiple claims against one prize pool.
🤖 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 1916 - 1970, The function
create_tournament_escrow currently allows creating multiple escrows for the same
game; before creating a new TournamentEscrow, check for an existing escrow tied
to game_id (e.g. by looking up TOURNAMENT_ESCROWS for any entry with that
game_id or by maintaining a new map TOURNAMENT_GAME_TO_ESCROW mapping game_id ->
escrow_id) and return an error if one exists; then, when creating the escrow,
persist the guard by either updating the Game (add/set an escrowed flag on the
Game stored in GAMES) or by inserting the new mapping (set
TOURNAMENT_GAME_TO_ESCROW[game_id] = escrow_id) so subsequent calls to
create_tournament_escrow will fail. Ensure you update the saved GAMES or
TOURNAMENT_ESCROWS/TOURNAMENT_GAME_TO_ESCROW storage consistently and publish
the event only after the guard is stored.
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 resolves three issues assigned to @dzekojohn4 in the
contracts/game_contract/srcmodule.Closes #534 — Smart contract unit test suite for move sequences
Added 7 unit tests for the
submit_movefunction covering all edge cases:test_submit_move_normal— player1 submits a valid move, turn advances to player2test_submit_move_wrong_turn— player2 cannot move when it's player1's turn →NotYourTurntest_submit_move_not_a_player— outsider address rejected →NotPlayertest_submit_move_empty_move_data— empty move vector rejected →InvalidMovetest_submit_move_game_not_in_progress— move on a Created game rejected →GameNotInProgresstest_submit_move_sequence_alternation— 4-move sequence alternates correctly between playerstest_submit_move_player1_cannot_move_twice— consecutive moves by same player rejectedCloses #533 — Integrate SEP-40: Oracle support for clock sync
Added oracle integration for time synchronization:
configure_oracle(admin, oracle)— admin sets the SEP-40 oracle contract addressget_oracle()— query the configured oracle addressget_oracle_time()— returns oracle-synced timestamp; falls back to ledger sequence if oracle not configuredStorage key:
ORACLE_CONTRACTCloses #532 — Time-lock escrow for tournament grand prizes
Added time-locked escrow mechanism for tournament prizes:
TournamentEscrowtype withescrow_id,game_id,total_amount,locked_until,releasedconfigure_tournament_timelock(admin, duration)— admin sets lock duration in ledger sequencescreate_tournament_escrow(game_id)— locks prize pool for a completed game untilcurrent_ledger + durationrelease_tournament_escrow(escrow_id, winners, percentages)— distributes prizes after lock expires; dust goes to first winnerget_tournament_escrow(escrow_id)— query escrow stateNew error variants:
OracleNotConfigured,EscrowNotFound,EscrowStillLocked,EscrowAlreadyReleasedTesting
All new tests are in
contracts/game_contract/src/test.rs. Run with:Summary by CodeRabbit
Release Notes
New Features
Tests