refactor(torii): entity update without being set before #3072
Conversation
|
Ohayo sensei, here’s the detailed summary: WalkthroughThe changes refine the handling of entity updates across several modules. In the gRPC service, the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/mod.rs (1)
454-460: Ohayo! Warning message could be clearer about what's actually happening.The warning message says the entity update is "ignored", but the code still proceeds with publishing the entity through SimpleBroker and adding it to the publish_queue. This mismatch between the warning message and actual behavior could cause confusion.
if entity_updated.keys.is_empty() { - warn!(target: LOG_TARGET, "Entity update ignored. An entity must be set with StoreSetRecord before it can be updated with StoreUpdateRecord or StoreUpdateMember."); + warn!(target: LOG_TARGET, "Entity update has empty keys. An entity must be set with StoreSetRecord before it can be updated with StoreUpdateRecord or StoreUpdateMember."); }crates/torii/grpc/src/server/subscriptions/entity.rs (1)
117-129: Wonderful improvement in handling empty keys, sensei!The conditional check for empty keys improves robustness by preventing unnecessary parsing attempts. This addresses the case where an entity is updated without being set before.
There's a formatting issue that's causing the CI pipeline to fail. Please fix the indentation of your comments:
- // keys is empty when an entity is updated with StoreUpdateRecord or Member but the entity has never been set before. - // In that case, we dont know the keys + // Keys is empty when an entity is updated with StoreUpdateRecord or Member but + // the entity has never been set before. In that case, we don't know the keys.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/grpc/src/server/subscriptions/entity.rs(1 hunks)crates/torii/sqlite/src/executor/mod.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/subscriptions/entity.rs
[error] 114-114: Rust formatting check failed. Please run 'rustfmt' to format the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3072 +/- ##
==========================================
- Coverage 57.38% 57.36% -0.03%
==========================================
Files 440 440
Lines 59963 59942 -21
==========================================
- Hits 34411 34386 -25
- Misses 25552 25556 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/subscriptions/mod.rs (1)
41-44: Early return for empty clause_model is a solid improvement!Ohayo sensei! I like this addition of an early return when a clause_model is empty. It properly handles the scenario where an empty model clause should be considered a match, consistent with the "match all" philosophy used elsewhere in the code (like with empty namespaces or "*" wildcards).
Adding a brief comment would help future maintainers understand the reasoning:
if clause_model.is_empty() { + // Empty model clause is treated as a wildcard that matches everything return true; }
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Chores
ERC1155Tokencontract'sclass_hashandaddressto reflect recent changes in deployment or configuration.