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

update permission api #2224

Merged
merged 1 commit into from
Jul 30, 2024
Merged

update permission api #2224

merged 1 commit into from
Jul 30, 2024

Conversation

remybar
Copy link
Contributor

@remybar remybar commented Jul 29, 2024

issue #2221

Summary by CodeRabbit

  • New Features

    • Enhanced function signatures for ownership management functions: is_owner, grant_owner, and revoke_owner to improve clarity by reordering input parameters.
  • Bug Fixes

    • Corrected parameter order in ownership management methods to ensure consistent and expected behavior.
  • Chores

    • Updated configuration files to reflect new class hashes, indicating revisions in class implementations across various environments.

Copy link

coderabbitai bot commented Jul 29, 2024

Walkthrough

Ohayo, sensei! The recent changes focus on reordering parameters in ownership-related functions across multiple files, including is_owner, grant_owner, and revoke_owner. This adjustment aims to improve clarity and consistency in the codebase. Additionally, updates to class hashes and contract addresses indicate potential changes in implementation and deployment. Overall, these modifications enhance the usability and integrity of ownership management within the system.

Changes

Files Change Summary
crates/dojo-core/src/tests/world.cairo, crates/dojo-core/src/world/world_contract.cairo, crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json, crates/dojo-world/src/contracts/abi/world.rs, crates/sozo/ops/src/auth.rs, examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json, examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json Reordered parameters in is_owner, grant_owner, and revoke_owner functions to place resource first, aligning with intended logic.
crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml, examples/spawn-and-move/manifests/dev/base/dojo-world.toml, examples/spawn-and-move/manifests/release/base/dojo-world.toml Updated class_hash and original_class_hash values, indicating changes in class implementation or metadata.
examples/spawn-and-move/Scarb.toml Modified world_address to a new contract address, affecting interactions in the application.
examples/spawn-and-move/manifests/dev/deployment/manifest.json, examples/spawn-and-move/manifests/dev/deployment/manifest.toml Adjusted class_hash, original_class_hash, address, and transaction_hash fields across contracts, reflecting updates due to redeployment.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Contract
    User->>Contract: Call grant_owner(resource, address)
    activate Contract
    Contract-->>User: Ownership granted
    deactivate Contract

    User->>Contract: Call revoke_owner(resource, address)
    activate Contract
    Contract-->>User: Ownership revoked
    deactivate Contract

    User->>Contract: Call is_owner(resource, address)
    activate Contract
    Contract-->>User: Ownership status
    deactivate Contract
Loading

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f2067e2 and cb644a1.

