dojo icon indicating copy to clipboard operation
dojo copied to clipboard

Published 20 hours ago • verified publisher icon dojoengine

Event fields as key

Open remybar opened this issue 1 year ago • 6 comments
trafficstars

Closes #2228.

Depends on https://github.com/dojoengine/dojo-core/pull/12.

TODO: dojo-core should be removed as it has been moved to dojo-core repo.

Tests

  • [X] Yes
  • [ ] No, because they aren't needed
  • [ ] No, because I need help

Added to documentation?

  • [ ] README.md
  • [ ] Dojo Book
  • [X] No documentation needed

Checklist

  • [X] I've formatted my code (scripts/prettier.sh, scripts/rust_fmt.sh, scripts/cairo_fmt.sh)
  • [X] I've linted my code (scripts/clippy.sh, scripts/docs.sh)
  • [X] I've commented my code
  • [X] I've requested a review after addressing the comments

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced event handling in the smart contract framework by marking key fields for several event structures, improving clarity and efficiency in event processing.
    • Introduced a unified Event enum for streamlined event handling in tests, enhancing type safety and modularity.
    • Updated data structure in the event processing functions for improved performance and clarity.

  • Bug Fixes

    • Improved testing accuracy and reliability for various functionalities by ensuring proper ownership handling and enhancing event log assertions.

  • Chores

    • Updated configuration files with new addresses and class hashes, reflecting changes in the deployment environment.

remybar avatar Aug 09 '24 14:08 remybar

Walkthrough

Ohayo, sensei! The recent updates to the dojo-core project enhance the world module by refining event handling, ownership management, and data structure categorization. Key changes include using namespace.clone() in the register_namespace function for improved ownership handling, implementing #[key] attributes in event structures for clearer identification, and transitioning to a unified event handling mechanism utilizing an Event enum for better scalability.

Changes

Files Change Summary
crates/dojo-core/src/tests/world.cairo, crates/dojo-core/src/tests/world/resources.cairo Updated event handling logic to use a generalized Event enum, simplifying assertions and enhancing maintainability across various test functions.
crates/dojo-core/src/world/world_contract.cairo Added #[key] attributes to event struct fields, improving key identification for StarkNet events, which optimizes how these are indexed and accessed.
crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json, examples/spawn-and-move/manifests/dev/base/dojo-world.toml, examples/spawn-and-move/manifests/dev/deployment/manifest.json Changed multiple fields' kind from "data" to "key", emphasizing their roles as identifiers.
examples/spawn-and-move/manifests/dev/deployment/manifest.json, examples/spawn-and-move/manifests/dev/deployment/manifest.toml Updated class_hash and original_class_hash values, reflecting changes in class identifiers for improved versioning and data integrity.
examples/spawn-and-move/dojo_dev.toml Modified world_address configuration to new hexadecimal addresses, indicating updates in network or environment settings.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Contract
    participant EventLog

    User->>Contract: Call register_namespace(namespace)
    Contract->>Contract: Process namespace
    Contract->>EventLog: Log event with namespace
    EventLog-->>Contract: Event logged
    Contract-->>User: Confirm registration

Assessment against linked issues

