feat(torii-grpc): erc tokens and balances #2698
Conversation
WalkthroughOhayo, sensei! This pull request introduces new public structs Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)crates/torii/grpc/src/server/mod.rs (2)
The addition of
The implementations correctly map fields from 🪧 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
🧹 Outside diff range and nitpick comments (2)
crates/torii/core/src/types.rs (2)
125-133: Ohayo! Consider adding timestamp fields for consistencyThe
ContractMetadatastruct follows good practices with appropriate derives and camelCase serialization. However, unlike other entities in this file, it's missingexecuted_atandcreated_attimestamp fields which might be useful for tracking when the metadata was added or updated.Consider adding these fields:
pub struct ContractMetadata { pub id: String, pub contract_address: String, pub name: String, pub symbol: String, pub decimals: u8, + pub created_at: DateTime<Utc>, + pub executed_at: DateTime<Utc>, }
127-143: Consider separating ERC20 and ERC721 concernsThe current design combines both ERC20 and ERC721 token handling in the same structs. While
ContractTypeenum differentiates between them, the data structures might benefit from clearer separation.Consider either:
- Creating separate structs for each token standard
- Adding a token_type field to explicitly indicate which standard the metadata/balance refers to
- Using different tables/models for ERC20 and ERC721 tokens
This would make the code more maintainable and prevent potential confusion when handling different token standards.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/core/src/types.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/core/src/types.rs (1)
135-143: 🛠️ Refactor suggestion
Ohayo sensei! Consider using a more precise type for balance
The Balance struct looks good overall, but storing the balance as a String might not be optimal for numerical operations and could lead to precision issues.
Consider using a more appropriate type for balance handling. For ERC20 tokens, you might want to use a numeric type that can handle large numbers precisely.
Let's verify the balance usage in the codebase:
Also, consider adding validation for addresses:
pub struct Balance {
pub id: String,
- pub balance: String,
+ pub balance: U256, // or another appropriate numeric type
pub account_address: String,
pub contract_address: String,
pub token_id: String,
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/grpc/proto/types.proto (1)
161-165: Consider using appropriate numeric types for balance field, sensei!The
balancefield is defined as a string, which might lead to precision loss or unnecessary string-number conversions. For token balances, considering using a numeric type likeuint256(if supported by your implementation) or at minimum documenting that the string should contain a decimal number.Additionally, the
token_idfield suggests ERC-1155 support. If this is intended for both ERC-20 and ERC-1155, consider documenting this or splitting into separate message types for clarity.Consider this structure:
message Balance { - string balance = 1; + // Decimal string representation of the token balance + string balance = 1; // TODO: Consider uint256 when available string account_address = 2; string contract_address = 3; + // Optional: Present only for ERC-1155 tokens string token_id = 4; }crates/torii/grpc/proto/world.proto (2)
50-54: Enhance documentation for better clarity, sensei!While the structure is good, consider enhancing the documentation with:
- Format/length expectations for contract addresses
- Maximum number of addresses that can be requested (if any)
- Example usage
Example documentation improvement:
// A request to retrieve tokens message RetrieveTokensRequest { - // The list of contract addresses to retrieve tokens for + // The list of contract addresses to retrieve tokens for. + // Each address should be a valid ERC token contract address in bytes format. + // Example: 0x1234...5678 repeated bytes contract_addresses = 1; }
56-58: Consider adding pagination support, sensei!For scalability, consider adding pagination support to handle large token lists efficiently. This aligns with the pattern seen in
RetrieveEntitiesResponsewhich includes atotal_countfield.Example enhancement:
// A response containing tokens message RetrieveTokensResponse { repeated types.Token tokens = 1; + // Total number of tokens available + uint32 total_count = 2; + // Pagination cursor for subsequent requests + optional string next_cursor = 3; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/proto/world.proto(1 hunks)
🔇 Additional comments (2)
crates/torii/grpc/proto/types.proto (1)
154-159: Ohayo! Field numbering starts from 2 in Token message.
The field numbers in the Token message start from 2 instead of 1. While this works, it's unconventional and might indicate a missing field.
Consider adjusting the field numbers to start from 1:
message Token {
- string contract_address = 2;
- string name = 3;
- string symbol = 4;
- uint32 decimals = 5;
+ string contract_address = 1;
+ string name = 2;
+ string symbol = 3;
+ uint32 decimals = 4;
}crates/torii/grpc/proto/world.proto (1)
45-48: Ohayo! The new RPC method looks good, sensei!
The RetrieveTokens method follows the established pattern of other retrieval methods in the service, maintaining consistency in the API design.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
crates/torii/grpc/proto/types.proto (1)
162-166: Consider using appropriate numeric types for balance fields, sensei!The
balancefield is defined as a string, but if it represents a numeric value, consider:
- For regular ERC20 tokens: Using
uint64orstring(if values exceed 64 bits)- For ERC721/ERC1155: Using
uint64for token_idExample improvement:
message Balance { - string balance = 1; + uint64 balance = 1; string account_address = 2; string contract_address = 3; - string token_id = 4; + uint64 token_id = 4; }Note: If you need to handle arbitrary-precision numbers (like in some DeFi applications), keeping it as string is appropriate.
crates/torii/core/src/types.rs (2)
125-134: Ohayo! Consider enhancing the Token struct's type safety and documentation.The Token struct looks good overall, but could benefit from some improvements:
- The
metadatafield as a plain String might be too flexible. Consider using a structured type or JSON validation.- The
decimalsfield could benefit from value range validation.Consider this enhancement:
+use serde_json::Value; + #[derive(FromRow, Deserialize, Debug, Clone)] #[serde(rename_all = "camelCase")] pub struct Token { pub id: String, pub contract_address: String, pub name: String, pub symbol: String, pub decimals: u8, - pub metadata: String, + pub metadata: Value, }Also consider adding documentation comments to describe the purpose of each field, sensei!
125-144: Consider adding database indices for performance.Both Token and Balance structs will likely be queried frequently. Consider adding database indices for common query patterns.
Recommended indices:
- Token: contract_address
- Balance: (account_address, contract_address) composite index
Would you like me to provide the SQL migration script for these indices, sensei?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/torii/core/src/types.rs(1 hunks)crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(4 hunks)crates/torii/grpc/src/types/mod.rs(1 hunks)
🔇 Additional comments (3)
crates/torii/grpc/proto/types.proto (1)
154-160: Ohayo! Field number 1 is missing in Token message.
The field numbers start from 2, which suggests a removed field. If this is intentional, consider adding a comment explaining why field 1 is reserved. If not, consider reordering the field numbers sequentially.
crates/torii/core/src/types.rs (1)
136-144: 🛠️ Refactor suggestion
Consider using a more precise numeric type for balance.
The Balance struct's balance field is currently a String, which might not be optimal for numeric operations and validation.
Consider using a more appropriate type for handling token balances:
+use ethers::types::U256;
+
#[derive(FromRow, Deserialize, Debug, Clone)]
#[serde(rename_all = "camelCase")]
pub struct Balance {
pub id: String,
- pub balance: String,
+ pub balance: U256,
pub account_address: String,
pub contract_address: String,
pub token_id: String,
}This change would:
- Ensure numeric validity
- Support large numbers safely
- Enable direct mathematical operations
Let's verify if the balance field is used in calculations elsewhere:
crates/torii/grpc/src/types/mod.rs (1)
13-13: LGTM! Clean import statement
Ohayo! The import of Token from torii_core::types is well-placed among other imports.
crates/torii/grpc/src/types/mod.rs
Outdated
| impl From<Token> for proto::types::Token { | ||
| fn from(value: Token) -> Self { | ||
| Self { | ||
| contract_address: value.contract_address, | ||
| name: value.name, | ||
| symbol: value.symbol, | ||
| decimals: value.decimals as u32, | ||
| metadata: serde_json::to_string(&value.metadata).unwrap(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Handle serialization errors gracefully in Token conversion
Ohayo sensei! Looking at the codebase, I can confirm this is a critical issue. The serde_json::to_string(&value.metadata).unwrap() call in the Token conversion could indeed panic. While there are other unwrap usages in the codebase, this one is particularly risky since it's in a type conversion that could be called frequently in production code.
Here's a suggested improvement:
impl From<Token> for proto::types::Token {
fn from(value: Token) -> Self {
+ // Serialize metadata, using empty JSON object as fallback
+ let metadata = serde_json::to_string(&value.metadata)
+ .unwrap_or_else(|_| "{}".to_string());
+
Self {
contract_address: value.contract_address,
name: value.name,
symbol: value.symbol,
decimals: value.decimals as u32,
- metadata: serde_json::to_string(&value.metadata).unwrap(),
+ metadata,
}
}
}The change ensures that even if metadata serialization fails, we'll get a valid JSON object rather than a panic. This is a safer approach for production code.
🔗 Analysis chain
Handle potential serialization errors gracefully
Ohayo sensei! While the implementation is generally good, the unwrap() call on line 27 could panic if the metadata serialization fails. Consider handling this error case gracefully.
Here's a suggested improvement:
impl From<Token> for proto::types::Token {
fn from(value: Token) -> Self {
+ // Serialize metadata, using empty JSON object as fallback
+ let metadata = serde_json::to_string(&value.metadata)
+ .unwrap_or_else(|_| "{}".to_string());
+
Self {
contract_address: value.contract_address,
name: value.name,
symbol: value.symbol,
decimals: value.decimals as u32,
- metadata: serde_json::to_string(&value.metadata).unwrap(),
+ metadata,
}
}
}Let's verify if there are similar unwrap patterns in other type conversions:
Let me search for similar patterns in the codebase to verify if this is a common issue or an isolated case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potentially dangerous unwrap calls in type conversions
rg 'unwrap\(\)' --type rust
Length of output: 192327
Script:
#!/bin/bash
# Search for serde_json::to_string unwrap patterns in type conversions
rg 'serde_json::to_string\(.*\)\.unwrap\(\)' --type rust
# Search for error handling patterns in similar type conversions
rg 'impl From<.*> for proto::types::.*' --type rust -A 10
Length of output: 13698
| async fn retrieve_tokens( | ||
| &self, | ||
| request: Request<RetrieveTokensRequest>, | ||
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | ||
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | ||
| let contract_addresses = contract_addresses | ||
| .iter() | ||
| .map(|address| Felt::from_bytes_be_slice(address)) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let tokens = self | ||
| .retrieve_tokens(contract_addresses) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
| Ok(Response::new(tokens)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance error handling for the gRPC endpoint.
The implementation needs better input validation and error handling:
- No validation of input contract addresses
- Silent failures during Felt conversion
Consider this improved implementation:
async fn retrieve_tokens(
&self,
request: Request<RetrieveTokensRequest>,
) -> Result<Response<RetrieveTokensResponse>, Status> {
let RetrieveTokensRequest { contract_addresses } = request.into_inner();
+ if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+ }
+
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let tokens = self
.retrieve_tokens(contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;
Ok(Response::new(tokens))
}📝 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.
| async fn retrieve_tokens( | |
| &self, | |
| request: Request<RetrieveTokensRequest>, | |
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| Felt::from_bytes_be_slice(address)) | |
| .collect::<Vec<_>>(); | |
| let tokens = self | |
| .retrieve_tokens(contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(tokens)) | |
| } | |
| async fn retrieve_tokens( | |
| &self, | |
| request: Request<RetrieveTokensRequest>, | |
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| if contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("No contract addresses provided")); | |
| } | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let tokens = self | |
| .retrieve_tokens(contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(tokens)) | |
| } |
crates/torii/grpc/src/server/mod.rs
Outdated
| async fn retrieve_tokens( | ||
| &self, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokensResponse, Status> { | ||
| let query = format!( | ||
| "SELECT * FROM tokens WHERE contract_address IN ({})", | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); | ||
|
|
||
| let tokens: Vec<Token> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | ||
| Ok(RetrieveTokensResponse { tokens }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Please address potential SQL injection and performance concerns.
The current implementation has several issues that need attention:
- SQL Injection vulnerability: Using
format!for SQL query construction is unsafe. - No limit on the number of contract addresses that can be queried.
Consider this safer implementation:
async fn retrieve_tokens(
&self,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokensResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let placeholders = contract_addresses
+ .iter()
+ .map(|_| "?")
+ .collect::<Vec<_>>()
+ .join(", ");
+
+ let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders);
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
- let query = format!(
- "SELECT * FROM tokens WHERE contract_address IN ({})",
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- );
-
- let tokens: Vec<Token> = sqlx::query_as(&query)
+ let tokens: Vec<Token> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let tokens = tokens.iter().map(|token| token.clone().into()).collect();
Ok(RetrieveTokensResponse { tokens })
}📝 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.
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| let query = format!( | |
| "SELECT * FROM tokens WHERE contract_address IN ({})", | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| ); | |
| let tokens: Vec<Token> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } | |
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| let placeholders = contract_addresses | |
| .iter() | |
| .map(|_| "?") | |
| .collect::<Vec<_>>() | |
| .join(", "); | |
| let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let tokens: Vec<Token> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2698 +/- ##
==========================================
- Coverage 56.34% 56.16% -0.19%
==========================================
Files 411 411
Lines 52742 52947 +205
==========================================
+ Hits 29719 29737 +18
- Misses 23023 23210 +187 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/torii/grpc/proto/world.proto (2)
59-62: Consider adding pagination support for large result setsThe response messages don't include pagination support. For endpoints that could return large sets of tokens or balances, this might lead to performance issues. Consider adding:
uint32 limitstring cursorbool has_moreExample addition:
message RetrieveTokensResponse { repeated types.Token tokens = 1; + string cursor = 2; + bool has_more = 3; }Also applies to: 72-74
64-70: Consider enhancing filtering capabilitiesThe request only allows filtering by account and contract addresses. Consider adding optional fields for more advanced filtering:
- Token metadata fields
- Balance ranges
- Last updated timestamp
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 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(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/grpc/proto/types.proto
🔇 Additional comments (4)
crates/torii/grpc/proto/world.proto (2)
45-51: Ohayo sensei! The new RPC method definitions look solid!
The new methods RetrieveTokens and RetrieveTokenBalances are well-structured and follow the service's existing patterns. The methods are appropriately placed and documented.
53-57: Consider adding address length validation, sensei!
The contract_addresses field uses bytes type without length validation. For Ethereum-like addresses, consider adding a comment specifying the expected length (20 bytes) or using a custom type with validation.
crates/torii/grpc/src/server/mod.rs (2)
790-810: Ohayo sensei! The SQL injection concerns from the previous review are still valid.
The implementation needs to be updated to use parameterized queries and address the performance concerns as mentioned in the previous review.
1196-1211: Ohayo sensei! The error handling concerns from the previous review are still valid.
The implementation needs better input validation and error handling as mentioned in the previous review.
crates/torii/grpc/src/server/mod.rs
Outdated
| async fn retrieve_balances( | ||
| &self, | ||
| account_address: Vec<Felt>, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveBalancesResponse, Status> { | ||
|
|
||
| } |
There was a problem hiding this comment.
Ohayo sensei! The retrieve_balances method is incomplete.
The method signature is defined but the implementation is missing. This could lead to runtime errors.
Would you like assistance in implementing this method?
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/core/src/types.rs(1 hunks)crates/torii/grpc/src/server/mod.rs(4 hunks)crates/torii/grpc/src/types/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/core/src/types.rs
- crates/torii/grpc/src/types/mod.rs
🔇 Additional comments (2)
crates/torii/grpc/src/server/mod.rs (2)
1216-1232: Previous review comment is still valid.
790-811:
Ohayo sensei! Please address SQL injection and performance concerns.
The current implementation has several security and performance issues:
- SQL Injection vulnerability: Using string formatting for SQL query construction
- No validation of input parameters
- No limit on number of addresses that can be queried
Consider this safer implementation:
async fn retrieve_tokens(
&self,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokensResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+ }
+ if contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let placeholders = contract_addresses
+ .iter()
+ .map(|_| "?")
+ .collect::<Vec<_>>()
+ .join(", ");
+
+ let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders);
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
- let query = format!(
- "SELECT * FROM tokens WHERE contract_address IN ({})",
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- );
-
- let tokens: Vec<Token> = sqlx::query_as(&query)
+ let tokens: Vec<Token> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let tokens = tokens.iter().map(|token| token.clone().into()).collect();
Ok(RetrieveTokensResponse { tokens })
}Likely invalid or redundant comment.
| async fn retrieve_token_balances( | ||
| &self, | ||
| request: Request<RetrieveTokenBalancesRequest>, | ||
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | ||
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | ||
| 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 balances = self | ||
| .retrieve_token_balances(account_addresses, contract_addresses) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
| Ok(Response::new(balances)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Enhance error handling for address conversions.
The implementation needs better error handling for Felt conversions:
Consider this improved implementation:
async fn retrieve_token_balances(
&self,
request: Request<RetrieveTokenBalancesRequest>,
) -> Result<Response<RetrieveTokenBalancesResponse>, Status> {
let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } =
request.into_inner();
let account_addresses = account_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid account address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
- .collect::<Vec<_>>();
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
- .collect::<Vec<_>>();
let balances = self
.retrieve_token_balances(account_addresses, contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;
Ok(Response::new(balances))
}📝 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.
| async fn retrieve_token_balances( | |
| &self, | |
| request: Request<RetrieveTokenBalancesRequest>, | |
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | |
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| 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 balances = self | |
| .retrieve_token_balances(account_addresses, contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(balances)) | |
| } | |
| async fn retrieve_token_balances( | |
| &self, | |
| request: Request<RetrieveTokenBalancesRequest>, | |
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | |
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| request.into_inner(); | |
| let account_addresses = account_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid account address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let balances = self | |
| .retrieve_token_balances(account_addresses, contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(balances)) | |
| } |
crates/torii/grpc/src/server/mod.rs
Outdated
| async fn retrieve_token_balances( | ||
| &self, | ||
| account_addresses: Vec<Felt>, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | ||
| let query = format!( | ||
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})", | ||
| account_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", "), | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); | ||
|
|
||
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | ||
| Ok(RetrieveTokenBalancesResponse { balances }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Similar security concerns in token balances retrieval.
The implementation has the same security and performance issues as retrieve_tokens:
- SQL Injection vulnerability
- No input validation
- No limits on address lists
Consider this safer implementation:
async fn retrieve_token_balances(
&self,
account_addresses: Vec<Felt>,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokenBalancesResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if account_addresses.is_empty() || contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("Both account and contract addresses are required"));
+ }
+ if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+ let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+
+ let query = format!(
+ "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})",
+ account_placeholders, contract_placeholders
+ );
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in account_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
- let query = format!(
- "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})",
- account_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", "),
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- );
-
- let balances: Vec<TokenBalance> = sqlx::query_as(&query)
+ let balances: Vec<TokenBalance> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let balances = balances.iter().map(|balance| balance.clone().into()).collect();
Ok(RetrieveTokenBalancesResponse { balances })
}📝 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.
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| let query = format!( | |
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})", | |
| account_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", "), | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| ); | |
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } | |
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if account_addresses.is_empty() || contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("Both account and contract addresses are required")); | |
| } | |
| if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let query = format!( | |
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})", | |
| account_placeholders, contract_placeholders | |
| ); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in account_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let balances: Vec<TokenBalance> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } |
| async fn retrieve_tokens( | ||
| &self, | ||
| request: Request<RetrieveTokensRequest>, | ||
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | ||
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | ||
| let contract_addresses = contract_addresses | ||
| .iter() | ||
| .map(|address| Felt::from_bytes_be_slice(address)) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let tokens = self | ||
| .retrieve_tokens(contract_addresses) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
| Ok(Response::new(tokens)) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Enhance error handling for the gRPC endpoint.
The implementation needs better input validation and error handling:
- No validation of input contract addresses
- Silent failures during Felt conversion
Consider this improved implementation:
async fn retrieve_tokens(
&self,
request: Request<RetrieveTokensRequest>,
) -> Result<Response<RetrieveTokensResponse>, Status> {
let RetrieveTokensRequest { contract_addresses } = request.into_inner();
+ if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+ }
+
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let tokens = self
.retrieve_tokens(contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;
Ok(Response::new(tokens))
}📝 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.
| async fn retrieve_tokens( | |
| &self, | |
| request: Request<RetrieveTokensRequest>, | |
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| Felt::from_bytes_be_slice(address)) | |
| .collect::<Vec<_>>(); | |
| let tokens = self | |
| .retrieve_tokens(contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(tokens)) | |
| } | |
| async fn retrieve_tokens( | |
| &self, | |
| request: Request<RetrieveTokensRequest>, | |
| ) -> Result<Response<RetrieveTokensResponse>, Status> { | |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| if contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("No contract addresses provided")); | |
| } | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let tokens = self | |
| .retrieve_tokens(contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(tokens)) | |
| } |
| async fn retrieve_token_balances( | ||
| &self, | ||
| request: Request<RetrieveTokenBalancesRequest>, | ||
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | ||
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | ||
| 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 balances = self | ||
| .retrieve_token_balances(account_addresses, contract_addresses) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
| Ok(Response::new(balances)) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Enhance error handling for address conversions.
The implementation needs better error handling for Felt conversions:
Consider this improved implementation:
async fn retrieve_token_balances(
&self,
request: Request<RetrieveTokenBalancesRequest>,
) -> Result<Response<RetrieveTokenBalancesResponse>, Status> {
let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } =
request.into_inner();
+ if account_addresses.is_empty() || contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("Both account and contract addresses are required"));
+ }
let account_addresses = account_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid account address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let balances = self
.retrieve_token_balances(account_addresses, contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;
Ok(Response::new(balances))
}📝 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.
| async fn retrieve_token_balances( | |
| &self, | |
| request: Request<RetrieveTokenBalancesRequest>, | |
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | |
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| 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 balances = self | |
| .retrieve_token_balances(account_addresses, contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(balances)) | |
| } | |
| async fn retrieve_token_balances( | |
| &self, | |
| request: Request<RetrieveTokenBalancesRequest>, | |
| ) -> Result<Response<RetrieveTokenBalancesResponse>, Status> { | |
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| request.into_inner(); | |
| if account_addresses.is_empty() || contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("Both account and contract addresses are required")); | |
| } | |
| let account_addresses = account_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid account address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let balances = self | |
| .retrieve_token_balances(account_addresses, contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| Ok(Response::new(balances)) | |
| } |
crates/torii/grpc/src/server/mod.rs
Outdated
| async fn retrieve_tokens( | ||
| &self, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokensResponse, Status> { | ||
| let query = format!( | ||
| "SELECT * FROM tokens WHERE contract_address IN ({})", | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); | ||
|
|
||
| let tokens: Vec<Token> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | ||
| Ok(RetrieveTokensResponse { tokens }) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Please address SQL injection and input validation concerns.
The implementation needs several security and robustness improvements:
- SQL Injection vulnerability: Using string formatting for SQL query construction
- No validation of input contract addresses
- No limit on number of addresses that can be queried
Consider this safer implementation:
async fn retrieve_tokens(
&self,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokensResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+ }
+ if contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let placeholders = contract_addresses
+ .iter()
+ .map(|_| "?")
+ .collect::<Vec<_>>()
+ .join(", ");
+
+ let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders);
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
- let query = format!(
- "SELECT * FROM tokens WHERE contract_address IN ({})",
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- );
-
- let tokens: Vec<Token> = sqlx::query_as(&query)
+ let tokens: Vec<Token> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let tokens = tokens.iter().map(|token| token.clone().into()).collect();
Ok(RetrieveTokensResponse { tokens })
}📝 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.
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| let query = format!( | |
| "SELECT * FROM tokens WHERE contract_address IN ({})", | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| ); | |
| let tokens: Vec<Token> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } | |
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("No contract addresses provided")); | |
| } | |
| if contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| let placeholders = contract_addresses | |
| .iter() | |
| .map(|_| "?") | |
| .collect::<Vec<_>>() | |
| .join(", "); | |
| let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let tokens: Vec<Token> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } |
crates/torii/grpc/src/server/mod.rs
Outdated
| async fn retrieve_token_balances( | ||
| &self, | ||
| account_addresses: Vec<Felt>, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | ||
| let query = format!( | ||
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN \ | ||
| ({})", | ||
| account_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", "), | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); | ||
|
|
||
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | ||
| Ok(RetrieveTokenBalancesResponse { balances }) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Similar security concerns in token balances retrieval.
The implementation has the same security and robustness issues as retrieve_tokens:
- SQL Injection vulnerability
- No input validation
- No limits on address lists
Consider this safer implementation:
async fn retrieve_token_balances(
&self,
account_addresses: Vec<Felt>,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokenBalancesResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if account_addresses.is_empty() || contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("Both account and contract addresses are required"));
+ }
+ if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+ let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+
+ let query = format!(
+ "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})",
+ account_placeholders, contract_placeholders
+ );
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in account_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
- let query = format!(
- "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})",
- account_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", "),
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- );
-
- let balances: Vec<TokenBalance> = sqlx::query_as(&query)
+ let balances: Vec<TokenBalance> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let balances = balances.iter().map(|balance| balance.clone().into()).collect();
Ok(RetrieveTokenBalancesResponse { balances })
}📝 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.
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| let query = format!( | |
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN \ | |
| ({})", | |
| account_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", "), | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| ); | |
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } | |
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if account_addresses.is_empty() || contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("Both account and contract addresses are required")); | |
| } | |
| if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let query = format!( | |
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})", | |
| account_placeholders, contract_placeholders | |
| ); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in account_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let balances: Vec<TokenBalance> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | ||
| let contract_addresses = contract_addresses | ||
| .iter() | ||
| .map(|address| Felt::from_bytes_be_slice(address)) | ||
| .collect::<Vec<_>>(); | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Enhance error handling for retrieve_tokens endpoint.
The current implementation doesn't validate input contract addresses and lacks error handling for Felt conversions, which can cause silent failures.
Apply this diff to improve error handling:
let RetrieveTokensRequest { contract_addresses } = request.into_inner();
+if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+}
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
- .collect::<Vec<_>>();
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let tokens = self
.retrieve_tokens(contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| Felt::from_bytes_be_slice(address)) | |
| .collect::<Vec<_>>(); | |
| let RetrieveTokensRequest { contract_addresses } = request.into_inner(); | |
| if contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("No contract addresses provided")); | |
| } | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let tokens = self | |
| .retrieve_tokens(contract_addresses) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; |
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | ||
| 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<_>>(); | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Improve error handling for retrieve_token_balances endpoint.
There's a need to validate input addresses and handle potential conversion errors to prevent silent failures.
Apply this diff to enhance error handling:
let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } =
request.into_inner();
+if account_addresses.is_empty() || contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("Both account and contract addresses are required"));
+}
let account_addresses = account_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
- .collect::<Vec<_>>();
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid account address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let contract_addresses = contract_addresses
.iter()
- .map(|address| Felt::from_bytes_be_slice(address))
- .collect::<Vec<_>>();
+ .map(|address| {
+ Felt::from_bytes_be_slice(address).map_err(|e| {
+ Status::invalid_argument(format!("Invalid contract address: {}", e))
+ })
+ })
+ .collect::<Result<Vec<_>, Status>>()?;
let balances = self
.retrieve_token_balances(account_addresses, contract_addresses)
.await
.map_err(|e| Status::internal(e.to_string()))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| 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 RetrieveTokenBalancesRequest { account_addresses, contract_addresses } = | |
| request.into_inner(); | |
| if account_addresses.is_empty() || contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("Both account and contract addresses are required")); | |
| } | |
| let account_addresses = account_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid account address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; | |
| let contract_addresses = contract_addresses | |
| .iter() | |
| .map(|address| { | |
| Felt::from_bytes_be_slice(address).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid contract address: {}", e)) | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, Status>>()?; |
| async fn retrieve_tokens( | ||
| &self, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokensResponse, Status> { | ||
| let query = if contract_addresses.is_empty() { | ||
| "SELECT * FROM tokens".to_string() | ||
| } else { | ||
| format!( | ||
| "SELECT * FROM tokens WHERE contract_address IN ({})", | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ) | ||
| }; | ||
|
|
||
| let tokens: Vec<Token> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | ||
| Ok(RetrieveTokensResponse { tokens }) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Address SQL injection and input validation in retrieve_tokens.
The current implementation constructs SQL queries using format!, which can lead to SQL injection vulnerabilities. Additionally, there's no input validation or limit on the number of contract addresses.
Apply this diff to fix the issues:
async fn retrieve_tokens(
&self,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokensResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ if contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("No contract addresses provided"));
+ }
+
+ let placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+ let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders);
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
-
- let query = if contract_addresses.is_empty() {
- "SELECT * FROM tokens".to_string()
- } else {
- format!(
- "SELECT * FROM tokens WHERE contract_address IN ({})",
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- )
- };
-
- let tokens: Vec<Token> = sqlx::query_as(&query)
+ let tokens: Vec<Token> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let tokens = tokens.iter().map(|token| token.clone().into()).collect();
Ok(RetrieveTokensResponse { tokens })
}Would you like assistance implementing these changes?
📝 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.
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| let query = if contract_addresses.is_empty() { | |
| "SELECT * FROM tokens".to_string() | |
| } else { | |
| format!( | |
| "SELECT * FROM tokens WHERE contract_address IN ({})", | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| ) | |
| }; | |
| let tokens: Vec<Token> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } | |
| async fn retrieve_tokens( | |
| &self, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokensResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| if contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("No contract addresses provided")); | |
| } | |
| let placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let query = format!("SELECT * FROM tokens WHERE contract_address IN ({})", placeholders); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let tokens: Vec<Token> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let tokens = tokens.iter().map(|token| token.clone().into()).collect(); | |
| Ok(RetrieveTokensResponse { tokens }) | |
| } |
| async fn retrieve_token_balances( | ||
| &self, | ||
| account_addresses: Vec<Felt>, | ||
| contract_addresses: Vec<Felt>, | ||
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | ||
| let mut query = "SELECT * FROM token_balances".to_string(); | ||
|
|
||
| let mut conditions = Vec::new(); | ||
| if !account_addresses.is_empty() { | ||
| conditions.push(format!( | ||
| "account_address IN ({})", | ||
| account_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| )); | ||
| } | ||
| if !contract_addresses.is_empty() { | ||
| conditions.push(format!( | ||
| "contract_address IN ({})", | ||
| contract_addresses | ||
| .iter() | ||
| .map(|address| format!("{:#x}", address)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| )); | ||
| } | ||
|
|
||
| if !conditions.is_empty() { | ||
| query += &format!(" WHERE {}", conditions.join(" AND ")); | ||
| } | ||
|
|
||
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | ||
| Ok(RetrieveTokenBalancesResponse { balances }) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Secure retrieve_token_balances and improve error handling.
Similar to retrieve_tokens, constructing SQL queries using format! can cause SQL injection vulnerabilities. Additionally, there's insufficient input validation and no limit on the number of addresses.
Apply this diff to address the issues:
async fn retrieve_token_balances(
&self,
account_addresses: Vec<Felt>,
contract_addresses: Vec<Felt>,
) -> Result<RetrieveTokenBalancesResponse, Status> {
+ const MAX_ADDRESSES: usize = 100;
+ if account_addresses.is_empty() || contract_addresses.is_empty() {
+ return Err(Status::invalid_argument("Both account and contract addresses are required"));
+ }
+ if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES {
+ return Err(Status::invalid_argument(
+ format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES)
+ ));
+ }
+
+ let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+ let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
+
+ let query = format!(
+ "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})",
+ account_placeholders, contract_placeholders
+ );
+
+ let mut query_builder = sqlx::query_as(&query);
+ for address in account_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
+ for address in contract_addresses {
+ query_builder = query_builder.bind(format!("{:#x}", address));
+ }
-
- let mut query = "SELECT * FROM token_balances".to_string();
-
- let mut conditions = Vec::new();
- if !account_addresses.is_empty() {
- conditions.push(format!(
- "account_address IN ({})",
- account_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- ));
- }
- if !contract_addresses.is_empty() {
- conditions.push(format!(
- "contract_address IN ({})",
- contract_addresses
- .iter()
- .map(|address| format!("{:#x}", address))
- .collect::<Vec<_>>()
- .join(", ")
- ));
- }
-
- if !conditions.is_empty() {
- query += &format!(" WHERE {}", conditions.join(" AND "));
- }
-
- let balances: Vec<TokenBalance> = sqlx::query_as(&query)
+ let balances: Vec<TokenBalance> = query_builder
.fetch_all(&self.pool)
.await
.map_err(|e| Status::internal(e.to_string()))?;
let balances = balances.iter().map(|balance| balance.clone().into()).collect();
Ok(RetrieveTokenBalancesResponse { balances })
}Let me know if you'd like help applying these changes.
📝 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.
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| let mut query = "SELECT * FROM token_balances".to_string(); | |
| let mut conditions = Vec::new(); | |
| if !account_addresses.is_empty() { | |
| conditions.push(format!( | |
| "account_address IN ({})", | |
| account_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| )); | |
| } | |
| if !contract_addresses.is_empty() { | |
| conditions.push(format!( | |
| "contract_address IN ({})", | |
| contract_addresses | |
| .iter() | |
| .map(|address| format!("{:#x}", address)) | |
| .collect::<Vec<_>>() | |
| .join(", ") | |
| )); | |
| } | |
| if !conditions.is_empty() { | |
| query += &format!(" WHERE {}", conditions.join(" AND ")); | |
| } | |
| let balances: Vec<TokenBalance> = sqlx::query_as(&query) | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } | |
| async fn retrieve_token_balances( | |
| &self, | |
| account_addresses: Vec<Felt>, | |
| contract_addresses: Vec<Felt>, | |
| ) -> Result<RetrieveTokenBalancesResponse, Status> { | |
| const MAX_ADDRESSES: usize = 100; | |
| if account_addresses.is_empty() || contract_addresses.is_empty() { | |
| return Err(Status::invalid_argument("Both account and contract addresses are required")); | |
| } | |
| if account_addresses.len() > MAX_ADDRESSES || contract_addresses.len() > MAX_ADDRESSES { | |
| return Err(Status::invalid_argument( | |
| format!("Too many addresses. Maximum allowed: {}", MAX_ADDRESSES) | |
| )); | |
| } | |
| let account_placeholders = account_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let contract_placeholders = contract_addresses.iter().map(|_| "?").collect::<Vec<_>>().join(", "); | |
| let query = format!( | |
| "SELECT * FROM token_balances WHERE account_address IN ({}) AND contract_address IN ({})", | |
| account_placeholders, contract_placeholders | |
| ); | |
| let mut query_builder = sqlx::query_as(&query); | |
| for address in account_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| for address in contract_addresses { | |
| query_builder = query_builder.bind(format!("{:#x}", address)); | |
| } | |
| let balances: Vec<TokenBalance> = query_builder | |
| .fetch_all(&self.pool) | |
| .await | |
| .map_err(|e| Status::internal(e.to_string()))?; | |
| let balances = balances.iter().map(|balance| balance.clone().into()).collect(); | |
| Ok(RetrieveTokenBalancesResponse { balances }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(5 hunks)
🔇 Additional comments (5)
crates/torii/grpc/src/server/mod.rs (5)
46-46: LGTM! Imports are well organized.
The new imports are correctly placed and necessary for the token-related functionality.
Also applies to: 57-61
104-113: LGTM! TokenBalance conversion is safe.
The implementation correctly converts all fields without any potential panics.
817-841: Previous review comments about SQL injection and input validation are still valid.
The implementation needs the same security improvements mentioned in the past review.
843-883: Previous review comments about SQL injection and input validation are still valid.
The implementation needs the same security improvements mentioned in the past review.
1261-1298: Previous review comments about error handling and input validation are still valid.
The implementation needs the same improvements mentioned in the past review.
crates/torii/grpc/src/server/mod.rs
Outdated
| impl From<Token> for proto::types::Token { | ||
| fn from(value: Token) -> Self { | ||
| Self { | ||
| contract_address: value.contract_address, | ||
| name: value.name, | ||
| symbol: value.symbol, | ||
| decimals: value.decimals as u32, | ||
| metadata: serde_json::to_string(&value.metadata).unwrap(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential serialization errors in Token conversion.
The serde_json::to_string(&value.metadata).unwrap() call could panic if the metadata is not valid JSON. Consider proper error handling.
- metadata: serde_json::to_string(&value.metadata).unwrap(),
+ metadata: serde_json::to_string(&value.metadata)
+ .map_err(|e| Status::internal(format!("Failed to serialize metadata: {}", e)))?,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
crates/torii/client/src/client/mod.rs (3)
91-97: Consider these improvements for the tokens method, sensei!
- The method uses
write().awaitbut appears to only perform read operations. Consider usingread().awaitinstead for better concurrency.- The collection transformation could be more explicit about error handling:
Ok(tokens .into_iter() .map(TryInto::try_into) .collect::<Result<Vec<Token>, _>>()?)
- Consider adding input validation for empty
contract_addresses.
99-109: Enhance the token_balances implementation, sensei!
- Similar to the
tokensmethod, consider usingread().awaitinstead ofwrite().await.- Add documentation comments explaining:
- Expected format of addresses
- Return value structure
- Error conditions
- Consider validating that neither
account_addressesnorcontract_addressesis empty.Example documentation:
/// Retrieves token balances for the specified accounts and contracts. /// /// # Arguments /// * `account_addresses` - List of account addresses to query balances for /// * `contract_addresses` - List of token contract addresses to query /// /// # Returns /// A vector of `TokenBalance` containing the balance information for each account-token pair /// /// # Errors /// Returns an error if the gRPC call fails or if the response cannot be parsed
91-109: Consider architectural improvements for token-related functionalityTo enhance maintainability and reusability, consider:
- Creating a dedicated trait or module for token-related operations
- Implementing common validation and error handling utilities
- Adding integration tests for the token-related functionality
Example structure:
mod token { pub trait TokenOperations { async fn tokens(&self, addresses: Vec<Felt>) -> Result<Vec<Token>, Error>; async fn token_balances( &self, accounts: Vec<Felt>, contracts: Vec<Felt> ) -> Result<Vec<TokenBalance>, Error>; } impl TokenOperations for Client { // Implementation here } }crates/torii/grpc/src/client.rs (2)
95-109: Ohayo! Method looks good, but could use some documentation love!The implementation is solid with proper error handling and data conversion. Consider adding documentation comments to explain the purpose and expected behavior of this method.
Add documentation like this:
+ /// Retrieve token information for the given contract addresses. + /// + /// # Arguments + /// * `contract_addresses` - Vector of contract addresses to query token information for + /// + /// # Returns + /// Token information for the specified contracts or an error if the retrieval fails pub async fn retrieve_tokens(
111-130: Consider adding input validation, sensei!While the implementation is solid, consider adding validation to ensure neither input vector is empty and that their sizes are reasonable for the expected use case.
Here's a suggested improvement:
pub async fn retrieve_token_balances( &mut self, account_addresses: Vec<Felt>, contract_addresses: Vec<Felt>, ) -> Result<RetrieveTokenBalancesResponse, Error> { + if account_addresses.is_empty() || contract_addresses.is_empty() { + return Err(Error::Grpc(tonic::Status::invalid_argument( + "Both account and contract addresses must be non-empty", + ))); + } + self.innerAlso, consider adding documentation similar to retrieve_tokens:
+ /// Retrieve token balances for the given accounts and contracts. + /// + /// # Arguments + /// * `account_addresses` - Vector of account addresses to query balances for + /// * `contract_addresses` - Vector of contract addresses of the tokens + /// + /// # Returns + /// Token balances for the specified accounts and contracts or an error if the retrieval fails pub async fn retrieve_token_balances(crates/torii/grpc/src/types/mod.rs (2)
30-41: Add validation for decimals and string fields.Ohayo sensei! While the implementation is functional, consider adding validation for:
decimalsto ensure it's within a reasonable range (typically 0-18 for ERC20)nameandsymbolto ensure they're not empty stringsHere's a suggested improvement:
impl TryFrom<proto::types::Token> for Token { type Error = SchemaError; fn try_from(value: proto::types::Token) -> Result<Self, Self::Error> { + // Validate decimals + if value.decimals > 18 { + return Err(SchemaError::InvalidField("decimals exceeds maximum value of 18".into())); + } + + // Validate strings + if value.name.is_empty() || value.symbol.is_empty() { + return Err(SchemaError::InvalidField("name and symbol cannot be empty".into())); + } + Ok(Self { contract_address: Felt::from_str(&value.contract_address)?, name: value.name, symbol: value.symbol, decimals: value.decimals as u8, metadata: value.metadata, }) } }
51-61: Consider validating token_id format.Ohayo sensei! The implementation looks good, but consider adding validation for
token_idto ensure it follows your expected format (e.g., numeric string for ERC20 or valid hex for ERC721/1155).Here's a suggested improvement:
impl TryFrom<proto::types::TokenBalance> for TokenBalance { type Error = SchemaError; fn try_from(value: proto::types::TokenBalance) -> Result<Self, Self::Error> { + // Validate token_id format + if value.token_id.is_empty() { + return Err(SchemaError::InvalidField("token_id cannot be empty".into())); + } + Ok(Self { balance: U256::from_be_hex(&value.balance), account_address: Felt::from_str(&value.account_address)?, contract_address: Felt::from_str(&value.contract_address)?, token_id: value.token_id, }) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/client/src/client/mod.rs(2 hunks)crates/torii/grpc/src/client.rs(2 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)
🔇 Additional comments (4)
crates/torii/client/src/client/mod.rs (1)
14-19: Ohayo sensei! The imports look good!
The new imports are well-organized and properly scoped for the token-related functionality.
crates/torii/grpc/src/client.rs (1)
14-20: Ohayo sensei! Import changes look good!
The new token-related types are properly imported and organized consistently with the existing code structure.
crates/torii/grpc/src/types/mod.rs (2)
21-28: LGTM! Token struct definition looks good.
The struct has appropriate fields and derives necessary traits for serialization and comparison.
43-49: LGTM! TokenBalance struct definition looks good.
The struct uses appropriate types, particularly U256 for balance which can handle large token amounts.
Summary by CodeRabbit
Release Notes
New Features
TokenandTokenBalanceto enhance token management.RetrieveTokensandRetrieveTokenBalancesfor fetching tokens and their balances.Improvements
These updates provide users with improved capabilities for managing and querying token information within the application.