Files selected for processing (15)
  • crates/dojo-core/src/tests/world.cairo (10 hunks)
  • crates/dojo-core/src/world/world_contract.cairo (6 hunks)
  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json (3 hunks)
  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml (1 hunks)
  • crates/dojo-world/src/contracts/abi/world.rs (3 hunks)
  • crates/sozo/ops/src/auth.rs (2 hunks)
  • crates/sozo/ops/src/tests/model.rs (1 hunks)
  • examples/spawn-and-move/Scarb.toml (1 hunks)
  • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json (3 hunks)
  • examples/spawn-and-move/manifests/dev/base/dojo-world.toml (1 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json (3 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.json (9 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml (5 hunks)
  • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json (3 hunks)
  • examples/spawn-and-move/manifests/release/base/dojo-world.toml (1 hunks)
Files skipped from review due to trivial changes (3)
  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json
  • examples/spawn-and-move/Scarb.toml
  • examples/spawn-and-move/manifests/release/base/dojo-world.toml
Files skipped from review as they are similar to previous changes (9)
  • crates/dojo-core/src/tests/world.cairo
  • crates/dojo-core/src/world/world_contract.cairo
  • crates/dojo-world/src/contracts/abi/world.rs
  • crates/sozo/ops/src/auth.rs
  • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json
  • examples/spawn-and-move/manifests/dev/base/dojo-world.toml
  • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml
  • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json
Additional comments not posted (4)
crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml (1)

2-3: Ohayo, sensei! Verify the propagation of new hash values.

The class_hash and original_class_hash have been updated. Ensure that these new values are correctly propagated throughout the system to avoid inconsistencies.

Verification successful

Ohayo, sensei! The new hash values have been successfully propagated throughout the system.

The old hash value 0x32fb65ebfe6d91bb4b7ba0640650722c43ca0c917f5fc0f649ee2ecf720cde is no longer present, and the new hash value 0x4c90da98d2bad157dec1d7b9b9c8c5861828a7ec1b323425d84fa6c3071303f is correctly found in all relevant files.

  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml
  • examples/spawn-and-move/manifests/release/base/dojo-world.toml
  • examples/spawn-and-move/manifests/dev/base/dojo-world.toml
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the propagation of new hash values in the codebase.

# Test: Search for occurrences of the old hash values. Expect: No occurrences.
rg --type toml '0x32fb65ebfe6d91bb4b7ba0640650722c43ca0c917f5fc0f649ee2ecf720cde'

# Test: Search for occurrences of the new hash values. Expect: Occurrences in relevant files.
rg --type toml '0x4c90da98d2bad157dec1d7b9b9c8c5861828a7ec1b323425d84fa6c3071303f'

Length of output: 1398

crates/sozo/ops/src/tests/model.rs (1)

40-41: Ohayo, sensei! Verify the alignment of the updated expected value.

The expected value for the test_model_ops function has been updated. Ensure that this change aligns with the updated logic or data being tested.

Verification successful

Ohayo, sensei! Verify the alignment of the updated expected value.

The expected value for the test_model_ops function has been updated. Ensure that this change aligns with the updated logic or data being tested.


Ohayo, sensei! Verify the alignment of the updated expected value.

The expected value for the test_model_ops function has been updated. Ensure that this change aligns with the updated logic or data being tested.


Ohayo, sensei! Verify the alignment of the updated expected value.

The expected value for the test_model_ops function has been updated. Ensure that this change aligns with the updated logic or data being tested.


Ohayo, sensei! Verify the alignment of the updated expected value.

The expected value for the test_model_ops function has been updated. Ensure that this change aligns with the updated logic or data being tested.


Ohayo, sensei! The updated expected value aligns with the new logic.

The expected value for the test_model_ops function has been updated to 0x2b14bbb22e6a21cc949e06a187436c96aeab0e0290b3a8d91fb357ed2e6d973. This change aligns with the logic in the model::model_contract_address function, which retrieves the contract address of a model using the WorldContractReader.

  • crates/sozo/ops/src/tests/model.rs: The test has been updated to reflect the new expected contract address.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the alignment of the updated expected value with the new logic or data.

# Test: Search for occurrences of the old expected value. Expect: No occurrences.
rg --type rust '0x6dd9f573496d64b1c026c68cbfe31a7724c7bcd4a142099666e8e22b82e0688'

# Test: Search for occurrences of the new expected value. Expect: Occurrences in relevant files.
rg --type rust '0x2b14bbb22e6a21cc949e06a187436c96aeab0e0290b3a8d91fb357ed2e6d973'

Length of output: 292


Script:

#!/bin/bash
# Description: Verify the alignment of the updated expected value with the new logic or data.

# Test: Retrieve the surrounding context of the new expected value in the `test_model_ops` function.
rg --type rust -A 10 'Felt::from_hex("0x2b14bbb22e6a21cc949e06a187436c96aeab0e0290b3a8d91fb357ed2e6d973")' crates/sozo/ops/src/tests/model.rs

Length of output: 143


Script:

#!/bin/bash
# Description: Verify the alignment of the updated expected value with the new logic or data.

# Test: Retrieve a wider surrounding context of the new expected value in the `test_model_ops` function.
rg --type rust -A 20 'Felt::from_hex("0x2b14bbb22e6a21cc949e06a187436c96aeab0e0290b3a8d91fb357ed2e6d973")' crates/sozo/ops/src/tests/model.rs

Length of output: 143


Script:

#!/bin/bash
# Description: Verify the alignment of the updated expected value with the new logic or data.

# Verify the file path
if [ -f crates/sozo/ops/src/tests/model.rs ]; then
  echo "File exists: crates/sozo/ops/src/tests/model.rs"
else
  echo "File does not exist: crates/sozo/ops/src/tests/model.rs"
fi

# Test: Retrieve the function definition and surrounding context of `test_model_ops`.
rg --type rust -A 30 'fn test_model_ops' crates/sozo/ops/src/tests/model.rs

Length of output: 1159


Script:

#!/bin/bash
# Description: Verify the alignment of the updated expected value with the new logic or data.

# Test: Retrieve the implementation of the `model::model_contract_address` function.
rg --type rust 'fn model_contract_address' -A 20

Length of output: 1238

examples/spawn-and-move/manifests/dev/deployment/manifest.json (2)

405-408: Ohayo, sensei! Verify the integration of the new address parameter.

The addition of the address parameter to the is_owner function looks good. Ensure that all invocations of this function in the codebase are updated to pass the new parameter.


441-444: Ohayo, sensei! Verify the integration of the new address parameter.

The addition of the address parameter to the revoke_owner function looks good. Ensure that all invocations of this function in the codebase are updated to pass the new parameter.

Verification successful

Ohayo, sensei! It seems the revoke_owner function has inconsistent input parameters across different files.

To ensure all invocations of this function are updated to pass the new address parameter, let's gather more information about the context of these different instances.


Ohayo, sensei! The revoke_owner function has been updated to include both resource and address parameters across the codebase.

The function signature has been updated correctly, and all invocations are consistent with the new signature.

  • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json
  • examples/spawn-and-move/manifests/dev/deployment/manifest.json
  • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json
  • crates/katana/storage/db/benches/artifacts/dojo_world_240.json
  • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json
  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json
  • examples/game-lib/armory/manifests/dev/base/abis/dojo-world.json
  • examples/game-lib/manifests/dev/base/abis/dojo-world.json
  • bin/sozo/tests/test_data/manifest.json
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `revoke_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'revoke_owner'

Length of output: 7172


Script:

#!/bin/bash
# Description: Gather more context around the `revoke_owner` function usage to ensure all invocations are updated correctly.

# Search for the `revoke_owner` function definition and its usages in the codebase.
rg --type json -A 10 '"name": "revoke_owner"' > revoke_owner_context.json

# Display the gathered context for review.
cat revoke_owner_context.json

Length of output: 9128


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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 as 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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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, codebase verification and nitpick comments (3)
examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json (1)

420-423: Ohayo, sensei! There are inconsistencies in the grant_owner function parameters.

The parameter address has been moved to the second position, but not all instances reflect this change. Here are the instances that need attention:

  • Update to new signature:

    • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json
    • examples/spawn-and-move/manifests/dev/deployment/manifest.json
    • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json
    • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json
    • examples/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json
  • Already updated:

    • examples/game-lib/manifests/dev/base/abis/dojo-world.json
    • bin/sozo/tests/test_data/manifest.json
    • crates/katana/storage/db/benches/artifacts/dojo_world_240.json
    • examples/game-lib/armory/manifests/dev/base/abis/dojo-world.json

Please ensure all instances of grant_owner match the new signature.

Analysis chain

Ohayo, sensei! Verify the function usage for grant_owner.

The parameter address has been moved to the second position. Ensure all calls to grant_owner match the new signature.


Ohayo, sensei! Verify the function usage for grant_owner.

The parameter address has been moved to the second position. Ensure all calls to grant_owner match the new signature.


Ohayo, sensei! Verify the function usage for grant_owner.

The parameter address has been moved to the second position. Ensure all calls to grant_owner match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'grant_owner'

Length of output: 7228


Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'grant_owner'

Length of output: 7228


Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'grant_owner'

Length of output: 7228

examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json (1)

400-403: Ohayo, sensei! Verify the function usage for is_owner.

The parameter address has been moved to the second position. Ensure all calls to is_owner match the new signature.

  • examples/spawn-and-move/manifests/dev/deployment/manifest.json: The is_owner function has resource as the first parameter, which is correct.
  • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json: The is_owner function has resource as the first parameter, which is correct.
  • examples/game-lib/manifests/dev/base/abis/dojo-world.json: The is_owner function has address as the first parameter, which is incorrect.
  • examples/game-lib/armory/manifests/dev/base/abis/dojo-world.json: The is_owner function has address as the first parameter, which is incorrect.
  • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json: The is_owner function has resource as the first parameter, which is correct.
  • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json: The is_owner function has resource as the first parameter, which is correct.
  • crates/katana/storage/db/benches/artifacts/dojo_world_240.json: The is_owner function has address as the first parameter, which is incorrect.
  • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json: The is_owner function has resource as the first parameter, which is correct.
  • bin/sozo/tests/test_data/manifest.json: The is_owner function has address as the first parameter, which is incorrect.

Locations with incorrect parameter order:

  • examples/game-lib/manifests/dev/base/abis/dojo-world.json
  • examples/game-lib/armory/manifests/dev/base/abis/dojo-world.json
  • crates/katana/storage/db/benches/artifacts/dojo_world_240.json
  • bin/sozo/tests/test_data/manifest.json

Please update these instances to match the new function signature.

Analysis chain

Ohayo, sensei! Verify the function usage for is_owner.

The parameter address has been moved to the second position. Ensure all calls to is_owner match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `is_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'is_owner'

Length of output: 7083

crates/dojo-core/src/tests/world.cairo (1)

1549-1549: Ohayo, sensei!

The grant_owner function calls in the crates/dojo-core/src/tests/world.cairo file are inconsistent with the new parameter order. Please update the following instances to match the new signature:

  • world.grant_owner(bytearray_hash(@"dojo"), bob);
  • world.grant_owner(0, alice);
  • world.grant_owner(foo_selector, bob);
  • world.grant_owner(0, alice);
  • world.grant_owner(bytearray_hash(@"dojo"), account);

Ensure all calls follow the new parameter order for clarity and consistency.

Analysis chain

Ohayo, sensei! The parameter order change in grant_owner looks good!

The new parameter order enhances clarity and consistency. Ensure that the test logic remains correct.


Ohayo, sensei!

The previous script encountered an issue because cairo is not a recognized file type by rg. Let's re-run the search without specifying the file type to ensure all grant_owner function calls are using the new parameter order.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` in tests match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type cairo -A 5 $'grant_owner'

Length of output: 64


Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` in .cairo files match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg 'grant_owner' --glob '*.cairo' -A 5

Length of output: 4649

@remybar remybar force-pushed the fix_permission_api branch from f2067e2 to cb644a1 Compare July 29, 2024 14:40
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, codebase verification and nitpick comments (1)
examples/spawn-and-move/manifests/dev/deployment/manifest.json (1)

425-428: Ohayo, sensei!

The addition of the address parameter to the grant_owner function is not consistently reflected across the codebase. Several instances of the function still use the old signature. Please update all invocations of the grant_owner function to include the new address parameter.

  • Examples of files needing updates:
    • examples/spawn-and-move/manifests/dev/deployment/manifest.json
    • examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json
    • examples/spawn-and-move/manifests/release/base/abis/dojo-world.json
    • examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json
    • crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json
Analysis chain

Ohayo, sensei! Verify the integration of the new address parameter.

The addition of the address parameter to the grant_owner function looks good. Ensure that all invocations of this function in the codebase are updated to pass the new parameter.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `grant_owner` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type json -A 5 $'grant_owner'

Length of output: 7228

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.86%. Comparing base (ea1b6a8) to head (cb644a1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2224      +/-   ##
==========================================
- Coverage   69.87%   69.86%   -0.01%     
==========================================
  Files         341      341              
  Lines       44719    44719              
==========================================
- Hits        31246    31244       -2     
- Misses      13473    13475       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Thanks for the quick refactoring on this one @remybar. 🙏

@glihm glihm merged commit 8bcdc3b into dojoengine:main Jul 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants