enhance gameplay logic by Kaylahray · Pull Request #197 · Prometheus-A/The-Velvet-Ace

Kaylahray · 2025-07-29T13:07:31Z

Closes #193

Highest Staker Logic

Requirement

The highest_staker should be updated:
- During a raise operation.
- During an all_in operation only if the all-in amount is greater than the game’s current bet.

Implementation

The raise() function in actions.cairo now sets highest_staker.
The all_in() function includes logic to update highest_staker only when the all-in amount exceeds game.current_bet.

Tests

test_raise_sets_highest_staker
Confirms that when a player successfully raises, they are correctly set as the highest_staker in the game state.
test_all_in_sets_highest_staker_when_amount_greater_than_current_bet
Verifies that a player who goes all-in with an amount greater than the current table bet becomes the new highest_staker.
test_all_in_does_not_set_highest_staker_when_amount_less_than_current_bet
Ensures that a player who goes all-in with an amount less than the current bet does not become the highest_staker.

Resetting Values After a Betting Round

Requirement

At the end of a betting round:
- Reset current_bet for all players and the game.
- Reset game.highest_staker to None.
This must happen automatically when betting concludes and must be tested.

Implementation

Created two new internal functions, is_betting_round_concluded and reset_betting_round_values. This cleanly separates the logic for checking if a round has ended (all active, non-all-in players have equal bets) and the logic for resetting the game state (player bets, game bets, highest staker) for the next phase of the round.

This function is called inside after_play() when is_betting_round_concluded() returns true.

Tests

test_betting_round_concludes_when_all_active_players_have_equal_bets
Ensures that when all active players have equal bets and a non-betting action (e.g., check) occurs, the round concludes and all values (current_bet, highest_staker) are reset.
test_betting_round_does_not_conclude_when_players_have_unequal_bets
A control test confirming that if players have unequal bets, the betting round does not conclude, and the game state remains unchanged.
test_betting_round_with_all_in_players_exempt_from_bet_matching
Verifies that all-in players (who can’t raise further) do not block the round from ending. It confirms that the round ends when all other active players match the highest bet.

Bet Spacing Validation

Requirement

Any raise amount must be a multiple of game_params.bet_spacing.
This constraint must be strictly enforced and tested.

Implementation

The raise() function includes an assertion:

assert(raise_amount % game.params.bet_spacing == 0, 'Raise amount must be in multiples of bet_spacing');

This ensures invalid raises are rejected at runtime.

Tests

test_bet_spacing_validation_in_raise
Confirms that a player can successfully raise when their amount is a valid multiple of game.params.bet_spacing.
test_bet_spacing_validation_fails_with_invalid_multiple
Ensures that any raise not adhering to the bet spacing rule correctly panics and is not accepted.

Birdmannn

Neat code, but too bloated and quite some overhead shenanigans. Anyways, it seems you don't understand the poker gameplay, and their edge case scenarios... You didn't even test the scenario where the betting round ended and the variables were reset for another round to start. Remember, a betting round is different from the game round.

Anyways, we are simulating gameplay, and not just mocking variables to test models' fields that have obviously changed. If you are to simulate by mocking, then you must explain the simulation using comments, though I'd still prefer you to use the abi calls for simulation.

Please check. Thank you very much @Kaylahray. Nice job.
Please pay attention to the requested changes to avoid another review overhead. Thanks.

Birdmannn · 2025-07-30T22:00:01Z

poker-texas-hold-em/contract/src/systems/actions.cairo

-            assert!(no_of_chips > 0, "Raise amount must be greater than zero.");
-            assert!(player.chips >= total_required, "You don't have enough chips to raise.");
+            assert!(no_of_chips > 0, "Raise must be > 0.");
+            assert!(player.chips >= total_required, "Insufficient chips to raise.");


I see good English. Welldone

Birdmannn · 2025-07-31T04:38:20Z

poker-texas-hold-em/contract/src/systems/actions.cairo

+        fn is_betting_round_concluded(
+            self: @ContractState, game_id: u64, world: @WorldStorage,
+        ) -> bool {
+            let game: Game = world.read_model(game_id);


You read the whole game model just to read the players value. Why not read the players "member" instead using world.read_member()

Birdmannn · 2025-07-31T06:05:18Z

poker-texas-hold-em/contract/src/systems/actions.cairo

        }

-        /// @Reentrancy, @Birdmannn
        fn after_play(ref self: ContractState, caller: ContractAddress) {


please replace all the comments you deleted in this function because I don't consider this function as one that has been finalized. And replace the Birdmannn and that of Reentrancy's.

Replaced Sensei

Birdmannn · 2025-07-31T06:06:47Z

poker-texas-hold-em/contract/src/systems/actions.cairo

+                    game.next_player = Option::Some(next_player_addr);
+                } else {
+                    // If no next player, something is wrong, or round should end.
+                    // `is_betting_round_concluded` should have caught this.


How would I know? Did you test it? 😂

👀🙇‍♀️

Birdmannn · 2025-07-31T06:32:37Z

poker-texas-hold-em/contract/src/systems/actions.cairo

+            let mut i: u32 = 0;
+            while i < game_players.len() {
+                let player_addr = *game_players.at(i);
+                let player: Player = world.read_model(player_addr);
+
+                if player.in_round {
+                    active_player_count += 1;
+                    if player.current_bet > highest_bet {
+                        highest_bet = player.current_bet;
+                    }
+                }
+                i += 1;
+            };
+


I don't think this would work out. I think the former implementation was just fine. after_play is called after each player has played, and checks Line588 for that particular player. You looping through all the players after each player has played just for the line588 is just gas.

Birdmannn · 2025-07-31T08:39:52Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+        assert_eq!(player_1_updated.chips, 0, "Player 1 should have 0 chips left");
+    }
+
+    #[test]


Please feel free to remove this test

Birdmannn · 2025-07-31T08:43:04Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+        let contracts = array![CoreContract::Actions];
+        let (mut world, systems) = deploy_contracts(contracts);
+        mock_poker_game(ref world);
+
+        // [Setup State] - Game with bet spacing of 20
+        let mut game: Game = world.read_model(1);
+        game.current_bet = 100;
+        let mut params = game.params;
+        params.bet_spacing = 20; // Set bet spacing to 20
+        game.params = params;
+        world.write_model(@game);
+
+        let mut player_1: Player = world.read_model(PLAYER_1());
+        player_1.current_bet = 100;
+        player_1.chips = 2000;
+        world.write_model(@player_1);
+


as normal, that function we spoke about earlier

Birdmannn · 2025-07-31T08:43:33Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+        // [Assert]
+        let player_1_updated: Player = world.read_model(PLAYER_1());
+        assert_eq!(
+            player_1_updated.current_bet, 240, "Raise should succeed with valid bet spacing",


no need for this, please remove this block

Birdmannn · 2025-07-31T08:50:14Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+    #[test]
+    #[should_panic(
+        expected: ("Raise amount must be in multiples of bet_spacing", 'ENTRYPOINT_FAILED'),
+    )]
+    fn test_bet_spacing_validation_fails_with_invalid_multiple() {
+        // [Setup] @kaylahray
+        let contracts = array![CoreContract::Actions];
+        let (mut world, systems) = deploy_contracts(contracts);
+        mock_poker_game(ref world);
+
+        // [Setup State] - Game with bet spacing of 20
+        let mut game: Game = world.read_model(1);
+        game.current_bet = 100;
+        let mut params = game.params;
+        params.bet_spacing = 20; // Set bet spacing to 20
+        game.params = params;
+        world.write_model(@game);
+
+        let mut player_1: Player = world.read_model(PLAYER_1());
+        player_1.current_bet = 100;
+        player_1.chips = 2000;
+        world.write_model(@player_1);
+
+        set_contract_address(player_1.id);
+


Merge this test with the previous by:

Removing this test

in the previous test, call Line361 with the next player

Yes, it'll panic, so add the should_panic macro at the top of the test.

And yes, when panicking, please remove the 'ENTRYPOINT_FAILED' message from the macro's expected value... for all your panic tests please.

Birdmannn · 2025-07-31T09:01:28Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+
+        // [Execute] - Player 2 checks, which should trigger betting round conclusion check
+        // Since all non-all-in players have equal bets, this should conclude the betting round
+        systems.actions.check();


if player_2 checks, then it should panic, because player 2 is the highest_staker. Though I don't understand what this test actually tests... but if this test passes, then you should understand the game flow and rework the logic. But first you must understand the game flow.

Help me explain what this test does, or you can remove this test totally.

… feat/ehnance-gameplay

Kaylahray · 2025-08-04T17:44:59Z

@Birdmannn I've implemented every change requested and returned your comment and fixed my grammar 👀🫡

Birdmannn

@Kaylahray check

Birdmannn · 2025-08-06T09:46:42Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+
+        // At this point, all players should have current_bet = 40, game.current_bet = 40
+        // The betting round should be complete and reset should have happened
+        assert!(game.highest_staker.is_none(), "Highest staker not reset");


Then the reset is complete. Check the community_dealing. As per last review on the tests, and the issue description, community dealing must be tested to. After first first round of betting, deal_community_card() must go through three times.

Add test: The fourth one must panic
Add test: After adding three community cards, the betting should resume
Add test: Try any of the betting functions when the betting round has ended. This should panic.

and this just means that the betting cycle you wrote above can be extracted into a separate function called feign_betting_round() or something, and can be reused in any of the three tests as above.

There are other edge cases tests, but I'll leave it for another issue.

Trying two betting functions 👇

Birdmannn · 2025-08-06T10:28:43Z

poker-texas-hold-em/contract/src/systems/actions.cairo

-                } else {
-                    // If no next player, something is wrong, or round should end.
-                    // `is_betting_round_concluded` should have caught this.
-                    game.showdown = true;
-                    game.next_player = Option::None;
+                game.next_player = next_player_option;
+
+                // Check if betting round is complete (more gas efficient) @kaylahray
+                if self.is_betting_round_complete(@game, @world) {
+                    // Reset betting state efficiently
+                    self.reset_betting_round(game_id, ref game, ref world);
                }
            }

            world.write_model(@game);