Objective Addressed Explanation
Make fields as keys in events where possible (#[2228]) ✅
Improve retrieval of events by reclassifying fields (#[2228]) ✅
Recent review details

Configuration used: .coderabbit.yaml Review profile: CHILL

Commits

Files that changed from the base of the PR and between e5c4871cf6e6a133031f8d1a8ea866d4f5f4e71d and 8ee0ab7fe79d7f951c79250c9996973c0ed0fa22.

Files ignored due to path filters (2)
  • spawn-and-move-db.tar.gz is excluded by !**/*.gz
  • types-test-db.tar.gz is excluded by !**/*.gz
Files selected for processing (12)
  • crates/dojo-core/src/world/world_contract.cairo (4 hunks)
  • crates/dojo-lang/src/event.rs (3 hunks)
  • crates/dojo-types/src/event.rs (1 hunks)
  • crates/dojo-world/src/manifest/mod.rs (3 hunks)
  • crates/torii/core/src/processors/event_message.rs (4 hunks)
  • crates/torii/core/src/processors/metadata_update.rs (2 hunks)
  • examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
  • examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.toml (1 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.json (18 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml (5 hunks)
  • examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
  • examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.toml (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • crates/dojo-core/src/world/world_contract.cairo
  • crates/dojo-world/src/manifest/mod.rs
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml
Additional comments not posted (20)
examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.toml (2)

2-3: LGTM! These changes look solid, sensei.

Assuming the verification checks out, the updates to class_hash and original_class_hash are approved. Nice work on keeping our contract metadata up-to-date!

2-3: Ohayo, sensei! Let's verify these hash updates.

The class_hash and original_class_hash have been updated. We should ensure these new hashes are correct and consistent with the latest contract implementation.

Run this script to check the hash consistency:

Verification successful

Ohayo, sensei! Hash update verified across the codebase.

The new hash 0x25395f6651e97bc6ada3d5de989a1e73cb35f75621097d4e19017c70a285ed6 is consistently updated across multiple TOML files, and the old hash is no longer present. This indicates a comprehensive update. If further verification of the hash's correctness is needed, please consider manual checks.

  • New hash found in:
    • examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.toml
    • examples/spawn-and-move/manifests/dev/deployment/manifest.toml
    • examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.toml
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new hash across the codebase

# Test: Search for occurrences of the new hash
rg --type toml "0x25395f6651e97bc6ada3d5de989a1e73cb35f75621097d4e19017c70a285ed6"

# Test: Check if the old hash is still present anywhere
rg --type toml "0x40e824b8814bafef18cce2cf68f5765e9c9a1c86f55a8491b0c2a4faebdcc87"

Length of output: 1164

examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.toml (2)

2-3: LGTM! The hash updates look good, sensei.

Assuming the hash updates were intentional and properly tested, the changes look good to me.

2-3: Ohayo, sensei! Verify the intention behind the hash updates.

I noticed that both class_hash and original_class_hash have been updated to the same new value. This change might have significant implications for the contract's behavior and interactions.

Could you confirm if this update was intentional? Also, have you thoroughly tested the contract with these new hash values to ensure everything works as expected?

examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml (1)

2-3: Ohayo, sensei! The hash update looks solid!

The class_hash and original_class_hash have been updated to the same new value. This change is likely intentional and represents a new version of the contract.

Let's make sure this new hash is consistent across the project, sensei:

Verification successful

Ohayo, sensei! The new hash is consistently used across the project, which is a good sign. The update seems intentional and well-applied. If you know the old hash, it might be worth doing a quick manual check to ensure it's fully replaced. Keep up the great work!

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new hash is used consistently across the project.

# Test: Search for the new hash in all files. Expect: Consistent usage of the new hash.
rg --type toml "0x270c8ffb9f92d3711fd67d9842f7519c663ddb17f15ddafd08bfc2aeb7d86d9"

# Test: Search for any occurrences of the old hash (if known). Expect: No occurrences of the old hash.
# Note: Replace <old_hash> with the actual old hash value if known.
# rg --type toml "<old_hash>"

Length of output: 1085

crates/dojo-types/src/event.rs (1)

3-3: Ohayo, sensei! New constant for system event identification.

The addition of SYSTEM_EVENT_SELECTOR looks good. It'll help distinguish system events from others.

Let's check if this constant is used elsewhere in the codebase:

Verification successful

Ohayo, sensei! Constant SYSTEM_EVENT_SELECTOR is well-integrated.

The SYSTEM_EVENT_SELECTOR constant is actively used in the codebase, particularly in event processing logic. Its presence in comments and macros highlights its importance in distinguishing system events.

  • Files using the constant:
    • crates/torii/core/src/processors/event_message.rs
    • crates/dojo-lang/src/event.rs
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usage of SYSTEM_EVENT_SELECTOR
rg --type rust "SYSTEM_EVENT_SELECTOR"

Length of output: 825

crates/torii/core/src/processors/event_message.rs (1)

3-3: Ohayo, sensei! Nice import addition!

The import of SYSTEM_EVENT_SELECTOR aligns well with the changes in the event_key function. Good job keeping the imports up-to-date!

crates/torii/core/src/processors/metadata_update.rs (1)

60-61: Ohayo again, sensei! The event processing looks updated.

The changes to extracting the resource and URI data align with the new event structure. Good job adapting to the new format!

To ensure system-wide consistency, let's verify that other components interacting with this event data have been updated accordingly. Run this script to check for potential inconsistencies:

This will help identify any other places in the codebase that might need similar updates to maintain consistency with the new event structure.

crates/dojo-lang/src/event.rs (3)

12-12: Ohayo, sensei! New import added for SYSTEM_EVENT_SELECTOR.

The addition of this import aligns with the PR objectives by introducing the system event selector concept.

79-81: Updated comments clarify the purpose of SYSTEM_EVENT_SELECTOR, sensei!

The comments now explain that the first key of a dojo event will be the SYSTEM_EVENT_SELECTOR, which helps distinguish dojo events from starknet events. This aligns with the PR objectives of enhancing event retrieval granularity.

90-91: SYSTEM_EVENT_SELECTOR now appended to keys array, sensei!

This change implements the core functionality described in the updated comments. It ensures that dojo events are uniquely identifiable, which aligns with the PR objectives of treating event fields as keys and improving event retrieval granularity.

Let's verify if this change is consistently applied across the codebase:

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

4-5: Ohayo, sensei! New class hash detected for the world contract.

The class_hash and original_class_hash for the world contract have been updated. This suggests a new version of the world contract has been deployed.

Let's make sure this change is intentional and properly propagated:

Verification successful

Ohayo, sensei! The class hash update is consistent across the codebase.

The new class_hash and original_class_hash are consistently used in multiple manifest files, including both development and release configurations, as well as in test data. This suggests that the update was intentional and has been properly propagated throughout the system.

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

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the new class hash is used consistently
rg -i "0x479648c316ee5f9b6999362aa9e72c5cb881ee12e0017559a29025de04a55b6"

Length of output: 1616

769-774: Ohayo! ContractDeployed event got a makeover, sensei!

Multiple fields in the ContractDeployed event have been changed from "data" to "key", and a new salt field has been added. This could significantly impact how contract deployments are tracked and queried.

Let's ensure this change is reflected in the contract deployment logic:

Also applies to: 779-785

Verification successful

Ohayo, sensei! ContractDeployed event changes are all good!

The changes to the ContractDeployed event, including the addition of the salt field and the marking of fields as keys, are correctly reflected in the contract deployment logic and verified by test cases. No issues found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ContractDeployed event usage
rg "ContractDeployed" -A 10 -g "*.cairo"

Length of output: 6912

801-801: Key change for ContractUpgraded event, sensei!

The class_hash field in the ContractUpgraded event has been changed from "data" to "key". This could affect how contract upgrades are indexed and queried.

Let's check if this change is consistently applied:

Verification successful

Ohayo, sensei! The class_hash field change is consistently applied.

The class_hash field in the ContractUpgraded event is marked as a key in the source code, specifically in crates/dojo-core/src/world/world_contract.cairo. This confirms the change is applied where it impacts functionality. The JSON files are representations and do not need to reflect the #[key] attribute. No further inconsistencies found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ContractUpgraded event usage
rg "ContractUpgraded" -A 5 -g "*.cairo"

Length of output: 3022

Script:

#!/bin/bash
# Search for other definitions or usages of ContractUpgraded in the codebase
rg "ContractUpgraded" -A 5

Length of output: 23734

1906-1908: More contract upgrades detected, sensei!

Two more contracts have had their address, class_hash, and original_class_hash updated. This appears to be a large-scale upgrade of multiple contracts in the system.

We should ensure all these upgrades are coordinated and properly tested:

Also applies to: 2127-2129

847-847: Ohayo, sensei! Multiple events got the key treatment!

The resource field in MetadataUpdate, namespace in NamespaceRegistered, and name and namespace in ModelRegistered events have been changed from "data" to "key". This could significantly impact event indexing and querying across the system.

Let's ensure these changes are consistently applied and handled in the event processing logic:

Also applies to: 864-864, 881-886

Verification successful

Ohayo, sensei! The key changes are consistently applied and handled. The MetadataUpdate, NamespaceRegistered, and ModelRegistered events have been updated with the #[key] attribute, and their usage in the event processing logic confirms that these changes are correctly integrated. The processors validate the keys, ensuring proper handling of these events.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for affected events
rg -e "MetadataUpdate" -e "NamespaceRegistered" -e "ModelRegistered" -A 10 -g "*.cairo"

Length of output: 13609

Script:

#!/bin/bash
# Search for usage of the affected events in the codebase to verify consistent handling
rg -e "MetadataUpdate" -e "NamespaceRegistered" -e "ModelRegistered" -A 10

Length of output: 90466

757-757: Key change for WorldSpawned event, sensei!

The creator field in the WorldSpawned event has been changed from "data" to "key". This could affect event indexing and querying.

Let's check if this change is consistent across the codebase:

Verification successful

Ohayo, sensei! The key change is consistent in the implementation. The creator field in the WorldSpawned event is marked as a key in the Cairo source file, confirming the review comment. The broader impact on indexing and querying would depend on how these files are used with the Cairo implementation. No inconsistencies were found in the references across the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for WorldSpawned event usage
rg "WorldSpawned" -A 5 -g "*.cairo"

Length of output: 1497

Script:

#!/bin/bash
# Search for all occurrences of WorldSpawned event across the codebase
rg "WorldSpawned" -A 5

Length of output: 21329

1232-1233: New world address and transaction hash, sensei!

The address and transaction_hash for the world contract have been updated. This suggests a new deployment of the world contract.

Let's make sure this change is properly reflected in the deployment scripts and any dependent contracts:

945-950: Massive key changes across multiple events, sensei!

Several fields in StoreSetRecord, StoreUpdateRecord, StoreUpdateMember, StoreDelRecord, WriterUpdated, and OwnerUpdated events have been changed from "data" to "key". This is a significant change that could affect how data is indexed and queried throughout the system.

We need to ensure these changes are properly handled in all event listeners and data processors:

Also applies to: 967-972, 989-999, 1016-1021, 1033-1038, 1055-1060

Verification successful

Ohayo, sensei! The key changes are handled properly.

The changes from "data" to "key" in the events StoreSetRecord, StoreUpdateRecord, StoreUpdateMember, StoreDelRecord, WriterUpdated, and OwnerUpdated are managed by specific event processors in the codebase. These processors validate and process the events, ensuring that the system handles them appropriately. No further action is needed.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for affected events
rg -e "StoreSetRecord" -e "StoreUpdateRecord" -e "StoreUpdateMember" -e "StoreDelRecord" -e "WriterUpdated" -e "OwnerUpdated" -A 10 -g "*.cairo"

Length of output: 13452

Script:

#!/bin/bash
# Search for usage of the affected events in the codebase to check for event listeners or data processors
rg -e "StoreSetRecord" -e "StoreUpdateRecord" -e "StoreUpdateMember" -e "StoreDelRecord" -e "WriterUpdated" -e "OwnerUpdated" -A 10

Length of output: 152882

1253-1255: New contract deployment detected, sensei!

The address, class_hash, and original_class_hash for a contract have been updated. This indicates a new version of the contract has been deployed.

Let's ensure this change is intentional and properly handled in the deployment process:

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 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.

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.

[!TIP]

Early access features: enabled

We are currently testing the following features in early access:

  • Anthropic claude-3-5-sonnet for code reviews: Anthropic claims that the new Claude model has stronger code understanding and code generation capabilities than their previous models. Note: Our default code review model was also updated late last week. Please compare the quality of the reviews between the two models by toggling the early access feature.

Note:

  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.
  • Please join our Discord Community to provide feedback and report issues on the discussion post.

coderabbitai[bot] avatar Aug 09 '24 14:08 coderabbitai[bot]

Thanks for that! The Torii processors must also be updated to ensure they are parsing the data from the keys array, and not data as currently. 👍

Ok !

I've just reported an issue to Starkware about the pop_log function which does not work where an event has a ByteArray field key: https://github.com/starkware-libs/cairo/issues/6186.

remybar avatar Aug 11 '24 12:08 remybar

Codecov Report

Attention: Patch coverage is 72.09302% with 48 lines in your changes missing coverage. Please review.

Project coverage is 67.86%. Comparing base (1d95782) to head (8ee0ab7). Report is 155 commits behind head on main.

Files with missing lines Patch % Lines
...s/torii/core/src/processors/store_update_member.rs 0.00% 16 Missing :warning:
crates/torii/core/src/processors/event_message.rs 0.00% 15 Missing :warning:
...s/torii/core/src/processors/store_update_record.rs 0.00% 12 Missing :warning:
...rates/torii/core/src/processors/metadata_update.rs 0.00% 4 Missing :warning:
...ates/torii/core/src/processors/store_set_record.rs 94.11% 1 Missing :warning:
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2280      +/-   ##
==========================================
- Coverage   67.86%   67.86%   -0.01%     
==========================================
  Files         359      359              
  Lines       46959    47003      +44     
==========================================
+ Hits        31869    31898      +29     
- Misses      15090    15105      +15

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 29 '24 23:08 codecov[bot]

was just thinking about this yesterday! since we already have ability to add custom key in the event the current behaviour of trying it for every event is slow in performance.

i think 2 and 3 makes sense to me, preferably 3 if possible so we have to just first key for all types of event.

itzlambda avatar Aug 30 '24 05:08 itzlambda

the event messages are not working anymore.

@Larkooo @lambda-0x as we have the event messages processors being a catch all, it will run on every message as we only check the length of the keys.

But as now, most of events have keys, and they contains ByteArray, the length is not longer a good filter.

We end up with this processor being run on all StoreSetRecord etc...

Some solutions we may have:

  1. Blacklist all the world events, which ensure we only process emit! generated events. But this is too bad for Torii performances.
  2. When using emit!, we already happen the system's address, we may also happen a selector!("SystemEvent") before the address. Like so, the event message processor can directly check the -2 position in the array.
  3. We could wrap the system event into a world's event to make the first key (the event selector) the actual SystemEvent.
  4. Something else?

Yeah I think blacklisting is not a sustainable solution, so 2 and 3 make the most sense to me. I think it just depends on if its going to be a breaking change or not. I remember when I was working on the event messages we had to follow the same event structure as a normal event for it to also work for the emit!. We need to take that into consideration and also what happens if we index older event messages (old structure) and newer event messages. Seems like we cannot have both

Larkooo avatar Aug 30 '24 15:08 Larkooo

@Larkooo should be ok, will let you make your checks and let's move forward if all good. In a second phase, we will make the adjustment on the event/model separation.

glihm avatar Sep 03 '24 02:09 glihm

Done in rc.0.

glihm avatar Oct 31 '24 19:10 glihm