Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move setWorld to init method #73

Conversation

evgongora
Copy link

@evgongora evgongora commented Oct 8, 2024

Pull Request Overview

📝 Summary

Changes to setWorld method to dojo_init function following example: https://github.com/dojoengine/dojo/blob/d039c6d46f819edc3f2161c0520b8c8fecec0092/examples/spawn-and-move/src/others.cairo#L18

🔄 Changes Made

  • Added dojo_init function inside the world contract module, this function directly initializes the world elements
  • Removed modular interface IWorldSetup and its implementation WorldSetupImpl
  • The setWorld function, which was part of the interface, has been removed.

🔧 Tests Results

image

image

🔜 Next Steps

Summary by CodeRabbit

  • New Features
    • Introduced a new initialization function, dojo_init, for setting up game entities like Beast, Player, Potion, and Mt.
  • Improvements
    • Simplified the module by removing the IWorldSetup trait, enhancing direct access to initialization logic and reducing complexity.

@danielcdz danielcdz added enhancement New feature or request external-contributor Available for external contributors labels Oct 8, 2024
@evgongora evgongora marked this pull request as ready for review October 9, 2024 00:46
Copy link

coderabbitai bot commented Oct 9, 2024

Walkthrough

The changes in the world_setup.cairo file involve the removal of the IWorldSetup trait and its setWorld method, which accepted an IWorldDispatcher reference. A new function, dojo_init, is introduced to replace this functionality, directly initializing game entities such as Beast, Player, Potion, and Mt. This refactor simplifies the module by eliminating the trait and consolidating the initialization logic.

Changes

File Change Summary
src/systems/world_setup.cairo Removed IWorldSetup trait and setWorld method; added dojo_init function for world initialization.

Assessment against linked issues

