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

feat(torii-client): token subscription & update subscription via id #3006

Merged
merged 11 commits into from
Feb 12, 2025

Conversation

Larkooo
Copy link
Collaborator

@Larkooo Larkooo commented Feb 11, 2025

Summary by CodeRabbit

  • New Features
    • Tokens now include an additional identifier field, enhancing token management and clarity for users.
    • A new asynchronous method allows clients to subscribe to real-time updates for token changes.
    • Users can update existing token subscriptions with a new method.
    • A new RPC method has been added for updating token subscriptions.
    • New structures for optimistic tokens and balances have been introduced to improve data handling.
  • Improvements
    • Enhanced response structure now includes subscription IDs for token updates.
    • Transitioned from balance to token subscription management, improving overall functionality.

Copy link

coderabbitai bot commented Feb 11, 2025

Walkthrough

Ohayo! The changes introduce a new field id to the Token structure in the gRPC types module, with the TryFrom implementation updated to initialize this field from value.token_id. Additionally, a new asynchronous method on_token_updated is added to the Client struct, enabling clients to subscribe to token updates. The WorldClient struct gains a subscribe_tokens method for subscribing to token updates, enhancing the overall functionality related to token management and updates.

Changes

File Change Summary
crates/torii/grpc/src/types/mod.rs Added new field id: String to the Token struct; updated the TryFrom implementation to assign id from value.token_id.
crates/torii/client/src/client/mod.rs Added asynchronous method on_token_updated for subscribing to token updates.
crates/torii/grpc/src/client.rs Added asynchronous methods subscribe_tokens and update_tokens_subscription; modified subscribe_token_balances; added TokenMappedStream type and TokenUpdateStreaming struct.
crates/torii/grpc/proto/world.proto Introduced UpdateTokensSubscription RPC method; added UpdateTokenSubscriptionRequest message; modified SubscribeTokensResponse to include subscription_id.
crates/torii/grpc/src/server/mod.rs Added update_tokens_subscription method; modified update_token_balances_subscription method for better handling.
crates/torii/grpc/src/server/subscriptions/token.rs Renamed variables and methods to reflect token subscription handling; updated logging and response structures.
crates/torii/grpc/src/server/subscriptions/token_balance.rs Transitioned from TokenBalance to OptimisticTokenBalance; updated related methods and structures.
crates/torii/sqlite/src/executor/erc.rs Changed import from TokenBalance to OptimisticTokenBalance; updated balance publishing logic.
crates/torii/sqlite/src/executor/mod.rs Added TokenBalanceUpdated enum variant; modified entity update handling with std::mem::transmute.
crates/torii/sqlite/src/types.rs Introduced new structs OptimisticToken and OptimisticTokenBalance for optimistic representation of tokens.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ProtoToken as "proto::types::Token"
    participant Token as "Token"
    participant Felt

    Client->>Token: TryFrom(proto::types::Token)
    Token->>Felt: from_str(&value.contract_address)
    Felt-->>Token: Parsed Felt value
    Token-->>Client: Returns Token { id, contract_address, ... }
Loading

Possibly related PRs

Suggested reviewers

  • glihm (Sensei)

🪧 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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@Larkooo Larkooo changed the title chore(torii-client): expose token id feat(torii-client): token subscription Feb 11, 2025
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 16.93548% with 103 lines in your changes missing coverage. Please review.

