feat: add first leaderboard model implementation with tests #39
Conversation
| #[dojo::model] | ||
| pub struct LeaderboardEntry { | ||
| #[key] | ||
| pub player_address: ContractAddress, // On-chain player address |
There was a problem hiding this comment.
As we are handling the player just with an ID instead of the address, we can keep that behavior here. What do you think?
| pub name: felt252, // Leaderboard name (e.g., "Global", "Monthly") | ||
| pub description: felt252, // Description of what this leaderboard tracks | ||
| pub entries: Array<LeaderboardEntry>, // List of leaderboard entries | ||
| pub last_updated: u64, // Timestamp of last update |
There was a problem hiding this comment.
what is the idea to use u64 here for the timestamp? are we going to use a format like this? 20240708
| // Add new entry | ||
| self.entries.append(entry); | ||
| // Update timestamp | ||
| self.last_updated = starknet::get_block_timestamp(); |
There was a problem hiding this comment.
what is the format of the returned value of this function? 😮
| use super::{LeaderboardTrait, MAX_ENTRIES}; | ||
| use bytebeasts::models::leaderboard::{Leaderboard, LeaderboardEntry}; | ||
|
|
||
| fn create_mock_entry(address: ContractAddress, name: felt252, score: u32, wins: u32, losses: u32, highest_score: u32, is_active: bool) -> LeaderboardEntry { |
| let mut leaderboard = create_empty_leaderboard(); | ||
| let entry = create_mock_entry(starknet::get_contract_address(), 'Alice', 100, 20, 5, 1000, true); | ||
|
|
||
| leaderboard.add_entry(entry).expect('Failed to add entry'); |
There was a problem hiding this comment.
love the idea for the game and i would be a marvelous implementation, but please take a look of @danielcdz review first and fix those errors
WalkthroughThe pull request introduces a comprehensive implementation of a leaderboard model within the bytebeasts context. It includes two main JSON files that define the data structures and interfaces necessary for managing leaderboard entries and overall leaderboard data. Key structures such as Additionally, several enumerations and span structures are introduced to handle optional values and collections of data effectively. The TOML files provide the necessary configuration for the models, detailing their members and types. A new module for the leaderboard is added to the existing library structure, along with a dedicated file that encapsulates the logic for managing leaderboard behavior, including methods for adding entries and checking leaderboard capacity. This implementation is supported by unit tests to ensure functionality. Assessment against linked 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json (2)
364-368: Consider adding implementation documentationWhile the implementation is correct, consider adding a documentation comment describing the purpose and responsibilities of the leaderboard_entryImpl.
421-438: Consider expanding interface with required functionalityBased on the PR objectives, the interface should include methods for:
- Score calculation and updates
- Rank management
- Entry ordering
- Entry validation
This will help implement the mentioned requirements for score logic and leaderboard ordering.
Example additions:
{ "type": "interface", "name": "bytebeasts::models::leaderboard::Ileaderboard_entry", "items": [ { "type": "function", "name": "ensure_abi", "inputs": [ { "name": "model", "type": "bytebeasts::models::leaderboard::LeaderboardEntry" } ], "outputs": [], "state_mutability": "view" }, + { + "type": "function", + "name": "update_score", + "inputs": [ + { + "name": "new_score", + "type": "core::integer::u32" + } + ], + "outputs": [], + "state_mutability": "external" + }, + { + "type": "function", + "name": "update_rank", + "inputs": [ + { + "name": "new_rank", + "type": "core::integer::u32" + } + ], + "outputs": [], + "state_mutability": "external" + } ] }manifests/dev/base/abis/models/bytebeasts-Leaderboard-7e680376.json (2)
384-420: Consider adding validation constraints for LeaderboardEntry fields.While the structure captures all necessary player statistics, consider adding validation constraints for:
- Score and rank ranges
- Player name length/format
- Win/loss ratio validation
This will help maintain data integrity and prevent invalid entries.
422-446: Consider adding capacity constraints for the entries array.The Leaderboard structure should define a maximum capacity for entries to:
- Prevent unbounded growth
- Ensure consistent performance
- Manage memory usage effectively
Consider adding a
max_capacityfield or implementing a fixed-size array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- manifests/dev/base/abis/models/bytebeasts-Leaderboard-7e680376.json (1 hunks)
- manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json (1 hunks)
- manifests/dev/base/models/bytebeasts-Leaderboard-7e680376.toml (1 hunks)
- manifests/dev/base/models/bytebeasts-LeaderboardEntry-7237950c.toml (1 hunks)
- src/lib.cairo (1 hunks)
- src/models/leaderboard.cairo (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- manifests/dev/base/models/bytebeasts-Leaderboard-7e680376.toml
🔇 Additional comments (7)
src/lib.cairo (1)
25-25: LGTM! Module declaration follows project structure.The addition of the leaderboard module in the models section is well-placed and follows the existing architectural patterns.
Let's verify the implementation details of the leaderboard module:
✅ Verification successful
LGTM! Leaderboard module is well-implemented with comprehensive functionality.
The leaderboard module implementation is robust and complete:
- Properly structured with
LeaderboardEntryandLeaderboardmodels using#[dojo::model]- Implements core functionality with
LeaderboardBehaviortrait- Includes comprehensive test coverage with multiple test cases
- Has proper validation (e.g., max entries limit)
- Well-documented with clear comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the leaderboard module implementation matches the JSON contract # Test 1: Check if the leaderboard module exists and contains expected structures ast-grep --pattern 'struct Leaderboard { $$$ }' # Test 2: Check if the module implements the IModel interface ast-grep --pattern 'impl $_ of IModel { $$$ }' # Test 3: Look for test coverage fd "test.*leaderboard.*cairo$"Length of output: 135
Script:
#!/bin/bash # Let's try a different approach to verify the leaderboard implementation # Check if the leaderboard module file exists and show its content fd "leaderboard.cairo$" --exec cat {} # Search for any references to leaderboard in the codebase rg -i "leaderboard" -A 3 -B 3 # Look for any model-related interfaces or traits that might be implemented rg "trait IModel" -A 5Length of output: 25670
manifests/dev/base/models/bytebeasts-LeaderboardEntry-7237950c.toml (3)
18-27: Consider adding constraints for score and rank.A few suggestions for the score and rank fields:
- Add validation to ensure score is non-negative
- Consider if rank should be a key field to enforce uniqueness
- Add documentation about how rank updates are handled (is it automatically calculated based on score?)
Let's check for similar ranking implementations in the codebase:
#!/bin/bash # Search for similar ranking implementations rg -A 5 "name.*rank" --type toml
13-17: Consider using a string type for player_name.The
felt252type might have limitations for storing player names. Consider if a string type would be more appropriate to handle various character sets and name lengths.Let's check if there are any existing string implementations in the codebase:
#!/bin/bash # Search for string type implementations rg -l "type.*String" --type toml
1-7: LGTM! Verify ABI file existence.The model metadata is well-structured with appropriate class hashes and consistent naming.
Let's verify the referenced ABI file exists:
✅ Verification successful
ABI file verification successful
The referenced ABI file exists at the correct location and follows the expected naming convention. The model metadata is properly configured with the corresponding ABI file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the ABI file exists and is valid JSON if [ -f "manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json" ]; then jq . "manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json" > /dev/null echo "ABI file exists and is valid JSON" else echo "ABI file not found" fiLength of output: 205
Script:
#!/bin/bash # Let's try a different approach to verify the ABI file ls -l manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json 2>/dev/null || echo "File not found" # Also search for any similar ABI files to ensure we're looking in the right place fd LeaderboardEntry.*.json manifests/Length of output: 317
manifests/dev/base/abis/models/bytebeasts-LeaderboardEntry-7237950c.json (1)
1-363: LGTM: Core model implementation follows Dojo standardsThe implementation properly implements the IModel interface with all required methods and includes comprehensive layout and introspection capabilities.
manifests/dev/base/abis/models/bytebeasts-Leaderboard-7e680376.json (2)
1-363: LGTM: Core model implementation follows Dojo standards.The implementation correctly follows the Dojo model patterns with all required interface methods properly defined.
384-446: Verify alignment with battle tracking requirements.While the current implementation tracks basic metrics, consider adding battle-specific fields to better align with issue #28 requirements:
- Battle participation count
- Battle win streak
- Last battle timestamp
- Battle-specific achievements
✅ Verification successful
Leaderboard model aligns well with battle system implementation
The current leaderboard model implementation with wins, losses, and scores is sufficient and well-aligned with the battle system's functionality. The battle system tracks battle outcomes through the
battle_activestate and emits appropriate events for wins/losses, which can directly update the leaderboard metrics. Additional suggested fields are unnecessary because:
- Battle participation is implicitly tracked through wins + losses
- Win streaks can be computed from the battle history if needed
- Battle timestamps are handled by
last_updatedfield- Battle-specific achievements would be better suited in a separate achievements system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for battle-related functionality in the codebase echo "Searching for battle-related files and implementations..." fd -e cairo battle rg "battle|combat|fight" -t cairoLength of output: 288
Script:
#!/bin/bash # Let's examine the battle-related files content echo "=== Checking battle model structure ===" cat src/models/battle.cairo echo -e "\n=== Checking battle system implementation ===" cat src/systems/battle.cairoLength of output: 8041
| [[members]] | ||
| name = "player_address" | ||
| type = "ContractAddress" | ||
| key = true | ||
|
|
||
| [[members]] | ||
| name = "player_name" | ||
| type = "felt252" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "score" | ||
| type = "u32" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "rank" | ||
| type = "u32" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "wins" | ||
| type = "u32" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "losses" | ||
| type = "u32" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "highest_score" | ||
| type = "u32" | ||
| key = false | ||
|
|
||
| [[members]] | ||
| name = "is_active" | ||
| type = "bool" | ||
| key = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timestamp fields.
To improve data tracking and enable features like:
- Leaderboard history
- Activity monitoring
- Seasonal rankings
Consider adding:
created_at: When the entry was first createdupdated_at: Last time any stats were modified
Also, for future scalability, consider:
- Adding a
season_idfield if you plan to implement seasonal leaderboards - Including a
total_gamesfield (wins + losses might not cover draws/disconnects)
| { | ||
| "type": "struct", | ||
| "name": "bytebeasts::models::leaderboard::LeaderboardEntry", | ||
| "members": [ | ||
| { | ||
| "name": "player_address", | ||
| "type": "core::starknet::contract_address::ContractAddress" | ||
| }, | ||
| { | ||
| "name": "player_name", | ||
| "type": "core::felt252" | ||
| }, | ||
| { | ||
| "name": "score", | ||
| "type": "core::integer::u32" | ||
| }, | ||
| { | ||
| "name": "rank", | ||
| "type": "core::integer::u32" | ||
| }, | ||
| { | ||
| "name": "wins", | ||
| "type": "core::integer::u32" | ||
| }, | ||
| { | ||
| "name": "losses", | ||
| "type": "core::integer::u32" | ||
| }, | ||
| { | ||
| "name": "highest_score", | ||
| "type": "core::integer::u32" | ||
| }, | ||
| { | ||
| "name": "is_active", | ||
| "type": "core::bool" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timestamp and validation constraints
The LeaderboardEntry struct has good coverage of player statistics, but consider these enhancements:
- Add timestamps for entry creation and last update to track entry history
- Consider adding validation constraints for:
- Maximum player name length
- Score ranges
- Rank boundaries
This will help maintain data integrity and provide better tracking capabilities.
Example addition:
{
"type": "struct",
"name": "bytebeasts::models::leaderboard::LeaderboardEntry",
"members": [
{
"name": "player_address",
"type": "core::starknet::contract_address::ContractAddress"
},
+ {
+ "name": "created_at",
+ "type": "core::integer::u64"
+ },
+ {
+ "name": "updated_at",
+ "type": "core::integer::u64"
+ },
// ... existing fields ...
]
}Committable suggestion was skipped due to low confidence.
| { | ||
| "type": "event", | ||
| "name": "bytebeasts::models::leaderboard::leaderboard_entry::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| } | ||
| ] |
There was a problem hiding this comment.
Add essential events for state changes
The Event enum is currently empty. Consider adding events for important state changes:
- Entry creation
- Score updates
- Rank changes
- Status changes (active/inactive)
These events will help track changes and enable external systems to react to leaderboard updates.
Example implementation:
{
"type": "event",
"name": "bytebeasts::models::leaderboard::leaderboard_entry::Event",
"kind": "enum",
- "variants": []
+ "variants": [
+ {
+ "name": "EntryCreated",
+ "type": "bytebeasts::models::leaderboard::LeaderboardEntry"
+ },
+ {
+ "name": "ScoreUpdated",
+ "type": "(core::starknet::contract_address::ContractAddress, core::integer::u32)"
+ },
+ {
+ "name": "RankChanged",
+ "type": "(core::starknet::contract_address::ContractAddress, core::integer::u32)"
+ },
+ {
+ "name": "StatusChanged",
+ "type": "(core::starknet::contract_address::ContractAddress, core::bool)"
+ }
+ ]
}Committable suggestion was skipped due to low confidence.
| "type": "event", | ||
| "name": "bytebeasts::models::leaderboard::leaderboard::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| } |
There was a problem hiding this comment.
Add essential leaderboard events.
The empty Event type should include crucial events such as:
- Entry added/updated/removed
- Rank changes
- Score updates
- Leaderboard reset
These events are essential for:
- Tracking leaderboard changes
- Implementing notifications
- Auditing purposes
Example event structure:
{
"type": "event",
"name": "bytebeasts::models::leaderboard::leaderboard::Event",
"kind": "enum",
"variants": [
{
"name": "EntryAdded",
"type": "bytebeasts::models::leaderboard::LeaderboardEntry"
},
{
"name": "EntryUpdated",
"type": "(bytebeasts::models::leaderboard::LeaderboardEntry, bytebeasts::models::leaderboard::LeaderboardEntry)"
},
{
"name": "EntryRemoved",
"type": "core::starknet::contract_address::ContractAddress"
},
{
"name": "LeaderboardReset",
"type": "core::integer::u64"
}
]
}| trait LeaderboardBehavior { | ||
| fn add_entry(ref self: Leaderboard, entry: LeaderboardEntry) -> Result<(), felt252>; | ||
| fn is_full(self: @Leaderboard) -> bool; | ||
| } |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| fn add_entry(ref self: Leaderboard, mut entry: LeaderboardEntry) -> Result<(), felt252> { | ||
| if self.is_full() { | ||
| return Result::Err('Leaderboard is full'); | ||
| } | ||
| // Assign an initial value | ||
| entry.rank = self.entries.len() + 1; | ||
| // Add new entry | ||
| self.entries.append(entry); |
There was a problem hiding this comment.
Prevent duplicate entries for the same player
There is no check to prevent adding multiple entries for the same player. This could lead to duplicate entries on the leaderboard. Consider checking if the player's address already exists in the leaderboard before adding a new entry.
You can add a check like this:
// Before adding the new entry
+ // Check for existing entry
+ if self.entries.iter().any(|e| e.player_address == entry.player_address) {
+ return Result::Err('Player already exists in leaderboard');
+ }
// Assign an initial rank or update ranks after sorting
entry.rank = self.entries.len() + 1;Committable suggestion was skipped due to low confidence.
| entry.rank = self.entries.len() + 1; | ||
| // Add new entry | ||
| self.entries.append(entry); | ||
| // Update timestamp | ||
| self.last_updated = starknet::get_block_timestamp(); | ||
| Result::Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rank assignment should be based on player score, not insertion order
Currently, entry.rank is assigned based on the order of insertion (self.entries.len() + 1), which may not accurately reflect the players' standings. To properly rank players, consider sorting the entries by score after adding a new entry and updating the ranks accordingly.
Here's how you might adjust the code:
// Add new entry
self.entries.append(entry);
+ // Sort entries by score in descending order
+ self.entries.sort_unstable_by(|a, b| b.score.cmp(&a.score));
+ // Update ranks
+ let mut rank = 1_u32;
+ for e in self.entries.iter_mut() {
+ e.rank = rank;
+ rank += 1;
+ }
// Update timestamp
self.last_updated = starknet::get_block_timestamp();Committable suggestion was skipped due to low confidence.
📝 Pull Request Overview
Implements Leaderboard functionality.
Closes Implement leaderboard model/entity and functions #28
🔄 Changes Made
Leaderboard Model Creation:
Leaderboardmodel inleaderboard.cairowith the following attributes:leaderboard_idnamedescriptionentrieslast_updatedLeaderboardEntrystruct to represent individual entries in the leaderboard.Comprehensive Tests:
Leaderboardmodel including:🔧 Testing
🔜 Next Steps
Summary by CodeRabbit
New Features
LeaderboardandLeaderboardEntry.Documentation
bytebeasts-Leaderboardandbytebeasts-LeaderboardEntrymodels, outlining their structure and properties.Tests