Adding the model and system tournament blueprint #96
Conversation
WalkthroughThe pull request introduces significant enhancements to the tournament management functionality within the application. It adds a new enumeration, Additionally, a new interface Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Hi @Gerson2102, if you execute sozo build and you do not receive errors it's ok, but remember to import the new files/code in the lib.cairo file (I think you need to do it). Also if you want to validate in a quickly way that your functions are working just write some unit tests for yourself to verify that (do not upload that in the PR), because the idea is to create a complete set of unit tests in the other issue. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Gerson2102
force-pushed
the
tournament-task
branch
from
cfc6c16 to
32f38c3
Compare
|
I'm getting this error while trying to test my code: sozo test -f test_create_tournament
testing test(bytebeasts_unittest) bytebeasts v0.1.0 (/root/ByteBeastsBackend/Scarb.toml)
running 1 test
test bytebeasts::tests::test_tournament::tests::test_create_tournament ... fail (gas usage est.: 10994620)
failures:
bytebeasts::tests::test_tournament::tests::test_create_tournament - Panicked with ("Model `Tournament`: deserialization failed. Ensure the length of the keys tuple is matching the number of #[key] fields in the model struct.", 0x454e545259504f494e545f4641494c4544 ('ENTRYPOINT_FAILED')).
error: test result: FAILED. 0 passed; 1 failed; 0 ignoredI dont know why is that happening, the Tournament model just has one #[key] argument. It should works. |
|
@Gerson2102 can you show me your tests? |
|
@danielcdz this is the test: #[test]
fn test_create_tournament() {
let (_, tournament_system) = setup_world();
let mut players = ArrayTrait::new();
let player_ash = Player {
player_id: 1,
player_name: 'Ash',
beast_1: 1, // Beast 1 assigned
beast_2: 0, // No beast assigned
beast_3: 0, // No beast assigned
beast_4: 0, // No beast assigned
potions: 1
};
players.append(player_ash);
tournament_system
.create_tournament(1, 'tournament', TournamentStatus::Pending, 1, 2, players, 1);
let tournament = tournament_system.get_tournament(1);
assert!(tournament.name == 'tournament', "The tournament name is wrong!");
} |
|
@Gerson2102 can you add the test file to this PR to have all the context and try to debug the test? |
This is my dojo version currently: |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/tests/test_tournament.cairo (1)
16-19: Simplify import statementsThe double curly braces in the imports are unnecessary and can be simplified.
-use bytebeasts::{ - models::tournament::{{Tournament, tournament}}, models::tournament::TournamentStatus, - models::player::{{Player, player}}, -}; +use bytebeasts::{ + models::tournament::{Tournament, tournament, TournamentStatus}, + models::player::{Player, player}, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/tests/test_tournament.cairo(1 hunks)
🔇 Additional comments (2)
src/tests/test_tournament.cairo (2)
30-32: Previous comment about error handling is still valid
41-60: Previous comment about test coverage and readability is still valid
|
@Gerson2102 I saw your tests and they seem to be good, I faced a similar issue a couple of weeks ago and I think is related to the Dojo version, for now, is ok to delete those system tests as we will be migrating all these repo to Dojo 1.0.0 soon, but please add some tests to the model, something like this: ByteBeastsBackend/src/models/battle.cairo Line 38 in d088b3b |
Got it. Changes made now. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/models/tournament.cairo (1)
25-69: Enhance test coverage with edge cases and validation tests.The current test only covers basic initialization. Consider adding:
Edge cases:
- Maximum number of participants
- Zero entry fee tournaments
- Various prize pool configurations
Validation tests:
- Invalid prize pool amounts
- Participant overflow
- Tournament status transitions
Realistic test data:
- Prize pool should typically be greater than a single entry fee
- Test with multiple participants
Example test cases to add:
#[test] fn test_tournament_validation() { // Test maximum participants let tournament_max = create_tournament_with_max_participants(); assert(tournament_max.current_participants.len() <= tournament_max.max_participants); // Test prize pool validation let tournament_prize = create_tournament_with_prize_pool(); assert(tournament_prize.prize_pool >= tournament_prize.entry_fee * tournament_prize.max_participants); } #[test] fn test_tournament_with_multiple_participants() { let tournament = create_tournament_with_multiple_players(); assert(tournament.current_participants.len() > 1); }src/systems/tournament.cairo (2)
18-18: TODO: Implementcomplete_tournamentmethod.The commented-out method suggests incomplete implementation. This aligns with the PR objectives mentioning that
complete_tournamentneeds to be implemented.Would you like me to help implement the
complete_tournamentmethod? I can provide a basic implementation that:
- Verifies the tournament is in Ongoing status
- Updates the tournament status to Completed
- Handles prize distribution to the winning player
1-89: Consider architectural improvements for scalability.A few architectural suggestions:
- Consider implementing events for tournament state changes to facilitate frontend updates
- Think about implementing a separate prize pool management system for better modularity
- Consider adding tournament types/formats for future extensibility
Example event implementation:
#[event] fn TournamentCreated(tournament_id: u32, name: felt252) {} #[event] fn TournamentStarted(tournament_id: u32) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/lib.cairo(2 hunks)src/models/tournament.cairo(1 hunks)src/systems/tournament.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.cairo
🔇 Additional comments (5)
src/models/tournament.cairo (3)
1-8: LGTM! Well-structured enum implementation.
The TournamentStatus enum is well-designed with appropriate trait derivations and clear status values that properly represent a tournament's lifecycle.
11-22: 🛠️ Refactor suggestion
Add documentation and field constraints.
While the struct implementation matches the requirements, it needs documentation and field constraints:
- Add documentation for the struct and each field
- Consider adding the following constraints in the tournament system contract:
entry_fee: Should be non-negativemax_participants: Should have a reasonable upper limitprize_pool: Should be >= total entry fees (entry_fee * max_participants)
#[derive(Drop, Serde)]
#[dojo::model]
+/// Represents a tournament in the ByteBeasts game
pub struct Tournament {
#[key]
+ /// Unique identifier for the tournament
pub tournament_id: u32,
+ /// Name of the tournament
pub name: felt252,
+ /// Current status of the tournament (Pending, Ongoing, or Completed)
pub status: TournamentStatus,
+ /// Entry fee required to participate (must be non-negative)
pub entry_fee: u32,
+ /// Maximum number of participants allowed
pub max_participants: u32,
+ /// Array of currently registered participants
pub current_participants: Array<Player>,
+ /// Total prize pool (must be >= entry_fee * max_participants)
pub prize_pool: u32,
}11-22: Verify struct design supports upcoming tournament functions.
The current struct design needs to support the pending functions mentioned in the PR objectives (register_player, start_tournament, complete_tournament). Let's verify the implementation:
✅ Verification successful
The initial results show that the functions are being implemented, but we need to verify the complete implementation details and the TournamentStatus enum to ensure the struct design properly supports these functions.
Tournament struct design adequately supports the system implementation
The Tournament struct design properly supports all the implemented and planned tournament functions:
register_player: Struct hascurrent_participantsarray andmax_participantsfor validationstart_tournament: Struct hasstatusfield to track tournament state transitionscomplete_tournament(planned): Struct includesstatusenum withCompletedstate andprize_poolfor rewards
The implementation shows proper usage of all struct fields with appropriate validations in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing tournament-related function implementations
echo "Searching for tournament-related functions..."
rg -A 5 "fn (register_player|start_tournament|complete_tournament)"
# Check for any existing tournament system implementations
echo "Checking for tournament system implementations..."
fd -e cairo -x grep -l "tournament" {}
Length of output: 1960
Script:
#!/bin/bash
# Let's check the tournament system implementation details
echo "Checking tournament system implementation..."
cat src/systems/tournament.cairo
# Let's also check the tournament status enum
echo "\nChecking tournament status definition..."
rg -A 5 "enum TournamentStatus"
Length of output: 3683
src/systems/tournament.cairo (2)
1-5: LGTM! Imports and interface declaration are well-structured.
The imports and interface declaration follow the dojo patterns correctly.
23-31: LGTM! Contract implementation follows dojo patterns.
The contract structure and interface implementation are correctly defined.
| assert!( | ||
| tournament.current_participants.len() < tournament.max_participants.try_into().unwrap(), | ||
| "Tournament is full" | ||
| ); |
There was a problem hiding this comment.
Fix potential integer overflow in length comparison.
The current comparison between len() and max_participants involves unsafe type conversion.
Consider this safer approach:
- assert!(
- tournament.current_participants.len() < tournament.max_participants.try_into().unwrap(),
- "Tournament is full"
- );
+ let current_count: u32 = tournament.current_participants.len().try_into().unwrap();
+ assert!(current_count < tournament.max_participants, "Tournament is full");📝 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.
| assert!( | |
| tournament.current_participants.len() < tournament.max_participants.try_into().unwrap(), | |
| "Tournament is full" | |
| ); | |
| let current_count: u32 = tournament.current_participants.len().try_into().unwrap(); | |
| assert!(current_count < tournament.max_participants, "Tournament is full"); |
| fn register_player(ref world: IWorldDispatcher, tournament_id: u32, new_player: Player) { | ||
| let mut tournament = get!(world, tournament_id, (Tournament)); | ||
|
|
||
| assert!(tournament.status == TournamentStatus::Pending, "Tournament not open for registration"); | ||
|
|
||
| assert!( | ||
| tournament.current_participants.len() < tournament.max_participants.try_into().unwrap(), | ||
| "Tournament is full" | ||
| ); | ||
|
|
||
| tournament.current_participants.append(new_player); | ||
|
|
||
| set!(world, (tournament)); | ||
| } |
There was a problem hiding this comment.
Add player validation and entry fee handling.
The register_player method needs additional validations:
- Check for duplicate players
- Handle entry fee payment
Consider adding these validations:
fn register_player(ref world: IWorldDispatcher, tournament_id: u32, new_player: Player) {
let mut tournament = get!(world, tournament_id, (Tournament));
assert!(tournament.status == TournamentStatus::Pending, "Tournament not open for registration");
assert!(
tournament.current_participants.len() < tournament.max_participants.try_into().unwrap(),
"Tournament is full"
);
+ // Check for duplicate players
+ let mut i = 0;
+ loop {
+ if i >= tournament.current_participants.len() {
+ break;
+ }
+ assert!(
+ tournament.current_participants[i].player_id != new_player.player_id,
+ "Player already registered"
+ );
+ i += 1;
+ };
+
+ // TODO: Handle entry fee payment
+ // This would require integration with a payment system
+ // assert!(payment_received == tournament.entry_fee, "Entry fee not paid");
tournament.current_participants.append(new_player);Committable suggestion skipped: line range outside the PR's diff.
| fn create_tournament( | ||
| ref world: IWorldDispatcher, | ||
| tournament_id: u32, | ||
| name: felt252, | ||
| status: TournamentStatus, | ||
| entry_fee: u32, | ||
| max_participants: u32, | ||
| current_participants: Array<Player>, | ||
| prize_pool: u32 | ||
| ) { | ||
| let tournament = Tournament { | ||
| tournament_id: tournament_id, | ||
| name: name, | ||
| status: status, | ||
| entry_fee: entry_fee, | ||
| max_participants: max_participants, | ||
| current_participants: current_participants, | ||
| prize_pool: prize_pool | ||
| }; | ||
| set!(world, (tournament)) | ||
| } |
There was a problem hiding this comment.
Add validation for tournament ID uniqueness.
The create_tournament method should verify that the tournament_id doesn't already exist to prevent overwriting existing tournaments.
Add this validation before creating the tournament:
prize_pool: u32
) {
+ let existing = get!(world, tournament_id, (Tournament));
+ assert!(existing.tournament_id == 0, "Tournament ID already exists");
let tournament = Tournament {Committable suggestion skipped: line range outside the PR's diff.
| fn get_tournament(world: @IWorldDispatcher, tournament_id: u32) -> Tournament { | ||
| let tournament_from_world = get!(world, tournament_id, (Tournament)); | ||
| tournament_from_world | ||
| } |
There was a problem hiding this comment.
Add error handling for non-existent tournament.
The get_tournament method should handle the case when the tournament doesn't exist.
Add error handling:
fn get_tournament(world: @IWorldDispatcher, tournament_id: u32) -> Tournament {
let tournament_from_world = get!(world, tournament_id, (Tournament));
+ assert!(tournament_from_world.tournament_id != 0, "Tournament not found");
tournament_from_world
}📝 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.
| fn get_tournament(world: @IWorldDispatcher, tournament_id: u32) -> Tournament { | |
| let tournament_from_world = get!(world, tournament_id, (Tournament)); | |
| tournament_from_world | |
| } | |
| fn get_tournament(world: @IWorldDispatcher, tournament_id: u32) -> Tournament { | |
| let tournament_from_world = get!(world, tournament_id, (Tournament)); | |
| assert!(tournament_from_world.tournament_id != 0, "Tournament not found"); | |
| tournament_from_world | |
| } |
|
@Gerson2102 are sozo build and sozo test working? |
|
@danielcdz yes
|
|
The CI is failing for a weird reason. Seems like is running out of memory, I remember to have faced that kind of issues locally with other projects. But I'm not sure if it is that. |
Closes #93. I just created the model and system blueprint to keep working on it. Any feedback is more then welcome. I still need to figure it out how to do the remaining functions. (register_player, start_tournament and complete_torunament)
And btw, how can I check that the code is working fine, when I run
sozo buildnothing happens. See this picture:Summary by CodeRabbit
New Features
Bug Fixes
Documentation