feat(torii): index whitelisted ERC20/ERC721 tokens #2277
Conversation
lambda-0x
force-pushed
the
torii-index-erc
branch
from
1436ca7 to
62a4f34
Compare
WalkthroughOhayo, sensei! The recent changes to the Torii application significantly enhance its capabilities for handling ERC contracts. Users can now dynamically index ERC20 and ERC721 tokens through a new command-line argument, alongside improved database schemas and GraphQL integration. These updates greatly enhance the application’s configurability and robustness, enabling seamless interaction with Ethereum standards. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant Database
participant GraphQL
User->>CLI: Start application with erc_contracts
CLI->>Config: Load ERC contracts from argument
Config->>Database: Register contracts
Database-->>Config: Confirmation of registration
CLI-->>User: Application ready
User->>GraphQL: Query for ERC balance
GraphQL->>Database: Fetch balance data
Database-->>GraphQL: Return balance info
GraphQL-->>User: Display balance
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (22)
crates/katana/scripts/deploy-erc20.sh (1)
1-1: Add a shebang for clarity and portability.Consider adding a shebang (e.g.,
#!/bin/bash) at the top of the script to specify the shell environment.+#!/bin/bash starkli deploy --account account.json --keystore signer.json --keystore-password "" 0x02a8846878b6ad1f54f6ba46f5f40e11cee755c677f130b2c4b60566c9003f1f 0x626c6f62 0x424c42 0x8 u256:10000000000 0xb3ff441a68610b30fd5e2abbf3a1548eb6ba6f3559f2862bf2dc757e5828cTools
Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
bin/torii/torii.toml (1)
6-8: Consider adding comments for clarity.Adding comments to explain the purpose of each field in the configuration can improve readability and maintainability.
erc_contracts = [ { type = "ERC20", contract_address = "0x07fc13cc1f43f0b0519f84df8bf13bea4d9fd5ce2d748c3baf27bf90a565f60a", start_block = 0 } # Example ERC20 contract ]crates/torii/core/src/processors/erc20_transfer.rs (3)
1-7: Ensure appropriate error handling and logging.The imports used are relevant and necessary for the functionality. However, consider adding more detailed error handling and logging to capture potential issues during event processing. This will aid in debugging and monitoring.
26-34: Validate event structure carefully.The
validatemethod checks the structure of the event. Ensure that this validation logic aligns with the expected format of ERC20 transfer events. Consider logging validation failures for better traceability.
36-57: Review the process method for completeness.The
processmethod handles the core logic of processing an ERC20 transfer. Ensure that all necessary steps, such as updating the database and logging the transfer, are correctly implemented. Consider adding more detailed logging for successful and failed operations.crates/torii/core/src/processors/erc20_legacy_transfer.rs (3)
1-7: Ensure appropriate error handling and logging.Similar to the
erc20_transfer.rsfile, consider enhancing error handling and logging to capture potential issues during the processing of legacy events.
26-34: Validate event structure carefully.The
validatemethod checks for a specific structure in legacy events. Ensure this aligns with the expected format of legacy ERC20 transfer events. Consider logging validation failures for better traceability.
36-57: Review the process method for completeness.The
processmethod should handle all necessary steps for processing a legacy ERC20 transfer. Ensure that differences from the standard ERC20 processing are correctly implemented and logged.crates/torii/core/src/processors/erc721_transfer.rs (3)
1-7: Ensure appropriate error handling and logging.As with the other processor files, consider enhancing error handling and logging to capture potential issues during the processing of ERC721 events.
26-34: Validate event structure carefully.The
validatemethod checks for a specific structure in ERC721 events. Ensure this aligns with the expected format of ERC721 transfer events. Consider logging validation failures for better traceability.
37-58: Review the process method for completeness.The
processmethod should handle all necessary steps for processing an ERC721 transfer. Ensure that differences from ERC20 processing are correctly implemented and logged.crates/torii/core/src/types.rs (3)
89-93: Add documentation forToriiConfig.The
ToriiConfigstruct is well-defined, but adding documentation comments would improve clarity and maintainability.+ /// Configuration for Torii, including ERC contracts to index. pub struct ToriiConfig {
103-108: Add documentation forErcContract.The
ErcContractstruct is well-defined, but adding documentation comments would improve clarity and maintainability.+ /// Represents an ERC contract with its address, start block, and type. pub struct ErcContract {
110-115: Add documentation forErcType.The
ErcTypeenum is well-defined, but adding documentation comments would improve clarity and maintainability.+ /// Enum representing the type of ERC contract, either ERC20 or ERC721. pub enum ErcType {crates/torii/graphql/src/object/erc/erc_balance.rs (3)
15-16: Add documentation forErcBalanceObject.The
ErcBalanceObjectstruct is defined but lacks documentation. Adding comments would improve clarity and maintainability.+ /// Represents a GraphQL object for ERC balance. pub struct ErcBalanceObject;
58-100: Optimize the SQL query infetch_erc_balances.The
fetch_erc_balancesfunction uses a SQL query that joins multiple tables. Ensure that the necessary indexes are present on the database to optimize this query.Consider adding indexes on
balances.account_address,tokens.id, andcontracts.contract_addressto improve query performance.
110-119: Add documentation forBalanceQueryResultRaw.The
BalanceQueryResultRawstruct is defined but lacks documentation. Adding comments would improve clarity and maintainability.+ /// Represents a raw query result for ERC balance. struct BalanceQueryResultRaw {crates/torii/graphql/src/schema.rs (2)
13-14: Add documentation for new imports.The new imports for
ErcBalanceObjectandErcTokenObjectare correct, but adding documentation comments would improve clarity.+ // Importing ERC objects for GraphQL schema. use crate::object::erc::erc_balance::ErcBalanceObject; use crate::object::erc::erc_token::ErcTokenObject;
33-33: Clarify the comment aboutQUERY_TYPE_NAME.The comment questioning the necessity of providing
QUERY_TYPE_NAMEis useful, but consider clarifying it further or addressing it in a separate issue if needed.//? why we need to provide QUERY_TYPE_NAME object here when its already passed to Schema? + // Consider investigating this further or documenting the reason.crates/torii/graphql/src/mapping.rs (2)
146-150: Add documentation forERC_BALANCE_TYPE_MAPPING.The
ERC_BALANCE_TYPE_MAPPINGis defined but lacks documentation. Adding comments would improve clarity and maintainability.+ /// Mapping for ERC balance type in GraphQL. pub static ref ERC_BALANCE_TYPE_MAPPING: TypeMapping = IndexMap::from([
152-157: Add documentation forERC_TOKEN_TYPE_MAPPING.The
ERC_TOKEN_TYPE_MAPPINGis defined but lacks documentation. Adding comments would improve clarity and maintainability.+ /// Mapping for ERC token type in GraphQL. pub static ref ERC_TOKEN_TYPE_MAPPING: TypeMapping = IndexMap::from([bin/torii/src/main.rs (1)
123-126: Add documentation forerc_contractsargument.The
erc_contractsargument is well-defined, but adding documentation comments would improve clarity and maintainability.+ /// Optional list of ERC contract addresses to index, specified in the format `erc_type:address:start_block`. #[arg(long, value_parser = parse_erc_contracts)]
| impl ResolvableObject for ErcBalanceObject { | ||
| fn resolvers(&self) -> Vec<Field> { | ||
| let account_address = "account_address"; | ||
| let argument = InputValue::new( | ||
| account_address.to_case(Case::Camel), | ||
| TypeRef::named_nn(TypeRef::STRING), | ||
| ); | ||
|
|
||
| let field = Field::new(self.name().0, TypeRef::named_list(self.type_name()), move |ctx| { | ||
| FieldFuture::new(async move { | ||
| let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?; | ||
| let address: String = extract::<String>( | ||
| ctx.args.as_index_map(), | ||
| &account_address.to_case(Case::Camel), | ||
| )?; | ||
|
|
||
| let erc_balances = fetch_erc_balances(&mut conn, &address).await?; | ||
|
|
||
| Ok(Some(Value::List(erc_balances))) | ||
| }) | ||
| }) | ||
| .argument(argument); | ||
| vec![field] | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in resolvers.
The resolvers method in ResolvableObject for ErcBalanceObject should ensure that errors are properly logged or handled, especially when fetching balances.
let field = Field::new(self.name().0, TypeRef::named_list(self.type_name()), move |ctx| {
FieldFuture::new(async move {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let address: String = extract::<String>(
ctx.args.as_index_map(),
&account_address.to_case(Case::Camel),
)?;
match fetch_erc_balances(&mut conn, &address).await {
Ok(erc_balances) => Ok(Some(Value::List(erc_balances))),
Err(e) => {
warn!("Failed to fetch ERC balances: {}", e);
Err(e.into())
}
}
})
})
.argument(argument);
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/torii/core/src/sql.rs (1)
Ohayo, sensei! I've reviewed the
handle_erc20_transferfunction, which manages ERC20 token transfers, including fetching token details and updating balances. While the logic seems correct, the error handling could be improved to avoid potential panics caused byunwrapcalls. Consider using more robust error handling strategies to ensure the function handles unexpected data gracefully.
- Improve error handling: Replace
unwrapcalls with proper error handling to avoid potential panics.Analysis chain
Line range hint
255-437: Verify ERC20 transfer handling logic and error handling.The
handle_erc20_transfermethod includes logic for fetching token details and updating balances. Ensure the transfer handling logic is correct and robust against errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the transfer handling logic and error handling in `handle_erc20_transfer`. # Test: Check for method usage and error handling. Expect: Correct transfer logic and robust error handling. rg --type rust 'pub async fn handle_erc20_transfer'Length of output: 142
Script:
#!/bin/bash # Extract the implementation of `handle_erc20_transfer` for detailed analysis. ast-grep --lang rust --pattern 'pub async fn handle_erc20_transfer<P: Provider + Sync>($_) { $$$ }' crates/torii/core/src/sql.rsLength of output: 132
Script:
#!/bin/bash # Use `rg` to search for the `handle_erc20_transfer` function and include surrounding lines for context. rg --type rust 'pub async fn handle_erc20_transfer' -A 50 crates/torii/core/src/sql.rsLength of output: 1960
Script:
#!/bin/bash # Continue retrieving more lines to capture the complete implementation of the `handle_erc20_transfer` function. rg --type rust 'pub async fn handle_erc20_transfer' -A 100 crates/torii/core/src/sql.rsLength of output: 4053
Script:
#!/bin/bash # Retrieve additional lines to capture the complete implementation of the `handle_erc20_transfer` function. rg --type rust 'pub async fn handle_erc20_transfer' -A 150 crates/torii/core/src/sql.rsLength of output: 6383
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (8)
crates/torii/graphql/src/tests/metadata_test.rs (1)
105-105: Check for potential configuration needs withHashMap.Adding a
HashMap::default()might suggest future configurability. Ensure that any necessary configurations are planned or documented.crates/torii/grpc/src/server/tests/entities_test.rs (1)
112-112: EnsureHashMap::default()is necessary inEngine::new.The inclusion of
HashMap::default()inEngine::newsuggests future configurability. Verify its necessity and document any potential configurations.crates/torii/graphql/src/tests/mod.rs (1)
366-366: EnsureHashMap::default()is necessary inEngine::new.The inclusion of
HashMap::default()inEngine::newsuggests future configurability. Verify its necessity and document any potential configurations.crates/torii/core/src/sql_test.rs (2)
122-122: Check for potential configuration needs withHashMap.Adding a
HashMap::default()might suggest future configurability. Ensure that any necessary configurations are planned or documented.
281-281: EnsureHashMap::default()usage is appropriate.The addition of
HashMap::default()should be reviewed to confirm its necessity and appropriateness in the context of the test.crates/torii/graphql/src/tests/subscription_test.rs (3)
151-151: Check for potential configuration needs withHashMap.Adding a
HashMap::default()might suggest future configurability. Ensure that any necessary configurations are planned or documented.
256-256: EnsureHashMap::default()usage is appropriate.The addition of
HashMap::default()should be reviewed to confirm its necessity and appropriateness in the context of the test.
385-385: Check for potential configuration needs withHashMap.Adding a
HashMap::default()might suggest future configurability. Ensure that any necessary configurations are planned or documented.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2277 +/- ##
==========================================
+ Coverage 66.18% 66.38% +0.20%
==========================================
Files 352 358 +6
Lines 46739 47005 +266
==========================================
+ Hits 30933 31205 +272
+ Misses 15806 15800 -6 ☔ View full report in Codecov by Sentry. |
lambda-0x
force-pushed
the
torii-index-erc
branch
from
6dee6c8 to
1240bdf
Compare
There was a problem hiding this comment.
add erc721_legacy_transfer
There was a problem hiding this comment.
it wont be possible to add it with current implementation because both erc20_legacy_transer and erc721_legacy_transfer would have same event format data which would make it not possible to distinguish between them.
i.e.
erc20_legacy_transfer:
key: [hash("Transfer")]
data: [from, to, value.0, value.1]
erc721_legacy_transfer:
key: [hash("Transfer")]
data: [from, to, token_id.0, token_id.1]
if we want to support this we would need to change how event proccessor are selected in the engine
There was a problem hiding this comment.
Great work @lambda-0x on this, some comments and general note on formatting. 👍
There was a problem hiding this comment.
We should definitely start to have a sql module and splitting up the code for easier addition/maintenance. :)
| ); | ||
| } | ||
|
|
||
| // Update balances in erc20_balance table |
There was a problem hiding this comment.
I'm not sure about the manipulation of balances here. Because until we reach head of the chain, the balance will not be accurate.
Or do we assume that we should reach the head of the chain pretty quickly, and then it's fine to keep updating the balances?
From a client perspective, let's take Controller as an example, we may want to query the current balance with regular interval when it's needed. In this case, using the balance in the DB makes little sense? Or once again we assume we should be quickly in the head of chain, and pulling data from the DB or the chain should return the same result.
There was a problem hiding this comment.
we assume that we should reach the head of the chain pretty quickly
yeah thats the assumption we make
say we only provide the info of which tokens are owned by user for that also we would need to index all the transfer events anyways and if we are lagging behind in indexing controller might not get info about some of the token.
so even by not keeping the balance we dont fully solve that issue. and not to mention that if user has large amount of tokens controller would need to make a rpc call to get balance for each of them.
| self.query_queue.enqueue( | ||
| update_query, | ||
| vec
|
@lambda-0x ideally rebased after #2423 is merged (or next release we should be tonight). |
|
superseded by #2442 |
Description
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores