Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion contracts/game_contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub struct Game {
pub moves: Vec<ChessMove>,
pub created_at: u64,
pub winner: Option<Address>,
pub proof_of_game: BytesN<32>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all Game struct literal constructions to ensure they include the new field.
rg -nP -U '(?s)\bGame\s*\{[^}]*\}' --type=rust -C2

Repository: NOVUS-X/XLMate

Length of output: 10043


Update test.rs Game struct literal to include the required proof_of_game field.

The Game struct literal in test.rs (lines 23–33) is missing the proof_of_game: BytesN<32> field that was added to the struct definition. This will cause a compilation error. Add proof_of_game: BytesN::from_array(&env, &[0; 32]), to the literal to match the struct definition and the initialization used in create_game (lib.rs:155).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/game_contract/src/lib.rs` at line 34, The Game struct literal in
the tests is missing the new proof_of_game field; update the Game literal to
include proof_of_game initialized the same way as in create_game by setting
proof_of_game to BytesN::from_array(&env, &[0; 32]) so the test struct matches
the Game definition (and the create_game initialization).

pub last_move_at: u64, // Ledger sequence of last move
}

Expand Down Expand Up @@ -209,6 +210,7 @@ impl GameContract {
moves: Vec::new(&env),
created_at: env.ledger().sequence() as u64,
winner: None,
proof_of_game: BytesN::from_array(&env, &[0; 32]),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing unit tests for the new proof_of_game feature.

Issue #507 explicitly calls for unit tests covering standard and edge cases. The PR adds the field and update logic but no tests in this file verify: (a) the initial zero proof on creation, (b) that proof_of_game changes deterministically after each submit_move, (c) that different move sequences produce different proofs, and (d) that the same inputs reproduce the same final proof off-chain. Please add coverage.

Want me to draft the test cases?

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/game_contract/src/lib.rs` at line 155, Add unit tests in
contracts/game_contract/src/lib.rs that assert the new proof_of_game behavior:
1) test initial state uses BytesN::from_array(&env, &[0; 32]) by instantiating
the contract (constructor/new/instantiate) and checking proof_of_game is zero;
2) test that calling submit_move(...) mutates proof_of_game deterministically by
calling submit_move once and asserting proof_of_game changed from zero to a
specific value (store that value from the call); 3) test that two different move
sequences produce different final proof_of_game values by submitting two
distinct sequences via submit_move and comparing their proofs; and 4) test
reproducibility by running the same sequence twice (or reproducing the on-chain
update logic off-chain using the same inputs) and asserting the final
proof_of_game values match; use the contract's getter or direct state access for
proof_of_game and reuse BytesN::from_array as the expected zero-proof sentinel.

last_move_at: env.ledger().sequence() as u64,
};

Expand Down Expand Up @@ -324,9 +326,21 @@ impl GameContract {

let chess_move = ChessMove {
player: player.clone(),
move_data,
move_data: move_data.clone(),
timestamp: env.ledger().sequence() as u64,
};

let mut proof_payload = Bytes::new(&env);
proof_payload.append(&game.proof_of_game.clone().into());
for m in move_data.iter() {
proof_payload.append(&Bytes::from_slice(&env, &m.to_le_bytes()));
}
proof_payload.append(&Bytes::from_slice(
&env,
&chess_move.timestamp.to_le_bytes(),
));
game.proof_of_game = env.crypto().sha256(&proof_payload).into();

game.moves.push_back(chess_move);
Comment on lines +332 to +344

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Proof payload is not canonical — omits player/game_id and lacks length framing for move_data.

The current chaining hashes prev_proof || concat(move_data u32 LE) || timestamp_le. Two issues weaken the "proof of game":

  1. No length prefix for move_data: since move_data: Vec<u32> has variable length and multiple moves are chained, concatenating raw bytes is ambiguous. A sequence of moves [1,2] then [3] produces the same concatenated bytes as [1] then [2,3] at the same timestamps — different histories can yield the same proof.
  2. Missing binding fields: neither player (move author) nor game.id is included. The chain can't attest who submitted each move, and proofs are not domain-separated per game.

Consider encoding the move with explicit framing, e.g.:

♻️ Proposed fix
         let mut proof_payload = Bytes::new(&env);
         proof_payload.append(&game.proof_of_game.clone().into());
+        // Domain separation / binding
+        proof_payload.append(&Bytes::from_slice(&env, &game.id.to_le_bytes()));
+        proof_payload.append(&Bytes::from_slice(&env, &(player_num as u32).to_le_bytes()));
+        // Length-prefix the variable-length move_data to prevent concatenation ambiguity
+        proof_payload.append(&Bytes::from_slice(&env, &(move_data.len() as u32).to_le_bytes()));
         for m in move_data.iter() {
             proof_payload.append(&Bytes::from_slice(&env, &m.to_le_bytes()));
         }
         proof_payload.append(&Bytes::from_slice(
             &env,
             &chess_move.timestamp.to_le_bytes(),
         ));
         game.proof_of_game = env.crypto().sha256(&proof_payload).into();
🤖 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 270 - 282, The proof payload
is non-canonical: update the construction around
proof_payload/game.proof_of_game so each move is length-prefixed and bound to
the game and player; specifically, when appending move_data (Vec<u32>) to
proof_payload, first append its length as a fixed-size integer (e.g., u32 LE),
then append each u32 element in LE, and also include chess_move.player (or
equivalent author identifier) and game.id into the payload before hashing;
ensure you still include the prior game.proof_of_game and timestamp
(chess_move.timestamp) so the chain remains incremental and domain-separated per
game.

game.current_turn = if game.current_turn == 1 { 2 } else { 1 };
game.last_move_at = env.ledger().sequence() as u64;
Expand Down
Loading