+


In the last review, I said you shouldn't loop per player, it's just brute force... eats gas. what if there are thirty players at the table?

By the way, didn't this logic over here work?

no?

Birdmannn · 2025-08-06T10:35:54Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

+    }
+    //   @kaylahray Testing all-in sets highest staker
+    #[test]
+    fn test_all_in_sets_highest_staker() {


As per the issue description, The aim was for all_in to work regardless of the bet_spacing. So write a new test that mocks the player's balance to not be a multiple of the bet. spacing and call the all_in. It should pass

… feat/ehnance-gameplay

Kaylahray · 2025-08-11T06:50:52Z

@Birdmannn

Birdmannn

Anyways, Thank you for your sound understanding of the poker game (and your willingness to learn) 💯⛩️

Birdmannn · 2025-08-19T01:59:25Z

poker-texas-hold-em/contract/src/tests/test_actions.cairo

Can you see that this block is repeated? Three good times...

enhance gameplay logic

7530be3

Kaylahray marked this pull request as ready for review July 29, 2025 13:57

Birdmannn self-requested a review July 29, 2025 14:07

Birdmannn reviewed Jul 31, 2025

View reviewed changes

Kaylahray added 3 commits August 4, 2025 18:24

fix: enhance gameplay logic

8d94af7

Merge branch 'v1' of https://github.com/Kaylahray/The-Velvet-Ace into…

1e9c400

… feat/ehnance-gameplay

fix: scarb fmt

21edf5c

Birdmannn reviewed Aug 6, 2025

View reviewed changes

Kaylahray added 3 commits August 11, 2025 07:16

updated contract and tests

c3534b4

Merge branch 'v1' of https://github.com/Kaylahray/The-Velvet-Ace into…

74ec301

… feat/ehnance-gameplay

scarb format

d2f921c

Birdmannn reviewed Aug 19, 2025

View reviewed changes

Birdmannn merged commit 7d34695 into Prometheus-A:v1 Aug 19, 2025
1 check passed

Conversation

Kaylahray commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Highest Staker Logic

Requirement

Implementation

Tests

Resetting Values After a Betting Round

Requirement

Implementation

Tests

Bet Spacing Validation

Requirement

Implementation

Tests

Uh oh!

Birdmannn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kaylahray commented Aug 4, 2025

Uh oh!

Birdmannn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kaylahray commented Aug 11, 2025

Uh oh!

Birdmannn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Kaylahray commented Jul 29, 2025 •

edited

Loading

Birdmannn left a comment •

edited

Loading