Project coverage is 56.38%. Comparing base (6c98096) to head (657510e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/torii/grpc/src/client.rs 0.00% 61 Missing ⚠️
...rates/torii/grpc/src/server/subscriptions/token.rs 45.00% 11 Missing ⚠️
crates/torii/grpc/src/server/mod.rs 0.00% 10 Missing ⚠️
crates/torii/client/src/client/mod.rs 0.00% 8 Missing ⚠️
crates/torii/sqlite/src/executor/erc.rs 0.00% 7 Missing ⚠️
crates/torii/grpc/src/types/mod.rs 0.00% 3 Missing ⚠️
crates/torii/sqlite/src/executor/mod.rs 83.33% 2 Missing ⚠️
...rii/grpc/src/server/subscriptions/token_balance.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
- Coverage   56.48%   56.38%   -0.10%     
==========================================
  Files         434      434              
  Lines       57999    58075      +76     
==========================================
- Hits        32759    32744      -15     
- Misses      25240    25331      +91     

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

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: 1

🧹 Nitpick comments (1)
crates/torii/client/src/client/mod.rs (1)

117-122: Enhance documentation for token subscription.

Ohayo! The implementation looks clean, but let's make the documentation more helpful by describing the return type and providing an example usage.

-    /// A direct stream to grpc subscribe tokens
+    /// Subscribe to token updates for the specified contract addresses.
+    ///
+    /// Returns a stream of token updates that can be used to monitor changes in real-time.
+    ///
+    /// # Example
+    /// ```rust
+    /// let contract_addresses = vec![contract_address];
+    /// let mut stream = client.on_token_updated(contract_addresses).await?;
+    /// while let Some(token) = stream.try_next().await? {
+    ///     println!("Token updated: {:?}", token);
+    /// }
+    /// ```
     pub async fn on_token_updated(&self, contract_addresses: Vec<Felt>) -> Result<TokenUpdateStreaming, Error> {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86f5a73 and ea87879.

📒 Files selected for processing (2)
  • crates/torii/client/src/client/mod.rs (1 hunks)
  • crates/torii/grpc/src/client.rs (5 hunks)
🔇 Additional comments (2)
crates/torii/grpc/src/client.rs (2)

387-403: LGTM! Clean implementation of TokenUpdateStreaming.

The implementation follows the established pattern for streaming types and properly implements the Stream trait.


354-357: LGTM! Improved balance handling.

The pattern matching for balance handling is now more explicit and consistent with the rest of the codebase.

@Larkooo Larkooo changed the title feat(torii-client): token subscription feat(torii-client): token subscription & update subscription via id Feb 11, 2025
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

♻️ Duplicate comments (1)
crates/torii/grpc/src/client.rs (1)

152-153: ⚠️ Potential issue

Improve error handling in token subscription.

Ohayo sensei! The implementation has a potential panic point in the try_into() call. Let's handle the error gracefully.

-                    Some(token) => token.try_into().expect("must able to serialize"),
+                    Some(token) => token.try_into().map_err(Error::Schema)?,
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)

57-57: Split long import statement for better readability.

Ohayo! The import statement is too long. Consider splitting it into multiple lines:

-use crate::proto::world::{RetrieveControllersRequest, RetrieveControllersResponse, RetrieveEntitiesStreamingResponse, RetrieveEventMessagesRequest, RetrieveTokenBalancesRequest, RetrieveTokenBalancesResponse, RetrieveTokensRequest, RetrieveTokensResponse, SubscribeEntitiesRequest, SubscribeEntityResponse, SubscribeEventMessagesRequest, SubscribeEventsResponse, SubscribeIndexerRequest, SubscribeIndexerResponse, SubscribeTokenBalancesResponse, SubscribeTokensResponse, UpdateEventMessagesSubscriptionRequest, UpdateTokenBalancesSubscriptionRequest, UpdateTokenSubscriptionRequest, WorldMetadataRequest, WorldMetadataResponse};
+use crate::proto::world::{
+    RetrieveControllersRequest, RetrieveControllersResponse,
+    RetrieveEntitiesStreamingResponse, RetrieveEventMessagesRequest,
+    RetrieveTokenBalancesRequest, RetrieveTokenBalancesResponse,
+    RetrieveTokensRequest, RetrieveTokensResponse,
+    SubscribeEntitiesRequest, SubscribeEntityResponse,
+    SubscribeEventMessagesRequest, SubscribeEventsResponse,
+    SubscribeIndexerRequest, SubscribeIndexerResponse,
+    SubscribeTokenBalancesResponse, SubscribeTokensResponse,
+    UpdateEventMessagesSubscriptionRequest, UpdateTokenBalancesSubscriptionRequest,
+    UpdateTokenSubscriptionRequest, WorldMetadataRequest, WorldMetadataResponse
+};
🧰 Tools
🪛 GitHub Actions: ci

[warning] 57-57: Long import statement should be split into multiple lines for better readability.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea87879 and 1a09c65.

📒 Files selected for processing (4)
  • crates/torii/grpc/proto/world.proto (2 hunks)
  • crates/torii/grpc/src/client.rs (5 hunks)
  • crates/torii/grpc/src/server/mod.rs (2 hunks)
  • crates/torii/grpc/src/server/subscriptions/token.rs (6 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/mod.rs

[warning] 57-57: Long import statement should be split into multiple lines for better readability.


[warning] 1404-1404: Function parameters should be formatted to enhance readability.


[warning] 1411-1411: Chained method calls should be formatted for clarity.

🔇 Additional comments (7)
crates/torii/grpc/src/server/subscriptions/token.rs (2)

23-23: LGTM! Logging target updated for token subscriptions.

The LOG_TARGET constant has been correctly updated to reflect the token subscription feature.


48-48: LGTM! Initial response includes subscription ID.

The initial empty response now correctly includes the subscription_id, maintaining consistency with the subscription pattern.

crates/torii/grpc/src/client.rs (1)

166-179: LGTM! Token subscription update implementation.

The update_tokens_subscription method follows the established pattern for subscription updates, with proper error handling and type conversions.

crates/torii/grpc/src/server/mod.rs (1)

1369-1382: LGTM! Token subscription update handler.

The implementation correctly handles the subscription update request, with proper error handling and type conversions.

crates/torii/grpc/proto/world.proto (3)

46-48: LGTM! New RPC method for token subscription updates.

The UpdateTokensSubscription RPC method is well-defined with proper return type and documentation.


106-111: LGTM! Updated SubscribeTokensResponse message.

The SubscribeTokensResponse message has been properly updated to include the subscription_id field with clear documentation.


113-119: LGTM! New UpdateTokenSubscriptionRequest message.

The UpdateTokenSubscriptionRequest message is well-structured with proper field documentation and numbering.

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: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a09c65 and 2003973.

📒 Files selected for processing (6)
  • crates/torii/client/src/client/mod.rs (1 hunks)
  • crates/torii/grpc/proto/world.proto (2 hunks)
  • crates/torii/grpc/src/client.rs (5 hunks)
  • crates/torii/grpc/src/server/mod.rs (2 hunks)
  • crates/torii/grpc/src/server/subscriptions/token.rs (6 hunks)
  • crates/torii/grpc/src/types/mod.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • crates/torii/client/src/client/mod.rs
  • crates/torii/grpc/proto/world.proto
  • crates/torii/grpc/src/types/mod.rs
  • crates/torii/grpc/src/server/mod.rs
  • crates/torii/grpc/src/server/subscriptions/token.rs
🔇 Additional comments (3)
crates/torii/grpc/src/client.rs (3)

132-164: Improve error handling in token subscription.

Ohayo sensei! The implementation has a potential panic point in the try_into() call. Let's handle the error gracefully.

-                    Some(token) => token.try_into().expect("must able to serialize"),
+                    Some(token) => token.try_into().map_err(Error::Schema)?,

166-179: LGTM! Clean implementation of token subscription update.

The implementation properly handles errors and follows the established pattern.


433-449: LGTM! Well-structured stream implementation.

The implementation follows the established pattern used by other stream types in the codebase.

Comment on lines +392 to +403
(
res.subscription_id,
match res.balance {
Some(balance) => balance.try_into().expect("must able to serialize"),
None => TokenBalance {
balance: U256::ZERO,
account_address: Felt::ZERO,
contract_address: Felt::ZERO,
token_id: "".to_string(),
},
},
)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply consistent error handling pattern.

Similar to the token subscription, let's handle the error gracefully here as well.

-                    Some(balance) => balance.try_into().expect("must able to serialize"),
+                    Some(balance) => balance.try_into().map_err(Error::Schema)?,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
(
res.subscription_id,
match res.balance {
Some(balance) => balance.try_into().expect("must able to serialize"),
None => TokenBalance {
balance: U256::ZERO,
account_address: Felt::ZERO,
contract_address: Felt::ZERO,
token_id: "".to_string(),
},
},
)
(
res.subscription_id,
match res.balance {
Some(balance) => balance.try_into().map_err(Error::Schema)?,
None => TokenBalance {
balance: U256::ZERO,
account_address: Felt::ZERO,
contract_address: Felt::ZERO,
token_id: "".to_string(),
},
},
)

Comment on lines +153 to +161
None => Token {
id: "".to_string(),
contract_address: Felt::ZERO,
name: "".to_string(),
symbol: "".to_string(),
decimals: 0,
metadata: "".to_string(),
},
},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider throwing an error instead of returning an empty token.

Returning an empty token when None is received might hide potential issues. Consider throwing an error to make the failure more explicit.

-                    None => Token {
-                        id: "".to_string(),
-                        contract_address: Felt::ZERO,
-                        name: "".to_string(),
-                        symbol: "".to_string(),
-                        decimals: 0,
-                        metadata: "".to_string(),
-                    },
+                    None => return Err(Error::Schema(SchemaError::MissingExpectedData("token".to_string()))),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
None => Token {
id: "".to_string(),
contract_address: Felt::ZERO,
name: "".to_string(),
symbol: "".to_string(),
decimals: 0,
metadata: "".to_string(),
},
},
None => return Err(Error::Schema(SchemaError::MissingExpectedData("token".to_string()))),
},

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: 3

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2003973 and 1bab5dc.

