feat(torii/graphql): use Connection abstraction to return data for ercBalance and ercTransfer query
#2527
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2527 +/- ##
==========================================
- Coverage 57.23% 57.02% -0.21%
==========================================
Files 408 409 +1
Lines 51791 52013 +222
==========================================
+ Hits 29645 29663 +18
- Misses 22146 22350 +204 ☔ View full report in Codecov by Sentry. |
lambda-0x
changed the title
Connection abstraction to return data for ercBalance queryConnection abstraction to return data for ercBalance and ercTransfer query
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
eb60abf to
fb20ebb
Compare
lambda-0x
force-pushed
the
spr/main/dd61fbb9
branch
from
48ec980 to
46b5b4b
Compare
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
2 times, most recently
from
bcfec7b to
4001fbd
Compare
lambda-0x
force-pushed
the
spr/main/dd61fbb9
branch
from
46b5b4b to
a7204ac
Compare
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
4001fbd to
1e9a382
Compare
WalkthroughOhayo, sensei! The changes in this pull request enhance the ERC-related functionality within the GraphQL module. New constants for table names and type names have been introduced, alongside significant updates to the 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: 4
🧹 Outside diff range and nitpick comments (9)
crates/torii/graphql/src/object/erc/mod.rs (1)
8-17: Ohayo again, sensei! Thehandle_cursorfunction looks solid!The function is well-structured and handles both success and error cases appropriately. It effectively decodes the cursor and formats the result for SQL queries. Great job!
A couple of small suggestions to make it even better:
- Consider adding a comment explaining the format of the returned string. This will help future maintainers understand its purpose and usage.
- The error message could be more specific about the expected cursor format to assist in troubleshooting.
Here's a suggested improvement:
fn handle_cursor( cursor: &str, direction: CursorDirection, id_column: &str, ) -> sqlx::Result<String> { + // Returns a formatted string for use in SQL queries, e.g., "id > '123'" match cursor::decode(cursor) { Ok((event_id, _)) => Ok(format!("{} {} '{}'", id_column, direction.as_ref(), event_id)), - Err(_) => Err(sqlx::Error::Decode("Invalid cursor format".into())), + Err(_) => Err(sqlx::Error::Decode("Invalid cursor format. Expected base64 encoded string.".into())), } }crates/torii/graphql/src/constants.rs (2)
12-13: Ohayo, sensei! New ERC table constants look good!The new constants
ERC_BALANCE_TABLEandERC_TRANSFER_TABLEare well-integrated with the existing table name constants. They follow the established naming convention and will be useful for database operations related to ERC balances and transfers.For consistency with other table names, consider using plurals for both:
-pub const ERC_BALANCE_TABLE: &str = "balances"; +pub const ERC_BALANCE_TABLE: &str = "erc_balances"; pub const ERC_TRANSFER_TABLE: &str = "erc_transfers";
Line range hint
51-53: Ohayo once more, sensei! ERC name tuple constants are looking sharp!The new constants
ERC_BALANCE_NAMEandERC_TRANSFER_NAMEare well-defined and follow the established pattern for name tuples. They use singular forms for the first element and empty strings for the second element, which is consistent with other similar constants.For better organization, consider moving these constants to be grouped with other similar name tuple constants (around line 45). This would improve readability and maintain a logical grouping of related constants.
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
Line range hint
246-246: Avoid usingassert!for runtime checksOhayo, sensei! Using
assert!(token_id.len() == 2);can cause a panic if the condition fails, which might not be desirable in production code. It's better to handle this scenario gracefully to prevent unexpected crashes. Consider replacing the assertion with proper error handling.You can apply this diff to address the issue:
- assert!(token_id.len() == 2); + if token_id.len() != 2 { + warn!("Unexpected token_id format: {}", row.token_id); + continue; + }
Line range hint
218-266: Handle unknown contract types more gracefullyOhayo, sensei! In the
erc_balance_connection_outputfunction, if an unknowncontract_typeis encountered, the code logs a warning and skips the entry. Consider whether skipping these entries is appropriate or if you should handle them differently to ensure all relevant data is processed correctly.crates/torii/graphql/src/object/erc/erc_transfer.rs (4)
51-84: Ohayo! Simplify the field creation in the resolver.Sensei, the creation of
fieldin theresolversmethod can be streamlined for better readability. The variablefieldis being reassigned, which might be unnecessary.Consider chaining the methods without reassigning:
let field = Field::new( self.name().0, TypeRef::named(format!("{}Connection", self.type_name())), move |ctx| { /* implementation */ }, ) .argument(arg_addr);
Line range hint
92-228: Consider optimizing SQL query construction for safety.Ohayo! The SQL query is constructed using string interpolation, which might introduce risks if table names or column names change.
Utilize parameterized queries or query builders provided by
sqlxto enhance safety and maintainability:let query = sqlx::query( r#" SELECT et.id, et.contract_address, et.from_address, et.to_address, et.amount, et.token_id, et.executed_at, t.name, t.symbol, t.decimals, c.contract_type FROM erc_transfers et JOIN tokens t ON et.token_id = t.id JOIN contracts c ON t.contract_address = c.contract_address WHERE (et.from_address = ? OR et.to_address = ?) -- Additional conditions ORDER BY et.id DESC LIMIT ? OFFSET ? "# ) .bind(felt_to_sql_string(&address)) .bind(felt_to_sql_string(&address)) .bind(limit) .bind(offset);
150-174: Simplify pagination logic for better readability.Sensei, the pagination logic involving
order_directionandlimitcalculations is quite complex. Simplifying this can improve code clarity and reduce potential errors.Consider refactoring pagination logic into a separate function or module, or leverage existing pagination utilities that handle these calculations.
Line range hint
230-302: Handle unknown contract types more gracefully.Ohayo, the current implementation logs a warning and skips unknown contract types. This might lead to incomplete data being returned to the client.
Consider including a fallback case to handle unknown contract types, perhaps by returning a minimal representation or an error message to inform the client:
_ => { let transfer_value = Value::Object(ValueMapping::from([ (Name::new("from"), Value::String(row.from_address)), (Name::new("to"), Value::String(row.to_address)), (Name::new("amount"), Value::String(row.amount)), (Name::new("type"), Value::String(row.contract_type.clone())), (Name::new("executedAt"), Value::String(row.executed_at)), (Name::new("tokenMetadata"), Value::Null), (Name::new("transactionHash"), Value::String(transaction_hash)), ])); edges.push(Value::Object(ValueMapping::from([ (Name::new("node"), transfer_value), (Name::new("cursor"), Value::String(cursor)), ]))); }This ensures that clients receive all relevant data, even if the contract type is unknown.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
crates/torii/graphql/src/object/erc/mod.rs (1)
1-2: Ohayo, sensei! These imports look great!The new imports for
cursorandCursorDirectionare well-placed and necessary for the new functionality. They're specific and don't introduce any unnecessary dependencies.crates/torii/graphql/src/constants.rs (1)
Line range hint
34-35: Ohayo again, sensei! ERC type name constants are on point!The new constants
ERC_BALANCE_TYPE_NAMEandERC_TRANSFER_TYPE_NAMEare well-integrated with the existing type name constants. They follow the established naming convention with double underscores, which is consistent with other type names in the file.crates/torii/graphql/src/object/erc/erc_balance.rs (1)
148-148: Verify the necessity of convertingaddressto a stringOhayo, sensei! When binding parameters in SQL queries, it's often preferable to pass them in their native types. Converting
addressto a string withfelt_to_sql_stringmay be unnecessary ifsqlxsupports theFelttype directly. This could improve performance and reduce potential errors.Would you like me to help check if binding
addressdirectly is supported?crates/torii/graphql/src/object/erc/erc_transfer.rs (1)
64-73: Verify the accuracy oftotal_countin queries.Sensei, please ensure that the
total_countaccurately reflects the number of matching records after applying filters. This is vital for correct pagination behavior.Run the following script to confirm
total_countmatches the expected value:
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
1e9a382 to
5a3612c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
53-86: Ohayo, sensei! Excellent work on updating theErcBalanceObjectimplementation!The changes you've made to support pagination and connection handling are well-implemented. The use of
ConnectionArgumentsand the updated query structure provide a robust foundation for paginated results.One small suggestion for improved readability:
Consider extracting the field creation logic into a separate method within the
ErcBalanceObjectimpl block. This would make theresolversmethod more concise and easier to understand at a glance.
Line range hint
210-272: Ohayo, sensei! Well done on implementing theerc_balance_connection_outputfunction!The function effectively formats the query results into the expected GraphQL connection structure, handling both ERC20 and ERC721 token types appropriately.
To further enhance this implementation:
- Consider extracting the token metadata formatting logic for ERC20 and ERC721 into separate helper functions. This would improve readability and make the main function more concise.
- The
warn!macro is used for unknown contract types, but it might be better to return aResultwith an error in this case, allowing the caller to handle the error appropriately.- Add error handling for the
token_id.split(':')operation to gracefully handle malformed token IDs.These changes will improve the robustness and maintainability of the function.
crates/torii/graphql/src/object/erc/erc_transfer.rs (2)
Line range hint
89-228: Ohayo, sensei! Impressive pagination implementation, but let's consider refactoring!The
fetch_erc_transfersfunction now robustly handles cursor-based pagination, which is fantastic! However, the function has become quite complex, which might make it harder to maintain in the future.Consider breaking down this function into smaller, more focused helper functions. For example:
- A function to construct the base query
- A function to handle cursor conditions
- A function to determine the limit and offset
- A function to process the query results and construct the
PageInfoThis refactoring would make the code more modular and easier to test. Here's a rough outline:
fn construct_base_query() -> String { ... } fn add_cursor_conditions(query: &mut String, connection: &ConnectionArguments) { ... } fn determine_limit_and_offset(connection: &ConnectionArguments) -> (u64, u64) { ... } fn process_query_results(data: Vec<SqliteRow>, connection: &ConnectionArguments) -> (Vec<SqliteRow>, PageInfo) { ... } async fn fetch_erc_transfers( conn: &mut SqliteConnection, address: Felt, connection: &ConnectionArguments, total_count: i64, ) -> sqlx::Result<(Vec<SqliteRow>, PageInfo)> { let mut query = construct_base_query(); add_cursor_conditions(&mut query, connection); let (limit, offset) = determine_limit_and_offset(connection); // Execute query... let data = sqlx::query(&query) // ... (bindings and execution) process_query_results(data, connection) }This structure would make the code more readable and maintainable. What do you think, sensei?
Line range hint
230-302: Ohayo, sensei! Excellent connection output implementation, but let's improve error handling!The
erc_transfer_connection_outputfunction does a great job of constructing the connection output structure and handling different token types. However, there's room for improvement in handling unknown contract types.Currently, when an unknown contract type is encountered, a warning is logged and the iteration continues. This silently skips the problematic data, which might lead to unexpected behavior or data loss.
Consider handling this case more explicitly. Here are a couple of options:
- Return an error instead of silently continuing:
_ => { return Err(sqlx::Error::RowNotFound); }
- Include the unknown type in the output with a special flag:
_ => { warn!("Unknown contract type: {}", row.contract_type); Value::Object(ValueMapping::from([ (Name::new("from"), Value::String(row.from_address)), (Name::new("to"), Value::String(row.to_address)), (Name::new("amount"), Value::String(row.amount)), (Name::new("type"), Value::String("unknown".to_string())), (Name::new("executedAt"), Value::String(row.executed_at)), (Name::new("tokenMetadata"), Value::Null), (Name::new("transactionHash"), Value::String(transaction_hash)), ])) }This way, we ensure that all data is accounted for, and unknown types can be handled appropriately by the client. What do you think, sensei?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/mod.rs
🧰 Additional context used
🔇 Additional comments (4)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
1-24: Ohayo, sensei! Great job on enhancing the imports and constants!The new imports and constants you've added are well-structured and provide a solid foundation for implementing pagination and connection handling. This change improves code organization and maintainability.
286-286: Ohayo, sensei! Good job on updating theBalanceQueryResultRawstruct!The addition of the
idfield to theBalanceQueryResultRawstruct is a necessary change that supports the new cursor-based pagination functionality. This update aligns well with the changes made in thefetch_erc_balances2function and enhances the overall consistency of the pagination implementation.crates/torii/graphql/src/object/erc/erc_transfer.rs (2)
1-1: Ohayo, sensei! New imports and constants look great!The added imports and constants are well-organized and provide the necessary components for implementing connection-based querying and pagination. They align perfectly with the new functionality being introduced.
Also applies to: 6-7, 13-21
Line range hint
1-334: Ohayo, sensei! Outstanding work on implementing connection-based querying and pagination!The changes in this file significantly enhance the ERC transfer functionality by introducing robust pagination and connection-based querying. The implementation aligns well with GraphQL best practices and provides a solid foundation for efficient data retrieval.
Key improvements:
- Comprehensive pagination support in
fetch_erc_transfers- Well-structured connection output in
erc_transfer_connection_output- Proper handling of different token types (ERC20 and ERC721)
Suggested enhancements:
- Improve error handling in the async closure of the
resolversmethod- Consider refactoring
fetch_erc_transfersfor better maintainability- Enhance error handling for unknown contract types in
erc_transfer_connection_outputOverall, this is a substantial improvement that will greatly benefit the GraphQL API's performance and flexibility. Great job, sensei!
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
5a3612c to
67fe85d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
53-87: Ohayo, sensei! Excellent work on updating theresolversmethod!The changes to return a connection type and incorporate pagination logic are well-implemented. The use of
parse_connection_argumentsis a good choice for handling pagination parameters.One small suggestion:
Consider extracting the query construction logic into a separate method to improve readability and maintainability. This would make the
resolversmethod more concise and easier to understand at a glance.
Line range hint
210-272: Ohayo, sensei! The newerc_balance_connection_outputfunction looks solid!This function does a great job of formatting the data into the expected connection output structure, handling both ERC20 and ERC721 token types appropriately.
To further improve the code, consider extracting the token metadata construction into a separate function. This would reduce repetition and make the code more DRY (Don't Repeat Yourself). For example:
fn create_token_metadata(row: &BalanceQueryResultRaw, is_erc20: bool) -> Value { let mut metadata = ValueMapping::from([ (Name::new("name"), Value::String(row.name.clone())), (Name::new("symbol"), Value::String(row.symbol.clone())), (Name::new("decimals"), Value::String(row.decimals.to_string())), (Name::new("contractAddress"), Value::String(row.contract_address.clone())), ]); if is_erc20 { metadata.insert(Name::new("tokenId"), Value::Null); } else { let token_id = row.token_id.split(':').collect::<Vec<&str>>(); metadata.insert(Name::new("tokenId"), Value::String(token_id[1].to_string())); } Value::Object(metadata) }This would make the main function more concise and easier to maintain.
crates/torii/graphql/src/object/erc/erc_transfer.rs (3)
Line range hint
89-228: Ohayo, sensei! Impressive work on pagination, but let's consider refactoring!The
fetch_erc_transfersfunction now handles various pagination scenarios comprehensively. However, its complexity has increased significantly, which might make it harder to maintain in the future.Consider splitting this function into smaller, more focused functions. For example:
- A function to build the base query
- A function to handle cursor-based pagination
- A function to handle offset-based pagination
This refactoring would improve readability and make the code easier to test and maintain.
What do you think about this approach, sensei?
Line range hint
230-302: Ohayo, sensei! Great job implementing the connection pattern, but let's improve error handling!The
erc_transfer_connection_outputfunction now correctly constructs aValueMappingthat includes edges and page information, aligning with the GraphQL connection pattern. However, there's a potential issue with how unknown token types are handled.Currently, when an unknown contract type is encountered, we log a warning and skip the item. This could lead to silent failures and incomplete data. Consider the following improvements:
- Instead of skipping unknown types, include them in the output with a special "unknown" type field.
- Alternatively, if skipping is intentional, consider logging more information about the skipped item for easier debugging.
For example:
_ => { warn!("Unknown contract type: {} for transfer ID: {}", row.contract_type, row.id); Value::Object(ValueMapping::from([ (Name::new("type"), Value::String("unknown".to_string())), (Name::new("contractType"), Value::String(row.contract_type)), // Include other fields as needed ])) }What are your thoughts on this approach, sensei?
Line range hint
1-338: Ohayo, sensei! Excellent work on implementing the Connection abstraction!Your implementation successfully achieves the PR objectives, introducing the
Connectionabstraction for ERC balance and transfer queries. The pagination logic is well-implemented throughout the file.To further improve the codebase, consider extracting common pagination logic into a separate module. This could include:
- A generic function for building paginated queries
- Utility functions for handling cursors and offsets
- A reusable structure for page info calculation
This refactoring would promote code reuse and make it easier to implement pagination for other types of queries in the future.
What are your thoughts on this suggestion, sensei?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/mod.rs
🧰 Additional context used
🔇 Additional comments (3)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
1-24: Ohayo, sensei! New imports and constants look great!The added imports and constants are well-chosen for implementing the Connection abstraction and pagination. They provide a solid foundation for the changes in this file.
286-286: Ohayo, sensei! Good addition to theBalanceQueryResultRawstruct!The new
idfield is a necessary change for supporting cursor-based pagination. This addition aligns well with the implementation of the Connection abstraction.crates/torii/graphql/src/object/erc/erc_transfer.rs (1)
1-21: Ohayo, sensei! Nice work on updating the imports and constants!The new imports and constants you've added are well-organized and clearly support the implementation of the connection-based query approach and pagination. This sets a solid foundation for the changes to come.
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
67fe85d to
83412df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/graphql/src/object/erc/erc_balance.rs (1)
Line range hint
210-272: Ohayo, sensei! The newerc_balance_connection_outputfunction looks great!This function effectively formats the output as a connection response, handling different token types (ERC20 and ERC721) appropriately. It correctly constructs the edges and page info for the connection.
One small suggestion: Consider using a
matchexpression instead ofif-elsefor handling different contract types. This would make it easier to add new token types in the future and provide better error handling for unknown types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/erc_transfer.rs
- crates/torii/graphql/src/object/erc/mod.rs
🧰 Additional context used
🔇 Additional comments (4)
crates/torii/graphql/src/object/erc/erc_balance.rs (4)
1-24: Ohayo, sensei! New imports and constants look good!The added imports and constants are well-aligned with the implementation of the
Connectionabstraction and pagination. They provide a solid foundation for the changes in the rest of the file.
53-87: Ohayo, sensei! Excellent work on updating theErcBalanceObjectimplementation!The changes to the
resolversmethod successfully implement theConnectionabstraction. The use ofparse_connection_argumentsand the new return type provide robust pagination support. This update aligns well with the PR objectives.
286-286: Ohayo, sensei! Good addition to theBalanceQueryResultRawstruct!The new
idfield is a necessary addition for supporting cursor-based pagination. This change aligns well with the implementation of theConnectionabstraction and enhances the overall functionality of the code.
89-208: 🛠️ Refactor suggestionOhayo, sensei! Great job on enhancing the pagination logic!
The updated
fetch_erc_balancesfunction now provides robust support for both cursor-based and offset-based pagination. The implementation is comprehensive and handles various edge cases well.However, the function name
fetch_erc_balances2doesn't clearly convey its new capabilities. Consider renaming it to something more descriptive, such asfetch_paginated_erc_balances.
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
83412df to
a5d7c4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
crates/torii/graphql/src/object/erc/mod.rs (1)
8-17: Ohayo again, sensei! Thehandle_cursorfunction looks solid!The implementation is clean and effectively handles cursor decoding for pagination. Great job on error handling and using
sqlx::Result!A small suggestion to consider:
Instead of using a generic "Invalid cursor format" error message, you could pass the specific error from
cursor::decode. This would provide more detailed information for debugging. Here's how you might modify the error case:- Err(_) => Err(sqlx::Error::Decode("Invalid cursor format".into())), + Err(e) => Err(sqlx::Error::Decode(format!("Invalid cursor format: {}", e).into())),This change would give more context about why the cursor decoding failed.
crates/torii/graphql/src/constants.rs (1)
12-13: Ohayo, sensei! New ERC-related constants look good!The addition of these constants for ERC balance and transfer tables and type names is a solid step towards implementing the
Connectionabstraction for ERC-related queries. They provide clear, centralized definitions for these important identifiers.A couple of observations:
- The
ERC_BALANCE_TABLEuses "balances" whileERC_TRANSFER_TABLEuses "erc_transfers". Consider if this naming inconsistency is intentional or if they should follow the same pattern.- The type names use double underscores (
__) which is a common convention for namespacing in GraphQL. Good choice!Consider adding comments to explain the purpose of these constants, especially if they're part of a new feature or significant change in the GraphQL schema.
Also applies to: 40-41
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
Line range hint
210-272: Handle Unknown Contract Types GracefullyOhayo, sensei! In the
erc_balance_connection_outputfunction, when an unknowncontract_typeis encountered, the code logs a warning and continues. This may result in missing entries in the response.Consider handling unknown contract types by providing a default case or including a minimal representation to avoid losing data.
_ => { warn!("Unknown contract type: {}", row.contract_type); let token_metadata = Value::Object(ValueMapping::from([ (Name::new("contractAddress"), Value::String(row.contract_address.clone())), (Name::new("name"), Value::String(row.name)), (Name::new("symbol"), Value::String(row.symbol)), // Include default or null values for other fields ])); edges.push(Value::Object(ValueMapping::from([ (Name::new("node"), Value::Object(ValueMapping::from([ (Name::new("balance"), Value::String(row.balance)), (Name::new("type"), Value::String(row.contract_type)), (Name::new("tokenMetadata"), token_metadata), ]))), (Name::new("cursor"), Value::String(cursor)), ]))); }
Line range hint
286-294: Add Documentation forBalanceQueryResultRawStructOhayo, sensei! The
BalanceQueryResultRawstruct is crucial for data mapping but lacks documentation. Adding comments can improve code understanding.Consider adding documentation comments:
/// Represents the raw result of an ERC balance query. #[derive(FromRow, Deserialize, Debug, Clone)] #[serde(rename_all = "camelCase")] struct BalanceQueryResultRaw { /// Unique identifier of the balance record. pub id: String, /// The contract address of the token. pub contract_address: String, /// The name of the token. pub name: String, /// The symbol of the token. pub symbol: String, /// The number of decimals the token uses. pub decimals: u8, /// The token ID (for NFTs). pub token_id: String, /// The balance of the token. pub balance: String, /// The type of the contract (e.g., "erc20", "erc721"). pub contract_type: String, }crates/torii/graphql/src/object/erc/erc_transfer.rs (1)
Line range hint
252-253: Ohayo, sensei! Avoid usingassert!macros in production codeUsing
assert!can cause the application to panic if the condition fails, which is undesirable in production environments. Instead, handle this scenario gracefully by returning a relevant error or logging a warning.Apply this diff to handle the error without panicking:
- assert!(token_id.len() == 2); + if token_id.len() != 2 { + warn!("Unexpected token_id format: {}", row.token_id); + continue; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
crates/torii/graphql/src/object/erc/mod.rs (1)
1-2: Ohayo, sensei! These imports look great!The new imports for
cursorandCursorDirectionare well-placed and necessary for the newhandle_cursorfunction. They contribute to better code organization and modularity.crates/torii/graphql/src/object/erc/erc_transfer.rs (1)
150-154: Ohayo, sensei! Verify thatorder_directionaligns with the intended paginationThe
order_directionis determined based on whetherfirstorlastis provided in theConnectionArguments. Ensure that this logic matches the desired pagination behavior, as an incorrect order might return results in an unexpected sequence.
| let mut query = format!( | ||
| "SELECT b.id, t.contract_address, t.name, t.symbol, t.decimals, b.balance, b.token_id, \ | ||
| c.contract_type | ||
| FROM {table_name} b | ||
| JOIN tokens t ON b.token_id = t.id | ||
| JOIN contracts c ON t.contract_address = c.contract_address | ||
| WHERE b.account_address = ?"; | ||
| JOIN contracts c ON t.contract_address = c.contract_address" | ||
| ); | ||
| let mut conditions = vec!["b.account_address = ?".to_string()]; |
There was a problem hiding this comment.
Ensure Safe SQL Query Construction to Prevent Injection Attacks
Ohayo, sensei! Constructing SQL queries using format! and string concatenation can introduce risks if any variables are user-controlled. Even though table_name and id_column are likely constants, it's a best practice to use parameterized queries or the query builders provided by sqlx to enhance security and prevent SQL injection attacks.
Consider modifying the query construction to use parameter binding instead of string interpolation. Here's an example:
let query = "
SELECT b.id, t.contract_address, t.name, t.symbol, t.decimals, b.balance, b.token_id, c.contract_type
FROM erc_balances b
JOIN tokens t ON b.token_id = t.id
JOIN contracts c ON t.contract_address = c.contract_address
WHERE b.account_address = ?";
let mut stmt = sqlx::query(&query).bind(felt_to_sql_string(&address));| if !data.is_empty() { | ||
| page_info.start_cursor = Some(cursor::encode( | ||
| &data[0].try_get::<String, &str>(ID_COLUMN)?, | ||
| &data[0].try_get_unchecked::<String, &str>(ID_COLUMN)?, | ||
| )); | ||
| page_info.end_cursor = Some(cursor::encode( | ||
| &data[data.len() - 1].try_get::<String, &str>(ID_COLUMN)?, | ||
| &data[data.len() - 1].try_get_unchecked::<String, &str>(ID_COLUMN)?, | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Cursor Encoding Logic for Clarity
Ohayo, sensei! The cursor encoding logic is repeated and could be simplified for better readability.
Consider extracting the cursor encoding into a helper function:
fn encode_cursor(row: &SqliteRow) -> sqlx::Result<String> {
Ok(cursor::encode(
&row.try_get::<String, &str>(ID_COLUMN)?,
&row.try_get_unchecked::<String, &str>(ID_COLUMN)?,
))
}Then update the code:
page_info.start_cursor = Some(encode_cursor(&data[0])?);
page_info.end_cursor = Some(encode_cursor(&data[data.len() - 1])?);| let order_direction = match (connection.first, connection.last) { | ||
| (Some(_), _) => Direction::Desc, | ||
| (_, Some(_)) => Direction::Asc, | ||
| _ => Direction::Desc, | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! After reviewing the implementation, it appears that order_direction is determined solely by first and last parameters without considering before and after. This may lead to inconsistencies with Relay's Cursor Connections Specification.
- Recommendation: Modify the
order_directionlogic to account forbeforeandafterparameters to ensure proper alignment with pagination behavior.
🔗 Analysis chain
Verify Pagination Direction Logic for Correctness
Ohayo, sensei! The determination of order_direction based on connection.first and connection.last might not align with expected pagination behaviors. Ensure that the ordering corresponds correctly to the first, last, before, and after parameters to comply with Relay's Cursor Connections Specification.
Here's a script to help verify the pagination logic:
Ohayo, sensei! To ensure the order_direction aligns correctly with all pagination parameters, let's examine how connection.before and connection.after are handled.
Ohayo, sensei! Let's ensure that order_direction is correctly influenced by both before and after pagination parameters to fully comply with Relay's specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that order_direction is correctly set based on pagination arguments.
# Test: Search for order_direction usage and related logic.
rg 'let order_direction.*' -A 10 crates/torii/graphql/src/object/erc/erc_balance.rs
Length of output: 481
Script:
#!/bin/bash
# Description: Verify handling of 'before' and 'after' pagination arguments.
rg 'connection\.(before|after)' crates/torii/graphql/src/object/erc/erc_balance.rs
Length of output: 279
Script:
#!/bin/bash
# Description: Check if 'order_direction' is modified or influenced by 'before' and 'after' parameters.
rg 'order_direction.*' crates/torii/graphql/src/object/erc/erc_balance.rs -A 10
Length of output: 703
| let mut cursor_param = &connection.after; | ||
| if let Some(after_cursor) = &connection.after { | ||
| conditions.push(handle_cursor(after_cursor, CursorDirection::After, ID_COLUMN)?); | ||
| } | ||
|
|
||
| if let Some(before_cursor) = &connection.before { | ||
| cursor_param = &connection.before; | ||
| conditions.push(handle_cursor(before_cursor, CursorDirection::Before, ID_COLUMN)?); | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Ensure correct handling when both after and before cursors are provided
Currently, if both after and before cursors are set, cursor_param is overwritten by connection.before, potentially ignoring the after cursor. This might lead to unexpected behavior in pagination. Consider validating that only one cursor is used at a time or implementing logic to handle both cursors appropriately.
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
a5d7c4a to
79572d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/torii/graphql/src/object/erc/erc_balance.rs (3)
53-82: Enhance error handling in the resolver.Ohayo, sensei! While the Connection implementation looks good, consider adding more specific error handling for pagination-related errors. Currently, errors are propagated directly, which might not provide clear feedback to API consumers.
Consider wrapping errors with context:
- Ok(Some(Value::Object(results))) + results.map_or_else( + |e| Err(format!("Failed to process ERC balance data: {}", e).into()), + |r| Ok(Some(Value::Object(r))) + )
122-133: Consider adding validation for pagination limits.Ohayo, sensei! The limit calculation logic could benefit from additional validation:
- Ensure
data_limitdoesn't exceed a maximum value to prevent resource exhaustion- Add validation for negative values in pagination parameters
Consider adding:
const MAX_LIMIT: i64 = 100; let data_limit = connection .first .or(connection.last) .or(connection.limit) .map(|l| l.min(MAX_LIMIT as u64)) .unwrap_or(DEFAULT_LIMIT);
Line range hint
216-265: Improve error handling for token_id parsing.Ohayo, sensei! The token_id parsing using
split(':').collect()followed by anassert!could be handled more gracefully:Consider this safer approach:
- let token_id = row.token_id.split(':').collect::<Vec<&str>>(); - assert!(token_id.len() == 2); + let token_id = row.token_id + .split(':') + .collect::<Vec<&str>>(); + if token_id.len() != 2 { + warn!("Invalid token_id format: {}", row.token_id); + continue; + }crates/torii/graphql/src/object/erc/erc_transfer.rs (2)
137-160: Ohayo, sensei! Consider adding documentation for the pagination strategy.The pagination logic handles both cursor-based and offset-based approaches with complex limit calculations. Adding documentation would help future maintainers understand:
- Why limit is increased by 1 or 2 items
- The relationship between cursor-based and offset-based pagination
- The impact of different combinations of first/last/limit parameters
Line range hint
237-295: Ohayo, sensei! Consider improving error handling for token_id parsing.The current implementation uses
assert!for token_id validation, which could cause panic in production. Consider using proper error handling:- let token_id = row.token_id.split(':').collect::<Vec<&str>>(); - assert!(token_id.len() == 2); + let token_id: Vec<&str> = row.token_id.split(':').collect(); + let token_id = token_id.get(1).ok_or_else(|| { + sqlx::Error::Protocol(format!( + "Invalid token_id format: {}. Expected format: contract_address:token_id", + row.token_id + )) + })?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/mod.rs
🧰 Additional context used
🔇 Additional comments (4)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
1-24: LGTM! Well-organized imports and constants.The new imports and constants are properly organized and necessary for implementing the Connection abstraction. The separation of constants into a dedicated module improves maintainability.
Line range hint
285-294: LGTM! Well-structured data model.Ohayo, sensei! The
BalanceQueryResultRawstruct is well-defined with proper serialization attributes and the newidfield supports cursor-based pagination effectively.crates/torii/graphql/src/object/erc/erc_transfer.rs (2)
1-21: LGTM! Well-organized imports for connection handling.The new imports and constants organization nicely supports the connection-based pagination implementation.
52-84: Ohayo, sensei! Excellent implementation of the connection-based field!The resolver now correctly implements the GraphQL connection pattern, supporting proper pagination through the
Connectiontype.
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
79572d8 to
ff46557
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/torii/graphql/src/object/erc/erc_transfer.rs (2)
Line range hint
98-120: Consider parameterizing the SQL query for better maintainability.Ohayo, sensei! While the SQL query is well-structured, consider extracting the column list into a constant or using a query builder to improve maintainability.
Example:
const TRANSFER_COLUMNS: &str = r#" et.id, et.contract_address, et.from_address, et.to_address, et.amount, et.token_id, et.executed_at, t.name, t.symbol, t.decimals, c.contract_type "#; let query = format!( "SELECT {columns} FROM {table_name} et ...", columns = TRANSFER_COLUMNS, table_name = table_name );
Line range hint
262-265: Add proper error handling for token ID parsing.Ohayo, sensei! The token ID splitting logic uses
assert!which could panic in production. Consider using proper error handling instead.- let token_id = row.token_id.split(':').collect::<Vec<&str>>(); - assert!(token_id.len() == 2); + let token_id = row.token_id + .split(':') + .collect::<Vec<&str>>(); + let token_id = token_id.get(1).ok_or_else(|| { + sqlx::Error::Protocol( + format!("Invalid token_id format: {}", row.token_id).into() + ) + })?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/erc_balance.rs
🔇 Additional comments (5)
crates/torii/graphql/src/object/erc/mod.rs (2)
1-2: LGTM! Clean and necessary imports.Ohayo! The imports are well-organized and directly support the new cursor handling functionality.
8-17: Verify SQL injection protection for id_column parameter.The implementation looks good, sensei! The cursor handling and error management are robust. However, let's verify that the
id_columnparameter is properly validated to prevent SQL injection.crates/torii/graphql/src/object/erc/erc_transfer.rs (3)
1-24: LGTM! Well-organized imports for connection implementation.The new imports and constants are properly organized and provide all necessary components for implementing the GraphQL connection pattern.
52-84: LGTM! Well-structured connection-based resolver implementation.The resolver correctly implements the GraphQL connection pattern with proper argument handling and pagination support.
180-183:⚠️ Potential issueReplace unsafe cursor access with safe alternatives.
Ohayo, sensei! The use of
try_get_uncheckedfor cursor encoding could lead to runtime panics. Consider using the safetry_getalternative with proper error handling.- &data[0].try_get_unchecked::<String, &str>(ID_COLUMN)?, + &data[0].try_get::<String, &str>(ID_COLUMN)?Likely invalid or redundant comment.
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
ff46557 to
1a61255
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
89-208: Add documentation for the complex pagination logic.Ohayo, sensei! The pagination implementation is robust but complex. Consider adding documentation that explains:
- The relationship between
first,last, and cursor parameters- The logic behind the limit adjustments (+1 or +2)
- The cursor-based vs. offset-based pagination behavior
Example documentation format:
/// Fetches ERC balances with pagination support. /// /// # Pagination Behavior /// - When using cursors: Fetches n+2 items (n=limit) to determine has_previous/has_next /// - When using offset: Uses the total_count to determine pagination info /// /// # Parameters /// - connection: Pagination parameters (first/last/before/after/offset) /// - total_count: Total number of records (used for offset-based pagination)
Line range hint
210-272: Consider using custom error types for better error handling.Ohayo, sensei! The function currently uses
sqlx::Result, but consider creating custom error types to handle domain-specific errors like:
- Invalid token types
- Malformed token IDs
- Missing required fields
Example implementation:
#[derive(Debug, thiserror::Error)] pub enum ErcBalanceError { #[error("Invalid token type: {0}")] InvalidTokenType(String), #[error("Malformed token ID: {0}")] MalformedTokenId(String), #[error(transparent)] SqlxError(#[from] sqlx::Error), } type Result<T> = std::result::Result<T, ErcBalanceError>;crates/torii/graphql/src/object/erc/erc_transfer.rs (3)
Line range hint
89-228: Ohayo, sensei! Consider breaking down this large function.The
fetch_erc_transfersfunction is quite long (140 lines) and handles multiple concerns: query building, cursor handling, and page info calculation. This makes it harder to maintain and test.Consider breaking it down into smaller, focused functions:
async fn fetch_erc_transfers( conn: &mut SqliteConnection, address: Felt, connection: &ConnectionArguments, total_count: i64, ) -> sqlx::Result<(Vec<SqliteRow>, PageInfo)> { let query = build_erc_transfers_query(address, connection)?; let data = execute_query(conn, &query, address).await?; let page_info = calculate_page_info(&data, connection, total_count)?; Ok((data, page_info)) } fn build_erc_transfers_query( address: Felt, connection: &ConnectionArguments, ) -> sqlx::Result<String> { // Query building logic } async fn execute_query( conn: &mut SqliteConnection, query: &str, address: Felt, ) -> sqlx::Result<Vec<SqliteRow>> { // Query execution logic } fn calculate_page_info( data: &[SqliteRow], connection: &ConnectionArguments, total_count: i64, ) -> sqlx::Result<PageInfo> { // Page info calculation logic }
Line range hint
262-267: Ohayo, sensei! Improve token_id parsing safety.The token_id parsing using string split and array access could panic if the format is unexpected.
Consider using a safer parsing approach:
let token_id = row.token_id .split(':') .nth(1) .ok_or_else(|| sqlx::Error::RowNotFound)? .to_string(); let token_metadata = Value::Object(ValueMapping::from([ (Name::new("name"), Value::String(row.name)), (Name::new("symbol"), Value::String(row.symbol)), (Name::new("tokenId"), Value::String(token_id)), // ... rest of the fields ]));
Line range hint
285-289: Ohayo, sensei! Enhance error handling for unknown contract types.Currently, unknown contract types are just logged and skipped. This could lead to silent failures and missing data in the response.
Consider propagating the error to the caller:
_ => { return Err(sqlx::Error::RowNotFound).with_context(|| format!("Unsupported contract type: {}", row.contract_type) ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/graphql/src/constants.rs (1 hunks)
- crates/torii/graphql/src/object/erc/erc_balance.rs (4 hunks)
- crates/torii/graphql/src/object/erc/erc_transfer.rs (4 hunks)
- crates/torii/graphql/src/object/erc/mod.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/mod.rs
🔇 Additional comments (3)
crates/torii/graphql/src/object/erc/erc_balance.rs (3)
1-24: LGTM! Well-organized imports for pagination support.The new imports and constants are properly organized and necessary for implementing the Connection abstraction.
Line range hint
285-294: LGTM! Well-structured query result type.The
BalanceQueryResultRawstruct is well-organized with proper field types and derives the necessary traits.
53-85: Consider enhancing error handling in the resolver.Ohayo, sensei! While the Connection implementation looks good, consider adding more specific error handling for pagination-related errors. For example:
- Invalid cursor format
- Out-of-range pagination values
- Conflicting pagination parameters (e.g., both first and last specified)
Let's verify the error handling coverage:
| let table_name = ERC_TRANSFER_TABLE; | ||
| let id_column = format!("et.{}", ID_COLUMN); |
There was a problem hiding this comment.
Ohayo, sensei! Protect against SQL injection in table name interpolation.
The table name is directly interpolated into the SQL query string. While the table name comes from a constant in this case, it's better to use proper SQL parameter binding for all dynamic values as a best practice.
Consider using a prepared statement or query builder that properly handles table name escaping:
let query = sqlx::query_builder::QueryBuilder::new()
.push("SELECT et.id, et.contract_address, et.from_address, et.to_address, ")
.push("et.amount, et.token_id, et.executed_at, t.name, t.symbol, t.decimals, ")
.push("c.contract_type FROM ")
.push_quoted(table_name) // This will properly escape the table name
.push(" et JOIN tokens t ON et.token_id = t.id ")
.push("JOIN contracts c ON t.contract_address = c.contract_address");Also applies to: 98-117
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
1a61255 to
d5ed9fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
53-85: Consider enhancing error handling in the resolver.Ohayo, sensei! While the Connection implementation looks good, consider adding more descriptive error messages when:
- Database connection fails
- Argument parsing fails
- Count query fails
Example enhancement:
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?; +let mut conn = ctx.data::<Pool<Sqlite>>() + .map_err(|e| format!("Failed to get database pool: {}", e))? + .acquire() + .await + .map_err(|e| format!("Failed to acquire connection: {}", e))?;
Line range hint
210-272: Enhance type safety in token type handling.Ohayo, sensei! The token type handling could be more type-safe and maintainable.
Consider using an enum for contract types:
#[derive(Debug, PartialEq)] enum ContractType { ERC20, ERC721, Unknown(String), } impl From<String> for ContractType { fn from(s: String) -> Self { match s.to_lowercase().as_str() { "erc20" => ContractType::ERC20, "erc721" => ContractType::ERC721, other => ContractType::Unknown(other.to_string()), } } }Then update the match statement:
-match row.contract_type.to_lowercase().as_str() { - "erc20" => { +match ContractType::from(row.contract_type.clone()) { + ContractType::ERC20 => {crates/torii/graphql/src/object/erc/erc_transfer.rs (1)
Line range hint
241-291: Ohayo, sensei! Consider using an enum for token types.The current string-based token type handling could be more maintainable using an enum.
#[derive(Debug, PartialEq)] enum TokenType { ERC20, ERC721, Unknown(String), } impl From<String> for TokenType { fn from(s: String) -> Self { match s.to_lowercase().as_str() { "erc20" => TokenType::ERC20, "erc721" => TokenType::ERC721, other => TokenType::Unknown(other.to_string()), } } }Then use it in the match statement:
- match row.contract_type.to_lowercase().as_str() { - "erc20" => { + match TokenType::from(row.contract_type.clone()) { + TokenType::ERC20 => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/torii/graphql/src/constants.rs(1 hunks)crates/torii/graphql/src/object/erc/erc_balance.rs(4 hunks)crates/torii/graphql/src/object/erc/erc_transfer.rs(4 hunks)crates/torii/graphql/src/object/erc/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/constants.rs
- crates/torii/graphql/src/object/erc/mod.rs
🔇 Additional comments (2)
crates/torii/graphql/src/object/erc/erc_balance.rs (2)
1-24: LGTM! Well-organized imports and constants.
The new imports and constants are properly organized, making the code more maintainable.
286-286: LGTM! Necessary field addition for cursor-based pagination.
Ohayo, sensei! The addition of the id field is well-placed and essential for the cursor-based pagination implementation.
| if data.is_empty() { | ||
| Ok((data, page_info)) | ||
| } else if is_cursor_based { | ||
| match cursor_param { | ||
| Some(cursor_query) => { | ||
| let first_cursor = cursor::encode( | ||
| &data[0].try_get::<String, &str>(&id_column)?, | ||
| &data[0].try_get_unchecked::<String, &str>(&id_column)?, | ||
| ); | ||
|
|
||
| if &first_cursor == cursor_query && data.len() != 1 { | ||
| data.remove(0); | ||
| page_info.has_previous_page = true; | ||
| } else { | ||
| data.pop(); | ||
| } | ||
|
|
||
| if data.len() as u64 == limit - 1 { | ||
| page_info.has_next_page = true; | ||
| data.pop(); | ||
| } | ||
| } | ||
| None => { | ||
| if data.len() as u64 == limit { | ||
| page_info.has_next_page = true; | ||
| data.pop(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract cursor-based pagination logic into a helper function.
Ohayo, sensei! The cursor-based pagination logic is complex and could benefit from being extracted into a dedicated helper function.
Consider creating a helper function:
fn handle_cursor_based_pagination(
data: &mut Vec<SqliteRow>,
cursor_param: &Option<String>,
limit: u64,
page_info: &mut PageInfo,
) -> sqlx::Result<()> {
match cursor_param {
Some(cursor_query) => {
let first_cursor = cursor::encode(
&data[0].try_get::<String, &str>(&id_column)?,
&data[0].try_get_unchecked::<String, &str>(&id_column)?,
);
if &first_cursor == cursor_query && data.len() != 1 {
data.remove(0);
page_info.has_previous_page = true;
} else {
data.pop();
}
if data.len() as u64 == limit - 1 {
page_info.has_next_page = true;
data.pop();
}
}
None => {
if data.len() as u64 == limit {
page_info.has_next_page = true;
data.pop();
}
}
}
Ok(())
}| let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some(); | ||
|
|
||
| let data_limit = | ||
| connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT); | ||
| let limit = if is_cursor_based { | ||
| match &cursor_param { | ||
| Some(_) => data_limit + 2, | ||
| None => data_limit + 1, // prev page does not exist | ||
| } | ||
| } else { | ||
| data_limit | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify limit calculation logic.
Ohayo, sensei! The limit calculation logic could be simplified using a more declarative approach.
Consider this refactoring:
-let data_limit =
- connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT);
-let limit = if is_cursor_based {
- match &cursor_param {
- Some(_) => data_limit + 2,
- None => data_limit + 1, // prev page does not exist
- }
-} else {
- data_limit
-};
+let data_limit = connection.first
+ .or(connection.last)
+ .or(connection.limit)
+ .unwrap_or(DEFAULT_LIMIT);
+let limit = data_limit + match (is_cursor_based, cursor_param) {
+ (true, Some(_)) => 2,
+ (true, None) => 1,
+ (false, _) => 0,
+};📝 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.
| let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some(); | |
| let data_limit = | |
| connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT); | |
| let limit = if is_cursor_based { | |
| match &cursor_param { | |
| Some(_) => data_limit + 2, | |
| None => data_limit + 1, // prev page does not exist | |
| } | |
| } else { | |
| data_limit | |
| }; | |
| let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some(); | |
| let data_limit = connection.first | |
| .or(connection.last) | |
| .or(connection.limit) | |
| .unwrap_or(DEFAULT_LIMIT); | |
| let limit = data_limit + match (is_cursor_based, cursor_param) { | |
| (true, Some(_)) => 2, | |
| (true, None) => 1, | |
| (false, _) => 0, | |
| }; |
| let total_count: (i64,) = sqlx::query_as( | ||
| "SELECT COUNT(*) FROM erc_transfers WHERE from_address = ? OR to_address \ | ||
| = ?", | ||
| ) | ||
| .bind(felt_to_sql_string(&address)) | ||
| .bind(felt_to_sql_string(&address)) | ||
| .fetch_one(&mut *conn) | ||
| .await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider optimizing the count query performance.
The current count query might be inefficient for large datasets. Consider using an indexed column or approximate count for better performance.
- let total_count: (i64,) = sqlx::query_as(
- "SELECT COUNT(*) FROM erc_transfers WHERE from_address = ? OR to_address \
- = ?",
- )
+ let total_count: (i64,) = sqlx::query_as(
+ "SELECT COUNT(*) FROM erc_transfers USE INDEX (idx_addresses)
+ WHERE from_address = ? OR to_address = ?",
+ )Committable suggestion was skipped due to low confidence.
| let row = TransferQueryResultRaw::from_row(row)?; | ||
| let transaction_hash = get_transaction_hash_from_event_id(&row.id); | ||
| let cursor = cursor::encode(&row.id, &row.id); |
There was a problem hiding this comment.
Ohayo, sensei! Handle potential transaction hash generation failures.
The get_transaction_hash_from_event_id call could fail silently. Consider proper error handling.
- let transaction_hash = get_transaction_hash_from_event_id(&row.id);
+ let transaction_hash = get_transaction_hash_from_event_id(&row.id)
+ .map_err(|e| {
+ sqlx::Error::Protocol(format!(
+ "Failed to generate transaction hash for event {}: {}",
+ row.id, e
+ ).into())
+ })?;📝 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.
| let row = TransferQueryResultRaw::from_row(row)?; | |
| let transaction_hash = get_transaction_hash_from_event_id(&row.id); | |
| let cursor = cursor::encode(&row.id, &row.id); | |
| let row = TransferQueryResultRaw::from_row(row)?; | |
| let transaction_hash = get_transaction_hash_from_event_id(&row.id) | |
| .map_err(|e| { | |
| sqlx::Error::Protocol(format!( | |
| "Failed to generate transaction hash for event {}: {}", | |
| row.id, e | |
| ).into()) | |
| })?; | |
| let cursor = cursor::encode(&row.id, &row.id); |
| let data_limit = | ||
| connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT); | ||
| let limit = if is_cursor_based { | ||
| match &cursor_param { | ||
| Some(_) => data_limit + 2, | ||
| None => data_limit + 1, // prev page does not exist | ||
| } | ||
| } else { | ||
| data_limit | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Simplify the limit calculation logic.
The current limit calculation can be simplified using pattern matching.
- let data_limit =
- connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT);
- let limit = if is_cursor_based {
- match &cursor_param {
- Some(_) => data_limit + 2,
- None => data_limit + 1, // prev page does not exist
- }
- } else {
- data_limit
- };
+ let data_limit = connection.first
+ .or(connection.last)
+ .or(connection.limit)
+ .unwrap_or(DEFAULT_LIMIT);
+ let limit = match (is_cursor_based, &cursor_param) {
+ (true, Some(_)) => data_limit + 2,
+ (true, None) => data_limit + 1,
+ (false, _) => data_limit,
+ };📝 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.
| let data_limit = | |
| connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT); | |
| let limit = if is_cursor_based { | |
| match &cursor_param { | |
| Some(_) => data_limit + 2, | |
| None => data_limit + 1, // prev page does not exist | |
| } | |
| } else { | |
| data_limit | |
| }; | |
| let data_limit = connection.first | |
| .or(connection.last) | |
| .or(connection.limit) | |
| .unwrap_or(DEFAULT_LIMIT); | |
| let limit = match (is_cursor_based, &cursor_param) { | |
| (true, Some(_)) => data_limit + 2, | |
| (true, None) => data_limit + 1, | |
| (false, _) => data_limit, | |
| }; |
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
d5ed9fd to
db005c1
Compare
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
2 times, most recently
from
37a667f to
df6e12a
Compare
…ercBalance and ercTransfer query commit-id:bc10539f
lambda-0x
force-pushed
the
spr/main/bc10539f
branch
from
df6e12a to
d50e51b
Compare
|
✓ Commit merged in pull request #2689 |
Stack:
Connectionabstraction to return data for ercBalance and ercTransfer query #2527 ⬅