Objective Addressed Explanation
Create init so we create the world entities when the contracts are deployed (#13)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/systems/world_setup.cairo (4)

Line range hint 9-17: LGTM! Consider updating the file name.

The module renaming from world_setup to world aligns well with the refactoring changes. The imports are correctly maintained.

Consider renaming the file from world_setup.cairo to world.cairo to match the new module name, improving consistency.


21-62: LGTM! Consider refactoring for improved maintainability.

The Beast initialization logic is correct and comprehensive. However, there are opportunities for improvement:

  1. Consider extracting the Beast initialization into a separate function for better code organization.
  2. The initialization of multiple Beasts follows a similar pattern, which could be abstracted into a reusable function to reduce code duplication.

Here's a suggested refactor to improve maintainability:

fn initialize_beast(
    world: @IWorldDispatcher,
    beast_id: u32,
    beast_name: felt252,
    beast_type: WorldElements,
    beast_description: felt252,
    player_id: u32,
    hp: u32,
    attack: u32,
    defense: u32,
    mt1: u32,
    mt2: u32,
    mt3: u32,
    mt4: u32,
) {
    set!(
        world,
        (Beast {
            beast_id,
            beast_name,
            beast_type,
            beast_description,
            player_id,
            hp,
            current_hp: hp,
            attack,
            defense,
            mt1,
            mt2,
            mt3,
            mt4,
            level: 5,
            experience_to_next_level: 1000
        })
    );
}

// Usage in dojo_init:
initialize_beast(
    world,
    1,
    'Firebeast',
    WorldElements::Draconic(()),
    'A fiery beast.',
    1,
    100,
    50,
    40,
    1,
    2,
    3,
    4
);

initialize_beast(
    world,
    2,
    'Aqua',
    WorldElements::Crystal(()),
    'A water beast',
    2,
    110,
    45,
    50,
    5,
    6,
    7,
    8
);

This refactoring improves code readability and makes it easier to add new Beasts in the future.


64-89: LGTM! Consider refactoring Player initialization for consistency.

The Player initialization logic is correct. To maintain consistency with the suggested Beast initialization refactor and improve maintainability, consider applying a similar approach to the Player initialization.

Here's a suggested refactor to improve consistency and maintainability:

fn initialize_player(
    world: @IWorldDispatcher,
    player_id: u32,
    player_name: felt252,
    beast_1: u32,
    beast_2: u32,
    beast_3: u32,
    beast_4: u32,
    potions: u32
) {
    set!(
        world,
        (Player {
            player_id,
            player_name,
            beast_1,
            beast_2,
            beast_3,
            beast_4,
            potions
        })
    );
}

// Usage in dojo_init:
initialize_player(world, 1, 'Ash', 1, 0, 0, 0, 2);
initialize_player(world, 2, 'Misty', 2, 0, 0, 0, 3);

This refactoring aligns the Player initialization with the Beast initialization style, improving overall code consistency and maintainability.


91-182: LGTM! Consider refactoring Potion and Mt initializations for improved maintainability.

The Potion and Mt (Move Type) initializations are correct. However, there's an opportunity to improve code organization and reduce duplication, especially for the Mt initializations.

Here are suggested refactors to improve maintainability:

  1. For Potions:
fn initialize_potion(
    world: @IWorldDispatcher,
    potion_id: u32,
    potion_name: felt252,
    potion_effect: u32
) {
    set!(world, (Potion { potion_id, potion_name, potion_effect }));
}

// Usage in dojo_init:
initialize_potion(world, 1, 'Health Potion', 50);
initialize_potion(world, 2, 'Super Potion', 100);
  1. For Mts:
fn initialize_mt(
    world: @IWorldDispatcher,
    mt_id: u32,
    mt_name: felt252,
    mt_type: WorldElements,
    mt_power: u32,
    mt_accuracy: u32
) {
    set!(
        world,
        (Mt { mt_id, mt_name, mt_type, mt_power, mt_accuracy })
    );
}

// Usage in dojo_init:
initialize_mt(world, 1, 'Fire Blast', WorldElements::Draconic(()), 90, 85);
initialize_mt(world, 2, 'Ember', WorldElements::Crystal(()), 40, 100);
initialize_mt(world, 3, 'Flame Wheel', WorldElements::Draconic(()), 60, 95);
initialize_mt(world, 4, 'Fire Punch', WorldElements::Crystal(()), 75, 100);
initialize_mt(world, 5, 'Water Gun', WorldElements::Crystal(()), 40, 100);
initialize_mt(world, 6, 'Bubble', WorldElements::Draconic(()), 20, 100);
initialize_mt(world, 7, 'Aqua Tail', WorldElements::Crystal(()), 90, 90);
initialize_mt(world, 8, 'Hydro Pump', WorldElements::Crystal(()), 110, 80);

These refactorings significantly reduce code duplication, improve readability, and make it easier to add new Potions or Mts in the future. They also provide a consistent initialization pattern across all entity types in the dojo_init function.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 367ad2a and cf3dd78.

📒 Files selected for processing (1)
  • src/systems/world_setup.cairo (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/systems/world_setup.cairo (2)

18-20: LGTM! Function declaration aligns with PR objectives.

The dojo_init function declaration is correct and properly annotated with #[dojo::contract]. It takes the expected world parameter of type @IWorldDispatcher, which aligns with the PR objectives of creating an initialization method for world entities.


Line range hint 1-183: Overall assessment: Changes meet PR objectives with room for further improvements.

The refactoring successfully achieves the goal of creating a dedicated dojo_init method for initializing world entities, aligning with the PR objectives. The removal of the IWorldSetup trait and setWorld method simplifies the module structure.

Key points:

  1. The initialization logic for all entity types (Beast, Player, Potion, Mt) has been correctly moved into the dojo_init function.
  2. The changes maintain the existing functionality while improving the code structure.

Suggestions for further improvement:

  1. Rename the file to world.cairo to match the new module name.
  2. Implement the suggested refactorings for Beast, Player, Potion, and Mt initializations to reduce code duplication and improve maintainability.

These changes have significantly improved the codebase, and implementing the suggested refactors would further enhance its quality and maintainability.

To ensure that the changes haven't introduced any regressions, please run the existing test suite and confirm that all tests pass.

@RolandoDrRobot
Copy link
Contributor

@evgongora Hello Erick could you post the following testing

  1. sozo build
  2. A pic of the successful deploy of the contracts sozo migrate appply
  3. right after that, could you open Torii and check that those first values for those models exists in Katana/Torii
  4. Last, sozo test
    Thanks!

great job so far

@RolandoDrRobot
Copy link
Contributor

@evgongora could you describe in a comment how did you fix the deploy thing? remember? It was like a versions problem with the rpc, thanks

@evgongora
Copy link
Author

Hello @RolandoDrRobot!
Per my conversation with Glihm I was able to get the next information:

Important

The init function wil keep being the same as dojo_init since we need permissions to be set first, hence the contract must be registered first. This so when you register a contract in dojo using the sozo migrate, constructor is always empty. Then you use the dojo_init, that can be called once.

Note

The error that is showing when using the new dojo_init is because of sozo old versions.

@RolandoDrRobot
Copy link
Contributor

This one is not gonna have any coed merged, I'll close It, thanks @evgongora for the findings and effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external-contributor Available for external contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create init so we create the world entities when the contracts are deployed
3 participants