📒 Files selected for processing (7)
  • crates/torii/client/src/client/mod.rs (1 hunks)
  • crates/torii/grpc/src/server/mod.rs (2 hunks)
  • crates/torii/grpc/src/server/subscriptions/token.rs (6 hunks)
  • crates/torii/grpc/src/server/subscriptions/token_balance.rs (4 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (2 hunks)
  • crates/torii/sqlite/src/executor/mod.rs (5 hunks)
  • crates/torii/sqlite/src/types.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/torii/client/src/client/mod.rs
  • crates/torii/grpc/src/server/subscriptions/token.rs
  • crates/torii/grpc/src/server/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: docs
  • GitHub Check: ensure-wasm
  • GitHub Check: build
🔇 Additional comments (7)
crates/torii/sqlite/src/executor/mod.rs (3)

26-26: Ohayo sensei: Newly introduced TokenBalance import is a neat addition
No concerns here, as it aligns well with the rest of the broker messages.


52-52: Ohayo sensei: TokenBalanceUpdated enum variant is well-defined
Adding this variant enhances the system’s capability to handle token balance updates.


827-827: Ohayo sensei: Introducing TokenBalanceUpdated dispatch
No issues noted—this dispatch pattern remains consistent with the publishing of other event types.

crates/torii/sqlite/src/types.rs (2)

127-136: Ohayo sensei: OptimisticToken struct introduction
This new struct seems consistent with the system’s approach to optimistic updates. No immediate concerns.


150-157: Ohayo sensei: OptimisticTokenBalance struct addition
Matches well with the shift to optimistic updates for token balances, ensuring alignment with the newly introduced TokenBalanceUpdated message.

crates/torii/grpc/src/server/subscriptions/token_balance.rs (1)

17-17: Ohayo sensei: Transition to OptimisticTokenBalance
All updated lines correctly replace the previous TokenBalance usage. This consistent switch ensures streamlined publishing and subscription of token balance events. No issues spotted.

Also applies to: 101-102, 109-109, 120-120, 131-131

crates/torii/sqlite/src/executor/erc.rs (1)

19-19: Ohayo! Import change looks good, sensei!

The addition of OptimisticTokenBalance aligns well with the token subscription enhancement objectives.

Comment on lines +515 to +517
SimpleBroker::publish(unsafe {
std::mem::transmute::<EntityUpdated, OptimisticEntity>(entity_updated.clone())
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo sensei: Using unsafe transmute is risky
Transmute can lead to undefined behavior if memory layouts diverge. A safer alternative is a dedicated conversion method or trait implementation.

Below is a suggested diff to replace the unsafe block with a From conversion:

- SimpleBroker::publish(unsafe {
-     std::mem::transmute::<EntityUpdated, OptimisticEntity>(entity_updated.clone())
- });
+ SimpleBroker::publish(OptimisticEntity::from(entity_updated.clone()));

And an example of the safe conversion trait (placed in an appropriate file or module):

impl From<EntityUpdated> for OptimisticEntity {
    fn from(eu: EntityUpdated) -> Self {
        OptimisticEntity {
            id: eu.id,
            keys: eu.keys,
            event_id: eu.event_id,
            executed_at: eu.executed_at,
            created_at: eu.created_at,
            updated_at: eu.updated_at,
            updated_model: eu.updated_model,
            deleted: eu.deleted,
        }
    }
}

Comment on lines +582 to +586
SimpleBroker::publish(unsafe {
std::mem::transmute::<EventMessageUpdated, OptimisticEventMessage>(
event_message.clone(),
)
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo sensei: Unsafe memory transmute for EventMessageUpdated
Recommend using a safe conversion approach as above to avoid undefined behavior.

- SimpleBroker::publish(unsafe {
-     std::mem::transmute::<EventMessageUpdated, OptimisticEventMessage>(
-         event_message.clone(),
-     )
- });
+ SimpleBroker::publish(OptimisticEventMessage::from(event_message.clone()));
impl From<EventMessageUpdated> for OptimisticEventMessage {
    fn from(emu: EventMessageUpdated) -> Self {
        OptimisticEventMessage {
            id: emu.id,
            keys: emu.keys,
            event_id: emu.event_id,
            executed_at: emu.executed_at,
            created_at: emu.created_at,
            updated_at: emu.updated_at,
            updated_model: emu.updated_model,
            historical: emu.historical,
        }
    }
}

Comment on lines +179 to +182
SimpleBroker::publish(unsafe {
std::mem::transmute::<TokenBalance, OptimisticTokenBalance>(token_balance.clone())
});
self.publish_queue.push(BrokerMessage::TokenBalanceUpdated(token_balance));
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo! Critical safety concern with unsafe transmute, sensei!

The use of unsafe { std::mem::transmute } is dangerous as it assumes compatible memory layouts between TokenBalance and OptimisticTokenBalance. If these types' layouts change in the future, this could lead to undefined behavior.

Consider these safer alternatives:

-        SimpleBroker::publish(unsafe {
-            std::mem::transmute::<TokenBalance, OptimisticTokenBalance>(token_balance.clone())
-        });
+        // Option 1: Implement From/Into trait (preferred)
+        SimpleBroker::publish(OptimisticTokenBalance::from(token_balance.clone()));
+
+        // Option 2: Manual conversion if more control is needed
+        SimpleBroker::publish(OptimisticTokenBalance {
+            id: token_balance.id.clone(),
+            // ... map other fields
+        });

The additional publish to queue looks good and enhances token update tracking.

Committable suggestion skipped: line range outside the PR's diff.

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

♻️ Duplicate comments (2)
crates/torii/sqlite/src/executor/erc.rs (2)

179-182: ⚠️ Potential issue

Ohayo! Critical safety concern with unsafe transmute, sensei!

The use of unsafe { std::mem::transmute } is dangerous as it assumes compatible memory layouts between TokenBalance and OptimisticTokenBalance. If these types' layouts change in the future, this could lead to undefined behavior.

Consider these safer alternatives:

-        SimpleBroker::publish(unsafe {
-            std::mem::transmute::<TokenBalance, OptimisticTokenBalance>(token_balance.clone())
-        });
+        // Option 1: Implement From/Into trait (preferred)
+        SimpleBroker::publish(OptimisticTokenBalance::from(token_balance.clone()));
+
+        // Option 2: Manual conversion if more control is needed
+        SimpleBroker::publish(OptimisticTokenBalance {
+            id: token_balance.id.clone(),
+            // ... map other fields
+        });

The additional publish to queue looks good and enhances token update tracking.


363-366: ⚠️ Potential issue

Ohayo! Similar safety concern with unsafe transmute here too, sensei!

The same unsafe transmute pattern is used here for converting Token to OptimisticToken. This carries the same risks as the previous instance.

Consider these safer alternatives:

-            SimpleBroker::publish(unsafe {
-                std::mem::transmute::<Token, OptimisticToken>(token.clone())
-            });
+            // Option 1: Implement From/Into trait (preferred)
+            SimpleBroker::publish(OptimisticToken::from(token.clone()));
+
+            // Option 2: Manual conversion if more control is needed
+            SimpleBroker::publish(OptimisticToken {
+                id: token.id.clone(),
+                // ... map other fields
+            });

The additional publish to queue enhances token registration tracking.

🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)

1-372: Ohayo! Consider implementing conversion traits systematically, sensei!

The repeated use of unsafe transmute for converting between regular and optimistic types suggests a need for a more systematic approach. Consider:

  1. Implementing From/Into traits for all pairs of regular and optimistic types
  2. Creating a common trait for optimistic conversions
  3. Using derive macros if the conversion patterns are similar across types

This would:

  • Eliminate unsafe code
  • Make conversions explicit and maintainable
  • Ensure type safety at compile time
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bab5dc and 70d7fc2.

📒 Files selected for processing (2)
  • crates/torii/client/src/client/mod.rs (2 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/torii/client/src/client/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: clippy
  • GitHub Check: docs
  • GitHub Check: build
🔇 Additional comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)

19-19: Ohayo! Import changes look good, sensei!

The import statement correctly includes the new optimistic types needed for the token updates.

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: 1

♻️ Duplicate comments (2)
crates/torii/sqlite/src/executor/erc.rs (1)

199-201: ⚠️ Potential issue

Ohayo sensei! Unsafe transmute detected.

Using unsafe { std::mem::transmute } is dangerous as it assumes compatible memory layouts between types.

Also applies to: 413-415

crates/torii/sqlite/src/executor/mod.rs (1)

516-518: ⚠️ Potential issue

Ohayo sensei! Unsafe transmute detected.

Using unsafe { std::mem::transmute } is dangerous as it assumes compatible memory layouts between types.

Also applies to: 583-587

🧹 Nitpick comments (1)
crates/torii/sqlite/src/types.rs (1)

127-136: Consider extracting common fields into a trait or base struct.

The optimistic and non-optimistic types share identical fields. Consider extracting these into a common trait or base struct to reduce code duplication and improve maintainability:

trait TokenFields {
    fn id(&self) -> &str;
    fn contract_address(&self) -> &str;
    fn name(&self) -> &str;
    fn symbol(&self) -> &str;
    fn decimals(&self) -> u8;
    fn metadata(&self) -> &str;
}

trait TokenBalanceFields {
    fn id(&self) -> &str;
    fn balance(&self) -> &str;
    fn account_address(&self) -> &str;
    fn contract_address(&self) -> &str;
    fn token_id(&self) -> &str;
}

Also applies to: 138-147, 149-157, 159-167

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70d7fc2 and fcb0722.

📒 Files selected for processing (4)
  • crates/torii/grpc/src/client.rs (5 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (3 hunks)
  • crates/torii/sqlite/src/executor/mod.rs (5 hunks)
  • crates/torii/sqlite/src/types.rs (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: ensure-wasm
  • GitHub Check: docs
  • GitHub Check: clippy
🔇 Additional comments (4)
crates/torii/grpc/src/client.rs (3)

153-162: Improve error handling and avoid empty tokens.

Ohayo sensei! The implementation has two potential issues:

  1. Using expect could cause panics
  2. Returning an empty token might mask issues

Apply this diff to handle errors gracefully and make failures explicit:

-                    Some(token) => token.try_into().expect("must able to serialize"),
-                    None => Token {
-                        id: "".to_string(),
-                        contract_address: Felt::ZERO,
-                        name: "".to_string(),
-                        symbol: "".to_string(),
-                        decimals: 0,
-                        metadata: "".to_string(),
-                    },
+                    Some(token) => token.try_into().map_err(Error::Schema)?,
+                    None => return Err(Error::Schema(SchemaError::MissingExpectedData("token".to_string()))),

396-403: Apply consistent error handling pattern.

Ohayo! Similar to the token subscription, let's handle errors gracefully here as well.

Apply this diff to improve error handling:

-                    Some(balance) => balance.try_into().expect("must able to serialize"),
-                    None => TokenBalance {
-                        balance: U256::ZERO,
-                        account_address: Felt::ZERO,
-                        contract_address: Felt::ZERO,
-                        token_id: "".to_string(),
-                    },
+                    Some(balance) => balance.try_into().map_err(Error::Schema)?,
+                    None => return Err(Error::Schema(SchemaError::MissingExpectedData("balance".to_string()))),

434-450: LGTM! Clean implementation of token streaming types.

The implementation follows the established patterns in the codebase, with good use of generics and traits.

crates/torii/sqlite/src/executor/mod.rs (1)

53-53: LGTM! TokenBalanceUpdated handling added.

The TokenBalanceUpdated variant and its handling in send_broker_message look good.

Also applies to: 832-832

Comment on lines +127 to +136
#[derive(FromRow, Deserialize, Debug, Clone)]
#[serde(rename_all = "camelCase")]
pub struct OptimisticToken {
pub id: String,
pub contract_address: String,
pub name: String,
pub symbol: String,
pub decimals: u8,
pub metadata: String,
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo sensei! Consider implementing From/Into traits for optimistic types.

The optimistic types are identical to their non-optimistic counterparts. To avoid unsafe transmutes and improve type safety, implement From/Into traits:

impl From<Token> for OptimisticToken {
    fn from(token: Token) -> Self {
        Self {
            id: token.id,
            contract_address: token.contract_address,
            name: token.name,
            symbol: token.symbol,
            decimals: token.decimals,
            metadata: token.metadata,
        }
    }
}

impl From<TokenBalance> for OptimisticTokenBalance {
    fn from(balance: TokenBalance) -> Self {
        Self {
            id: balance.id,
            balance: balance.balance,
            account_address: balance.account_address,
            contract_address: balance.contract_address,
            token_id: balance.token_id,
        }
    }
}

Also applies to: 149-157

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

🧹 Nitpick comments (1)
crates/torii/grpc/src/types/mod.rs (1)

75-76: Enhance the comment clarity, sensei!

The implementation is correct, but let's make the comment more specific about the compatibility requirement.

-    // Remove the "0x" prefix from the balance to be compatible with U256::from_be_hex.
+    // U256::from_be_hex expects a hex string without "0x" prefix, so we need to trim it.
+    // This is safe as trim_start_matches is a no-op if the prefix isn't found.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcb0722 and 657510e.

📒 Files selected for processing (2)
  • crates/torii/grpc/src/types/mod.rs (3 hunks)
  • crates/torii/sqlite/src/executor/mod.rs (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/torii/sqlite/src/executor/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: docs
  • GitHub Check: ensure-wasm
  • GitHub Check: clippy
🔇 Additional comments (2)
crates/torii/grpc/src/types/mod.rs (2)

39-47: Ohayo! The Token struct changes look good, sensei!

The addition of the id field is well-structured and aligns perfectly with the PR objectives for token subscription via id.


49-61: The TryFrom implementation is properly updated, sensei!

The mapping of token_id to the new id field is correctly implemented.

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 @Larkooo! Issue on ERC20 balances is still on-going, let's tackle that in an other PR.

@glihm glihm enabled auto-merge (squash) February 12, 2025 17:53
@glihm glihm merged commit 130adb1 into dojoengine:main Feb 12, 2025
13 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