feat(torii): retrieve controllers & usernames graphql and grpc #2992
Conversation
WalkthroughOhayo, sensei! This pull request introduces new protocol buffer definitions and extends the gRPC service. A new message type, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant W as WorldService
participant D as DojoWorld
participant DB as Database
C->>W: RetrieveControllersRequest (contract_addresses)
W->>D: retrieve_controllers(contract_addresses)
D->>DB: Execute SQL query on 'controllers'
DB-->>D: Return controller data (address, username, timestamp)
D-->>W: RetrieveControllersResponse (controllers)
W-->>C: RetrieveControllersResponse (controllers)
Possibly related PRs
Suggested reviewers
🪧 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 (5)
crates/torii/grpc/src/server/mod.rs (2)
1047-1077: Ohayo sensei! The SQL query looks good but let's handle invalid addresses gracefully.The SQL query construction is safe with proper parameterization. However, the address parsing could panic if the database contains invalid addresses.
Apply this diff to handle invalid addresses:
- address: address.parse::<Felt>().unwrap().to_bytes_be().to_vec(), + address: address.parse::<Felt>() + .map_err(|e| Error::from(ParseError::FromStr(e)))? + .to_bytes_be() + .to_vec(),
1307-1322: Consider adding input validation for empty contract addresses.While the error handling is good, consider validating that the contract addresses are not empty byte arrays before parsing them.
Apply this diff to add validation:
let contract_addresses = contract_addresses .iter() + .filter(|address| !address.is_empty()) .map(|address| Felt::from_bytes_be_slice(address)) .collect::<Vec<_>>();crates/torii/grpc/proto/types.proto (1)
190-194: Ohayo! Let's add documentation for the Controller message fields.The field types and numbers look good, but documentation would help users understand the purpose of each field.
Apply this diff to add documentation:
message Controller { + // The controller's address in bytes bytes address = 1; + // The username associated with the controller string username = 2; + // Unix timestamp when the controller was deployed uint64 deployed_at_timestamp = 3; }crates/torii/grpc/proto/world.proto (2)
58-60: Add documentation for the RetrieveControllers RPC method.The method signature looks good, but documentation would help users understand its purpose.
Apply this diff to add documentation:
+ // Retrieves controller information based on provided contract addresses. + // If no addresses are provided, returns all controllers. rpc RetrieveControllers (RetrieveControllersRequest) returns (RetrieveControllersResponse);
62-68: Add documentation for the request and response messages.The message structures look good, but documentation would help users understand their fields.
Apply this diff to add documentation:
message RetrieveControllersRequest { + // List of contract addresses to retrieve controllers for. + // If empty, returns all controllers. repeated bytes contract_addresses = 1; } message RetrieveControllersResponse { + // List of controllers matching the requested contract addresses repeated types.Controller controllers = 1; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/proto/world.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2992 +/- ##
==========================================
+ Coverage 56.97% 57.14% +0.17%
==========================================
Files 426 428 +2
Lines 56376 56802 +426
==========================================
+ Hits 32119 32461 +342
- Misses 24257 24341 +84 ☔ View full report in Codecov by Sentry. |
Larkooo
changed the title
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/graphql/src/object/controller.rs (2)
8-9: Ohayo sensei! Consider removing unnecessary derive.The
#[derive(Debug)]seems unnecessary for an empty struct that's only used as a type marker.-#[derive(Debug)] pub struct ControllerObject;
11-45: Implementation looks good but could be generalized.The implementation follows the established pattern and is correct. However, since this pattern is repeated across multiple entity types, consider extracting it into a macro or default implementation to reduce boilerplate.
crates/torii/graphql/src/constants.rs (2)
12-12: Consider grouping with other table constants.Move
CONTROLLER_TABLEconstant to be grouped with other table constants for better organization.pub const METADATA_TABLE: &str = "metadata"; +pub const CONTROLLER_TABLE: &str = "controllers"; -pub const CONTROLLER_TABLE: &str = "controllers";
69-70: Consider alphabetical ordering of type names.Move
CONTROLLER_TYPE_NAMEandCONTROLLER_NAMESto maintain alphabetical ordering with other type names and name tuples.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/graphql/src/constants.rs(2 hunks)crates/torii/graphql/src/mapping.rs(1 hunks)crates/torii/graphql/src/object/controller.rs(1 hunks)crates/torii/graphql/src/object/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/graphql/src/object/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (1)
crates/torii/graphql/src/mapping.rs (1)
180-188: Ohayo sensei! The type mapping looks perfect!The controller mapping is well-defined with appropriate non-null constraints and correct type usage for each field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1307-1322: Consider more specific error handling.The implementation is good, but we could improve error handling by providing more specific error statuses:
- .map_err(|e| Status::internal(e.to_string()))?; + .map_err(|e| match e { + Error::Sql(sqlx::Error::RowNotFound) => Status::not_found("Controllers not found"), + e => Status::internal(e.to_string()), + })?;This would provide better error context to API consumers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/torii/grpc/src/server/mod.rs (2)
59-65: LGTM!The new imports for controller-related types are properly organized.
1047-1077: Verify the database schema for deployed_at field.Ohayo, sensei! The implementation looks good, but based on past review comments, there might be a mismatch between the database schema (TEXT) and the code's expectation (DateTime). Let's verify the schema:
The timestamp conversion from
DateTime<Utc>tou64is handled correctly, but ensure that the database schema matches these expectations.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the schema of the controllers table echo "Checking controllers table schema..." sqlite3 .sqlite_db ".schema controllers"Length of output: 223
🏁 Script executed:
#!/bin/bash echo "Ohayo, sensei! Let's search for the controllers table schema and the deployed_at column definition in the repository..." echo "Searching for controllers table definition (e.g., CREATE TABLE controllers)..." rg -i "CREATE TABLE.*controllers" . echo "Searching for deployed_at column within the repository..." rg -i "deployed_at" .Length of output: 1555
Ohayo, sensei! The controllers table’s schema now shows that the deployed_at column is defined as TIMESTAMP (see
crates/torii/migrations/20250128051146_controllers.sql), which matches the code’s expectation of aDateTime<Utc>. No action is necessary here.
Summary by CodeRabbit
Controllerto capture essential details like address, username, and deployment timestamp.ControllerObjectfor handling controller-related queries.Controllerentity for improved structure and access.