feat(torii-grpc): pagination for tokens & order by & next cursor #3143
Conversation
WalkthroughOhayo, sensei! This change introduces new request messages for subscribing to token updates and balances, namely Changes
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: 2
🔭 Outside diff range comments (2)
crates/torii/grpc/src/server/mod.rs (2)
1411-1427:⚠️ Potential issueThe WorldServer implementation doesn't pass the pagination parameters!
The retrieve_tokens method signature was updated, but this implementation doesn't pass the new limit and offset parameters.
async fn retrieve_tokens( &self, request: Request<RetrieveTokensRequest>, ) -> Result<Response<RetrieveTokensResponse>, Status> { - let RetrieveTokensRequest { contract_addresses, token_ids } = request.into_inner(); + let RetrieveTokensRequest { contract_addresses, token_ids, limit, offset } = request.into_inner(); let contract_addresses = contract_addresses .iter() .map(|address| Felt::from_bytes_be_slice(address)) .collect::<Vec<_>>(); let token_ids = token_ids.iter().map(|id| crypto_bigint::U256::from_be_slice(id)).collect::<Vec<_>>(); let tokens = self - .retrieve_tokens(contract_addresses, token_ids) + .retrieve_tokens(contract_addresses, token_ids, limit, offset) .await .map_err(|e| Status::internal(e.to_string()))?; Ok(Response::new(tokens)) }
1464-1485:⚠️ Potential issueMissing pagination parameters in WorldServer implementation for token balances too!
Similar to the retrieve_tokens method, this implementation doesn't pass the new limit and offset parameters.
async fn retrieve_token_balances( &self, request: Request<RetrieveTokenBalancesRequest>, ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { - let RetrieveTokenBalancesRequest { account_addresses, contract_addresses, token_ids } = + let RetrieveTokenBalancesRequest { account_addresses, contract_addresses, token_ids, limit, offset } = request.into_inner(); let account_addresses = account_addresses .iter() .map(|address| Felt::from_bytes_be_slice(address)) .collect::<Vec<_>>(); let contract_addresses = contract_addresses .iter() .map(|address| Felt::from_bytes_be_slice(address)) .collect::<Vec<_>>(); let token_ids = token_ids.iter().map(|id| U256::from_be_slice(id)).collect::<Vec<_>>(); let balances = self - .retrieve_token_balances(account_addresses, contract_addresses, token_ids) + .retrieve_token_balances(account_addresses, contract_addresses, token_ids, limit, offset) .await .map_err(|e| Status::internal(e.to_string()))?; Ok(Response::new(balances)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/grpc/proto/world.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(4 hunks)
🔇 Additional comments (4)
crates/torii/grpc/proto/world.proto (2)
102-105: Ohayo! The pagination fields added to RetrieveTokensRequest look good, sensei!These pagination fields will allow clients to limit the number of results returned and skip records, which is great for performance when dealing with large datasets.
139-142: Pagination support for token balances - nice work!Adding pagination to token balances follows the same pattern as tokens, which maintains consistency in the API.
crates/torii/grpc/src/server/mod.rs (2)
848-849: Method signature update for retrieve_tokens looks good!Adding the optional pagination parameters is consistent with the proto file changes.
895-896: Method signature update for retrieve_token_balances looks good!The optional pagination parameters match the proto changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/grpc/src/server/mod.rs (1)
825-834:⚠️ Potential issueOhayo sensei! SQL clause ordering issue with
LIMITandOFFSET.
They appear before theWHEREclause. Standard SQL requiresWHERE ... ORDER BY ... LIMIT ... OFFSET. Move theWHEREandORDER BYbefore these clauses.Apply a fix like this:
if !conditions.is_empty() { - query += &format!(" WHERE {} ORDER BY id", conditions.join(" AND ")); } +if !conditions.is_empty() { + query += &format!(" WHERE {} ", conditions.join(" AND ")); + query += "ORDER BY id"; +} if let Some(limit) = limit { query += &format!(" LIMIT {}", limit); } if let Some(offset) = offset { query += &format!(" OFFSET {}", offset); }
🧹 Nitpick comments (2)
crates/torii/grpc/proto/world.proto (2)
102-107: Ohayo sensei! Nice addition of pagination fields inRetrieveTokensRequest.
Includinglimitandoffsetis a solid approach to enable pagination. Consider ensuring in the server logic that zero or extremely large values are handled gracefully to prevent potential performance issues.
147-151: Ohayo sensei! Pagination fields for token balances look correct.
Just make sure the server properly bounds or rejects invalidlimitoroffsetvalues so they don’t degrade performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/grpc/proto/world.proto(3 hunks)crates/torii/grpc/src/client.rs(7 hunks)crates/torii/grpc/src/server/mod.rs(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (21)
crates/torii/grpc/proto/world.proto (4)
38-38: Ohayo sensei! Looks good on using the new subscription request.
Thank you for matching the new proto request type. This aligns with the updated usage in the server code.
44-44: Ohayo sensei! Smooth transition to theSubscribeTokensRequest.
Great job switching the RPC to the newly introduced request type for subscribing to tokens.
108-114: Ohayo sensei! The newSubscribeTokensRequestmessage is integrated well.
Everything looks consistent with the existing types, and field naming is clear.
153-161: Ohayo sensei!SubscribeTokenBalancesRequestis properly structured.
This new message aligns well with your subscription design and reuses field naming from other requests.crates/torii/grpc/src/server/mod.rs (10)
66-69: Ohayo sensei! Updated imports for token subscription requests look consistent.
ReferencingSubscribeTokenBalancesRequestandSubscribeTokensRequestmatches the proto updates perfectly.
269-270: Ohayo sensei! Good job addinglimitandoffsetto theentities_all/query_by_hashed_keyscalls.
This further unifies pagination across entity queries. Verify that the final SQL statements placeLIMITandOFFSETafterWHEREandORDER BYas well.Also applies to: 281-282
807-808: Ohayo sensei! Thanks for adding thelimitandoffsetparameters toretrieve_tokens.
This syncs perfectly with the updated request.
854-855: Ohayo sensei! Consistent addition of pagination parameters toretrieve_token_balances.
This keeps the interface uniform across token retrieval endpoints.
1074-1105: Ohayo sensei! The new limit/offset logic forretrieve_eventsis well-placed.
Since you only addLIMITandOFFSETafter forming theORDER BY id DESCclause, the query ordering is correct here. Good job.
1413-1428: Ohayo sensei! Retrieving tokens with explicit limit/offset is implemented cleanly.
Ensuringif limit > 0 { Some(limit) } else { None }is a nice pattern that seamlessly handles “no limit.”
1436-1438: Ohayo sensei!subscribe_tokensusage is consistent with the newSubscribeTokensRequest.
No issues spotted.
1473-1479: Ohayo sensei! Great job mapping the newRetrieveTokenBalancesRequestfields.
This mirrors the approach used for tokens, preserving a predictable structure.
1491-1497: Ohayo sensei! Correct holding pattern on limit/offset inretrieve_token_balances.
Same pattern:if limit > 0 { Some(limit) } else { None }. Makes the logic consistent.
1559-1561: Ohayo sensei! Alignment withSubscribeTokenBalancesRequestis on point.
Your final usage ofcontract_addresses,account_addresses,token_idsmatches the updated proto.crates/torii/grpc/src/client.rs (7)
21-24: Ohayo! Clean imports for new request types.The imports have been updated to include the new subscription-specific request and response types for token operations, maintaining a clean organization of imports.
117-123: Added pagination support to retrieve_tokens method, sensei!The method signature has been updated to include optional
limitandoffsetparameters, which is the standard approach for implementing pagination. This will allow clients to request specific ranges of tokens.
131-132: Setting sensible defaults for pagination parameters.Using
unwrap_or(0)provides sensible defaults - a limit of 0 typically means "no limit" in database queries, and an offset of 0 means start from the beginning.
144-144: Transitioned from retrieve to subscribe request type.Changed from using
RetrieveTokensRequestto the more appropriateSubscribeTokensRequestfor the subscription method, which better reflects the operation's intent.
195-202: Added pagination support to retrieve_token_balances method, sensei!Similar to the tokens retrieval method, this adds optional
limitandoffsetparameters to enable pagination for token balances.
214-215: Consistent default implementation for pagination parameters.The same default approach used in the tokens retrieval method is applied here, maintaining consistency across the API.
394-394: Updated subscribe_token_balances to use subscription-specific request type.Changed to use
SubscribeTokenBalancesRequestinstead ofRetrieveTokenBalancesRequest, which is more semantically accurate for a subscription operation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/client/src/client/mod.rs (1)
84-117: Consider adding documentation for the pagination parameters, sensei!While the implementation is solid, it would be helpful to add documentation comments for the new parameters explaining their purpose and any constraints (e.g., maximum values or behavior when null).
/// Retrieves tokens matching contract addresses. pub async fn tokens( &self, contract_addresses: Vec<Felt>, token_ids: Vec<U256>, + /// Maximum number of tokens to return limit: Option<u32>, + /// Number of tokens to skip offset: Option<u32>, ) -> Result<Vec<Token>, Error> {/// Retrieves token balances for account addresses and contract addresses. pub async fn token_balances( &self, account_addresses: Vec<Felt>, contract_addresses: Vec<Felt>, token_ids: Vec<U256>, + /// Maximum number of token balances to return limit: Option<u32>, + /// Number of token balances to skip offset: Option<u32>, ) -> Result<Vec<TokenBalance>, Error> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/client/src/client/mod.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
crates/torii/client/src/client/mod.rs (2)
84-95: Ohayo! Nice addition of pagination support for tokens retrieval, sensei!The implementation correctly adds optional
limitandoffsetparameters to the method signature, allowing clients to paginate through token results. This change aligns perfectly with the PR objective of implementing pagination for tokens.
97-117: Clean implementation of pagination for token balances, sensei!Similar to the tokens method, this implementation nicely adds pagination support with optional
limitandoffsetparameters. I appreciate the multi-line formatting for improved readability with the increased number of parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1081-1117: Binding limit/offset as strings vs integers
Ohayo sensei! Using strings in bind parameters is acceptable, but consider binding them as integers for extra clarity and consistency.- bind_values.push(limit.to_string()); - bind_values.push(offset.to_string()); + row_events = row_events.bind(limit); + row_events = row_events.bind(offset);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
crates/torii/grpc/src/server/mod.rs (14)
66-69: Ohayo sensei, new subscription requests introduced!
Your additions for token and token balance subscriptions align with the PR objectives. No issues found here.
269-270: Pagination parameters forentities_all
The newlimitandoffsetfields are introduced cleanly, matching the rest of the pagination strategy with no concerns.Also applies to: 281-282
807-808: Including limit/offset inretrieve_tokens
Nice addition to support pagination within this function. Implementation looks good.
829-837: SQL ordering is correct
PlacingWHERE, thenORDER BY, followed byLIMITandOFFSETis best practice for valid SQL.
856-858: Additional parameters inretrieve_token_balances
This signature change aligns well with the introduced pagination.
886-895: Maintaining correct clause order for token_balances
Confirmed the pagination clauses are appended properly afterORDER BY.
980-982: Extracting limit and offset from the query
UsingOptionto handle non-positive values is logical. No issues found.
1078-1080: Pagination forretrieve_events
Great inclusion of limit and offset for event retrieval.
1417-1418: Destructuring new fields inRetrieveTokensRequest
Implementation is seamless, no concerns.
1427-1432: Passing limit/offset toretrieve_tokens
Looks consistent with the newly introduced parameters.
1440-1440: Updatedsubscribe_tokenssignature
Matches the request structure. Implementation looks correct, sensei.Also applies to: 1442-1442
1477-1483: New pagination fields forRetrieveTokenBalancesRequest
No issues: the additions are coherent with the existing approach.
1495-1501: Applying pagination inretrieve_token_balances
Great job wiring up limit and offset arguments here.
1563-1563:subscribe_token_balancesrequest
The subscription method structure looks clean with no concurrency pitfalls.Also applies to: 1565-1565
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3143 +/- ##
==========================================
- Coverage 55.65% 55.61% -0.05%
==========================================
Files 443 443
Lines 62878 62953 +75
==========================================
+ Hits 34994 35010 +16
- Misses 27884 27943 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/client/src/client/mod.rs(2 hunks)crates/torii/grpc/proto/world.proto(3 hunks)crates/torii/grpc/src/client.rs(7 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/src/client.rs
- crates/torii/grpc/proto/world.proto
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/grpc/src/server/mod.rs (2)
crates/torii/client/src/client/mod.rs (4)
tokens(84-95)tokens(94-94)new(38-43)balances(116-116)crates/torii/grpc/src/client.rs (3)
retrieve_token_balances(195-220)new(61-74)new(78-85)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (19)
crates/torii/client/src/client/mod.rs (4)
88-90: Ohayo, sensei! Great addition of pagination parameters to tokens method!Adding the optional pagination parameters (
limit,offset,cursor) to the tokens method allows clients to control result set size and implement proper pagination. This aligns well with the PR objectives.
93-95: Correctly passing pagination parameters to the underlying gRPC clientThe implementation properly passes the new pagination parameters to the underlying gRPC client method.
105-107: Consistent implementation of pagination in token_balances method!Good job implementing the same pagination pattern for token_balances as you did for tokens. This consistency makes the API more predictable for clients.
110-118: Well-structured parameter passing to retrieve_token_balancesThe updated method call correctly passes all parameters to the underlying gRPC method. The code is well-formatted and easy to read.
crates/torii/grpc/src/server/mod.rs (15)
66-69: Ohayo, sensei! Good proto import updates to support new message typesThe imports have been updated to include the new request and response types for token subscriptions.
269-270: Consistent pagination parameter addition across methodsAdding pagination parameters to the entities_all method maintains consistency with other query methods.
807-809: Good addition of pagination parameters to retrieve_tokensThe method signature now properly includes the pagination parameters that are standard across the codebase.
826-829: Well-implemented cursor-based pagination!Adding cursor-based pagination alongside offset pagination gives clients flexibility in how they navigate through large result sets. The cursor implementation looks good.
835-845: Fixed SQL query construction order - looks good!The SQL query construction now follows the correct order: WHERE clause, followed by ORDER BY, then LIMIT and OFFSET. Using bind parameters for limit and offset also prevents SQL injection.
854-857: Nice implementation of next_cursor in responseIncluding the next_cursor in the response allows clients to easily request the next page of results without having to calculate offsets manually.
865-867: Consistent pagination parameters in retrieve_token_balancesThe method signature now properly includes the same pagination parameters as retrieve_tokens.
892-895: Good cursor implementation in token_balancesThe cursor implementation for token_balances follows the same pattern as in retrieve_tokens, maintaining consistency.
918-921: Well-implemented next_cursor in token_balances responseGood job implementing next_cursor consistently across token-related responses.
1094-1095: Good addition of pagination to retrieve_eventsAdding pagination to retrieve_events makes the API consistent across all data retrieval methods.
1104-1125: Well-structured SQL query construction for eventsThe query construction for events pagination is well-implemented with proper ordering of clauses and parameter binding.
1433-1449: Clean implementation of handler for retrieve_tokensThe handler properly extracts and passes all parameters to the underlying function, with good handling of default values.
1457-1459: Updated request type for subscribe_tokensThe handler has been updated to use the new request type, keeping the implementation in sync with the proto changes.
1494-1520: Well-structured handler for retrieve_token_balancesThe handler properly extracts all parameters from the request and passes them to the implementation function.
1582-1584: Updated request type for subscribe_token_balancesThe handler has been updated to use the new request type, maintaining consistency with the proto changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/grpc/src/types/mod.rs (2)
21-25: Ohayo, sensei! New pagination struct looks good but could benefit from documentation.The new
Page<T>struct is a clean, generic implementation for pagination which aligns perfectly with our PR objectives. The structure provides a reusable component for paginated data across different endpoints.Consider adding documentation comments to explain:
- The purpose of this pagination structure
- How the
next_cursorshould be formatted and used- Example usage in token-related operations
+/// Represents a paginated collection of items with cursor-based navigation. +/// +/// # Example +/// ``` +/// let page = Page { +/// items: vec![token1, token2], +/// next_cursor: "base64_encoded_cursor_value" +/// }; +/// ``` #[derive(Debug, Serialize, Deserialize, PartialEq, Hash, Eq, Clone)] pub struct Page<T> { + /// Collection of items for the current page pub items: Vec<T>, + /// Opaque cursor string to fetch the next page of results. + /// Pass this value to the `cursor` field in subsequent requests. + /// An empty string indicates there are no more results. pub next_cursor: String, }
21-25: Consider implementing helpful methods and conversion traits for the Page structWhile the current implementation works for basic pagination, adding a few helper methods would improve the developer experience.
Consider implementing:
- A method to check if there are more pages available
- Conversion traits to/from protobuf types for consistency with the rest of the codebase
#[derive(Debug, Serialize, Deserialize, PartialEq, Hash, Eq, Clone)] pub struct Page<T> { pub items: Vec<T>, pub next_cursor: String, } + +impl<T> Page<T> { + /// Returns true if there are more pages available. + pub fn has_more(&self) -> bool { + !self.next_cursor.is_empty() + } + + /// Creates a new page with the given items and next cursor. + pub fn new(items: Vec<T>, next_cursor: String) -> Self { + Self { items, next_cursor } + } +}You might also want to implement conversion traits between this Page struct and any corresponding protobuf message types that would be defined in your proto files.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/client/src/client/mod.rs(3 hunks)crates/torii/grpc/src/types/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/client/src/client/mod.rs
Larkooo
dismissed
glihm’s stale review
Fixed token sql query when paginating & added cursor support
Larkooo
force-pushed
the
pagination-tokens-grpc
branch
from
85fa1c1 to
8146b8e
Compare
#3142
Summary by CodeRabbit