feat(torii): index whitelisted erc20/erc721 #2442
Conversation
WalkthroughOhayo, sensei! This pull request enhances the application by adding support for ERC contract addresses through a new command-line argument. It updates the processing logic for ERC20 and ERC721 transfers and introduces a configuration file for ERC contracts. Additionally, the database schema is expanded to manage token balances and transfers, improving the overall functionality related to ERC tokens. Changes
Suggested reviewers
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (5)
scripts/deploy_erc20_katana.sh (1)
3-3: Enhance script readability with commentsSensei, adding comments to explain the purpose of each parameter in the
starkli deploycommand will improve the script's maintainability and help others understand its functionality.crates/torii/core/src/processors/erc20_transfer.rs (1)
14-58: Consider adding unit tests forErc20TransferProcessorSensei, to ensure the robustness of the
Erc20TransferProcessor, consider adding unit tests that cover various scenarios, including valid events, invalid events, and error handling. This will help maintain code quality and catch potential issues early.crates/torii/core/src/sql/mod.rs (3)
35-35: Updating test module path totest.rsThe test module path has been updated from
sql_test.rstotest.rs. Ensure that this change is reflected in any documentation or build scripts that reference the test module.Confirm that all tests are still properly located and executed with the new module path.
39-39: Review the necessity of publicly re-exporting theutilsmoduleThe line
pub use utils::*;publicly exposes all items from theutilsmodule. If these utilities are intended for internal use only within thesqlmodule, consider removing thepubkeyword to encapsulate the implementation details.This change can help maintain a clean public API and prevent unintended usage of internal utilities.
742-746: Simplify conditional expression inbuild_set_entity_queries_recursiveThe conditional expression can be simplified for readability.
Consider rewriting the condition as:
if let Ty::Tuple(t) = &o.ty { t.is_empty() } else { false }This makes the intent clearer and improves code readability.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (15)
- bin/torii/src/main.rs (7 hunks)
- bin/torii/torii.toml (1 hunks)
- crates/torii/core/Cargo.toml (1 hunks)
- crates/torii/core/src/engine.rs (12 hunks)
- crates/torii/core/src/processors/erc20_legacy_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc20_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc721_transfer.rs (1 hunks)
- crates/torii/core/src/processors/mod.rs (2 hunks)
- crates/torii/core/src/sql/erc.rs (1 hunks)
- crates/torii/core/src/sql/mod.rs (6 hunks)
- crates/torii/core/src/sql/utils.rs (1 hunks)
- crates/torii/core/src/types.rs (2 hunks)
- crates/torii/migrations/20240913104418_add_erc.sql (1 hunks)
- scripts/deploy_erc20_katana.sh (1 hunks)
- scripts/send_erc20_transfer.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- bin/torii/torii.toml
Additional comments not posted (20)
scripts/deploy_erc20_katana.sh (1)
3-3: Consider securing your keystore with a passwordSensei, the script uses an empty password for the keystore (
--keystore-password ""), which may pose a security risk if the keystore contains sensitive information. It's recommended to secure the keystore with a strong password to protect your private keys.[security]
scripts/send_erc20_transfer.sh (1)
12-12: Consider securing your keystore with a passwordSensei, the script uses an empty password for the keystore (
--keystore-password ""), which may pose a security risk. It's advisable to secure the keystore with a strong password to protect your private keys.[security]
crates/torii/migrations/20240913104418_add_erc.sql (1)
28-29: Consider adding indexes onfrom_addressandto_addressSensei, to improve query performance when filtering or joining on
from_addressandto_address, consider adding indexes on these columns in theerc_transferstable.[performance]
Apply this SQL to add the indexes:
CREATE INDEX erc_transfers_from_address ON erc_transfers (from_address); CREATE INDEX erc_transfers_to_address ON erc_transfers (to_address);crates/torii/core/Cargo.toml (1)
41-41: Dependency addition looks goodSensei, adding the
tomlcrate to the workspace dependencies is appropriate and will allow for consistent use across the project.crates/torii/core/src/processors/erc20_legacy_transfer.rs (2)
26-34: Ohayo, sensei! Validation Logic is SolidThe
validatemethod appropriately checks that the event has exactly one key and four data elements, aligning with the expected structure of a legacy ERC20 transfer event. This ensures that only correctly formatted events are processed.
36-57: Efficient Handling of Legacy ERC20 Transfer EventsThe
processmethod correctly extracts thefrom,to, andvaluefields from the event data. Good job on deserializing the value properly and logging the transfer details. This will help in accurate tracking of token transfers.crates/torii/core/src/processors/erc721_transfer.rs (2)
26-35: Ohayo, sensei! Proper Validation for ERC721 EventsThe
validatemethod correctly checks that the event has five keys and no data, which matches the expected structure of an ERC721 transfer event. This validation ensures that only relevant events are processed.
46-52: Ensure Correct Extraction offrom,to, andtoken_idFieldsPlease verify that the indices used to extract
from,to, andtoken_idfromevent.keysmatch the ERC721 event structure. Specifically, confirm that:
event.keys[1]corresponds to thefromaddress.event.keys[2]corresponds to thetoaddress.event.keys[3..5]are correctly used to deserialize thetoken_id.crates/torii/core/src/processors/mod.rs (2)
15-17: Ohayo, sensei! Modules Exported SuccessfullyUncommenting the module declarations for
erc20_legacy_transfer,erc20_transfer, anderc721_transfercorrectly includes these processors in the processing logic. This expansion allows the system to handle a wider range of events related to ERC token transfers.
84-90: Correct Modification to Support Multiple Processors per Event KeyChanging the
generate_event_processors_mapfunction to useVec<Box<dyn EventProcessor<P>>>allows multiple processors to be associated with the same event key. This modification enhances flexibility in event handling.Please verify that all parts of the codebase that interact with this function can accommodate the updated return type and structure.
bin/torii/src/main.rs (1)
Line range hint
122-145: Ohayo, sensei! Conflicting Arguments Managed ProperlyThe introduction of the
erc_contractsparameter with a conflict against theconfigparameter is handled correctly. This prevents ambiguous configurations and ensures that only one source of ERC contract configuration is used at a time.crates/torii/core/src/engine.rs (5)
9-9: Ohayo, sensei! Preparing for concurrent event fetching withjoin_allThe addition of
use futures_util::future::join_all;allows for concurrent execution of futures, which can significantly improve the performance of fetching events from multiple sources.
521-530: Ohayo, sensei! Ensure thread safety when merging local databases in parallel tasksIn the
process_tasksmethod, each task processes events using alocal_db, which is then merged intoself.db. Ensure that themergefunction is thread-safe and can handle concurrent modifications without causing data races or deadlocks.Please confirm that
self.db.merge(local_db)?;is safe in a concurrent context. If necessary, consider adding synchronization mechanisms or using thread-safe data structures to protect shared resources during the merge.
604-606: Filtering events to process only relevant onesThe added condition ensures that only events from the world contract or the specified tokens are processed. This helps in skipping unrelated events and improves processing efficiency.
Line range hint
710-747: Verify task identification logic for parallelized event processingThe
task_identifieris determined based on the event data for specific event keys. This mechanism is used to group events for parallel processing.Please confirm that this approach correctly identifies tasks and that events are grouped appropriately. Also, consider documenting the rationale behind using specific event keys for task identification for clarity.
756-780: Addition ofget_all_eventsfunction for comprehensive event retrievalThe new
get_all_eventsfunction properly handles pagination by looping until all events are retrieved. This ensures that all events are collected, which is essential for processing large numbers of events without missing any.crates/torii/core/src/sql/mod.rs (4)
22-23: Ohayo, sensei! Importing necessary types for ERC contract handlingThe addition of imports for
ErcContract,Event,EventMessage, andModelensures that the types needed for the new functionality are available.
33-33: Introducing theercmodule for ERC contract supportThe inclusion of
mod erc;indicates the introduction of a new module to handle ERC contracts. This modular approach keeps the codebase organized and maintainable.
61-65: Ohayo, sensei! ExtendingSql::newto initialize with ERC contractsThe
Sql::newfunction now acceptserc_contractsto insert ERC contract data into the database during initialization. This change allows the system to recognize and manage ERC contracts effectively from the start.
79-91: Inserting ERC contracts into thecontractstableThe code correctly iterates through
erc_contractsand inserts each contract into thecontractstable if it doesn't already exist. This ensures that all relevant ERC contracts are accounted for in the database.
|
@lambda-0x please consider indexing ERC-1155 as well |
|
@rsodre thanks for the feedback we have designed tables in such a way that it should be straight forward to add, so will try to add it in this PR itself. Although I am curious how does game use ERC1155, do they mainly need one ERC20 and multiplle ERC721, or just multiple ERC20s? |
lambda-0x
force-pushed
the
spr/main/5231a946
branch
from
d2b7c23
to
d53a2e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
crates/torii/core/src/engine.rs (1)
Line range hint
672-749: Ensure Catch-All Event Processor InvocationOhayo, sensei! The current implementation might skip processing events if no specific processors are found, even when the
catch_all_eventprocessor is meant to handle them.Adjust the logic to ensure the
catch_all_eventprocessor is invoked appropriately:let Some(processors) = self.processors.event.get(&event_key) else { // If we don't have a processor for this event key + if self.processors.catch_all_event.validate(event) { + if let Err(e) = self + .processors + .catch_all_event + .process( + &self.world, + &mut self.db, + block_number, + block_timestamp, + event_id, + event, + ) + .await + { + error!(target: LOG_TARGET, error = %e, "Processing catch-all event."); + } + } else { + // Log unprocessed events if necessary + } return Ok(()); };This ensures that events without specific processors are still handled by the
catch_all_eventprocessor.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (18)
- bin/torii/src/main.rs (7 hunks)
- bin/torii/torii.toml (1 hunks)
- crates/torii/core/Cargo.toml (1 hunks)
- crates/torii/core/src/engine.rs (13 hunks)
- crates/torii/core/src/lib.rs (0 hunks)
- crates/torii/core/src/processors/erc20_legacy_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc20_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc721_transfer.rs (1 hunks)
- crates/torii/core/src/processors/mod.rs (2 hunks)
- crates/torii/core/src/processors/store_set_record.rs (1 hunks)
- crates/torii/core/src/sql/erc.rs (1 hunks)
- crates/torii/core/src/sql/mod.rs (5 hunks)
- crates/torii/core/src/sql/utils.rs (1 hunks)
- crates/torii/core/src/types.rs (2 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
- crates/torii/migrations/20240913104418_add_erc.sql (1 hunks)
- scripts/deploy_erc20_katana.sh (1 hunks)
- scripts/send_erc20_transfer.sh (1 hunks)
Files not reviewed due to no reviewable changes (1)
- crates/torii/core/src/lib.rs
Files skipped from review due to trivial changes (4)
- bin/torii/torii.toml
- crates/torii/core/Cargo.toml
- crates/torii/core/src/processors/store_set_record.rs
- crates/torii/libp2p/src/server/mod.rs
Files skipped from review as they are similar to previous changes (10)
- bin/torii/src/main.rs
- crates/torii/core/src/processors/erc20_legacy_transfer.rs
- crates/torii/core/src/processors/erc20_transfer.rs
- crates/torii/core/src/processors/erc721_transfer.rs
- crates/torii/core/src/processors/mod.rs
- crates/torii/core/src/sql/utils.rs
- crates/torii/core/src/types.rs
- crates/torii/migrations/20240913104418_add_erc.sql
- scripts/deploy_erc20_katana.sh
- scripts/send_erc20_transfer.sh
Additional comments not posted (2)
crates/torii/core/src/engine.rs (1)
518-527: Potential Task Execution Issue with Semaphore AcquisitionOhayo, sensei! In the
process_tasksmethod, acquiring a semaphore permit inside the spawned task may not effectively limit the number of concurrent tasks, leading to potential resource exhaustion.Consider acquiring the semaphore permit before spawning the task to ensure proper concurrency control:
for (task_id, events) in self.tasks.drain() { let db = self.db.clone(); let world = self.world.clone(); let processors = self.processors.clone(); let semaphore = semaphore.clone(); + let permit = semaphore.acquire().await.unwrap(); set.spawn(async move { - let _permit = semaphore.acquire().await.unwrap(); + let _permit = permit; // Keep the permit alive for the task duration // Task code... }); }This modification ensures that the semaphore controls the number of tasks being spawned, preventing too many concurrent tasks from executing.
Likely invalid or redundant comment.
crates/torii/core/src/sql/mod.rs (1)
742-746: Correct Enum Handling in Recursive Query BuildingOhayo, sensei! In the
build_set_entity_queries_recursivemethod, the condition for skipping empty enum options may incorrectly return early, causing code that should execute to be skipped.Modify the condition to prevent unintentional early returns:
if e.options.iter().all( |o| match &o.ty { - Ty::Tuple(t) => t.is_empty(), + Ty::Tuple(t) => t.is_empty(), + _ => false, }, ) { return; }This adjustment ensures that only enums with all empty tuple options are skipped, allowing proper processing of enums with meaningful variants.
Likely invalid or redundant comment.
lambda-0x
force-pushed
the
spr/main/5231a946
branch
2 times, most recently
from
fc56768
to
83ac6d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (5)
crates/torii/core/src/sql/test.rs (1)
48-52: Ohayo sensei! The code changes look good to me.The shift from
Arc::newtoBox::newfor wrapping the processors is a valid change in ownership semantics.However, please consider the performance implications of using
Box::newinstead ofArc::new. WhileBox::newallocates on the heap,Arc::newprovides reference counting, which may be more suitable in certain scenarios. Ensure that the chosen approach aligns with the performance requirements of the codebase.crates/torii/core/src/engine.rs (3)
Line range hint
232-349: Ohayo sensei! The changes tofetch_rangeare looking great.Fetching events concurrently for the world contract and ERC tokens using
join_allis a significant performance improvement. The processing of fetched events to extract transactions and blocks is handled correctly.One suggestion for improvement:
Consider collecting the results of each task and handling errors individually. This would allow the function to continue processing events from other contracts even if one task fails, making the engine more resilient to transient errors.
Line range hint
351-372: Ohayo sensei! Thefetch_pendingfunction looks good.It correctly fetches the pending block data and handles the case when the pending block is not available.
Don't forget to address the TODO comment once the provider is updated to always return a pending block. Let me know if you need any help with that!
Line range hint
552-576: Ohayo sensei! Theprocess_transaction_with_eventsfunction looks good.It correctly processes each event associated with the transaction by calling the
process_eventfunction with the necessary parameters.I noticed that the transaction processor is commented out due to performance reasons. If it is no longer needed, consider removing the commented-out code to keep the codebase clean.
crates/torii/core/src/sql/mod.rs (1)
Line range hint
68-98: Ohayo sensei! Offer assistance with unit tests forerc_contractshandlingThe updated
newmethod now acceptserc_contractsand processes them to insert contracts into the database. To ensure this new functionality is reliable, it's important to have appropriate unit tests. I can help by generating unit tests to cover different scenarios, such as inserting multiple contracts and handling duplicates.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (26)
- bin/torii/src/main.rs (7 hunks)
- bin/torii/torii.toml (1 hunks)
- crates/torii/core/Cargo.toml (1 hunks)
- crates/torii/core/src/engine.rs (24 hunks)
- crates/torii/core/src/lib.rs (0 hunks)
- crates/torii/core/src/processors/erc20_legacy_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc20_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc721_transfer.rs (1 hunks)
- crates/torii/core/src/processors/mod.rs (2 hunks)
- crates/torii/core/src/processors/store_set_record.rs (1 hunks)
- crates/torii/core/src/sql/erc.rs (1 hunks)
- crates/torii/core/src/sql/mod.rs (9 hunks)
- crates/torii/core/src/sql/query_queue.rs (1 hunks)
- crates/torii/core/src/sql/test.rs (5 hunks)
- crates/torii/core/src/sql/utils.rs (1 hunks)
- crates/torii/core/src/types.rs (2 hunks)
- crates/torii/graphql/src/tests/metadata_test.rs (3 hunks)
- crates/torii/graphql/src/tests/mod.rs (3 hunks)
- crates/torii/graphql/src/tests/subscription_test.rs (6 hunks)
- crates/torii/grpc/src/server/tests/entities_test.rs (3 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
- crates/torii/libp2p/src/tests.rs (2 hunks)
- crates/torii/migrations/20240913104418_add_erc.sql (1 hunks)
- crates/torii/migrations/20240918200125_rename_column_contracts_table.sql (1 hunks)
- scripts/deploy_erc20_katana.sh (1 hunks)
- scripts/send_erc20_transfer.sh (1 hunks)
Files not reviewed due to no reviewable changes (1)
- crates/torii/core/src/lib.rs
Files skipped from review due to trivial changes (3)
- crates/torii/core/src/processors/store_set_record.rs
- crates/torii/migrations/20240918200125_rename_column_contracts_table.sql
- scripts/deploy_erc20_katana.sh
Files skipped from review as they are similar to previous changes (11)
- bin/torii/src/main.rs
- bin/torii/torii.toml
- crates/torii/core/Cargo.toml
- crates/torii/core/src/processors/erc20_transfer.rs
- crates/torii/core/src/processors/erc721_transfer.rs
- crates/torii/core/src/sql/erc.rs
- crates/torii/core/src/sql/utils.rs
- crates/torii/core/src/types.rs
- crates/torii/libp2p/src/server/mod.rs
- crates/torii/migrations/20240913104418_add_erc.sql
- scripts/send_erc20_transfer.sh
Additional comments not posted (36)
crates/torii/core/src/processors/erc20_legacy_transfer.rs (5)
1-10: Ohayo sensei! The imports look good to me.The imported dependencies are relevant and necessary for the functionality implemented in this file. No unused imports detected.
12-15: Ohayo sensei! The constant and struct definitions are on point.The
LOG_TARGETconstant follows the naming convention and will be useful for logging. TheErc20LegacyTransferProcessorstruct is derived with appropriate traits.
17-34: Ohayo sensei! TheEventProcessortrait implementation looks solid.The
event_keymethod returns the correct key for ERC20 transfer events. Thevalidatemethod properly checks the event structure to ensure it matches the expected format for ERC20 transfers.
36-57: Ohayo sensei! Theprocessmethod implementation is top-notch.The method correctly extracts relevant data from the event, handling the Cairo serialization format for the value. The interaction with the database and logging of transaction details are implemented properly.
58-58: Ohayo sensei! The closing brace is in the right spot.The closing brace matches the opening brace of the
Erc20LegacyTransferProcessorstruct.crates/torii/core/src/processors/mod.rs (4)
12-17: Ohayo sensei! The uncommented module declarations look good.Making the
erc20_legacy_transfer,erc20_transfer, anderc721_transfermodules publicly accessible will allow them to be utilized in the processing logic. This change aligns with the objective of expanding functionality related to ERC20 and ERC721 token transfers.
82-82: Ohayo sensei! TheEventProcessors<P>type alias definition looks good.Defining
EventProcessors<P>asVec<Box<dyn EventProcessor<P>>>aligns with the changes made to thegenerate_event_processors_mapfunction. UsingBoxinstead ofArcindicates a shift in ownership semantics, allowing the function to own the event processors rather than using shared references.
86-87: Ohayo sensei! The modifications to thegenerate_event_processors_mapfunction look good.Changing the parameter type to
EventProcessors<P>and the return type toResult<HashMap<Felt, EventProcessors<P>>>allows multiple processors to be associated with a single event key. This enhances the flexibility of event handling and enables more complex event processing scenarios.
92-92: Ohayo sensei! The processor insertion logic looks good.Using
event_processors.entry(key).or_insert(vec![]).push(processor)ensures that a new vector is created for each event key if it doesn't already exist, and the processor is added to the vector. This allows multiple processors to be associated with a single event key, enhancing the flexibility of event handling.crates/torii/graphql/src/tests/metadata_test.rs (3)
3-4: Ohayo sensei! The import statement looks good to me.The
HashMapimport is valid and necessary for the code changes. Nice work!
106-106: Ohayo sensei! The modifiedSql::newinstantiation looks good here too.The changes are consistent with the updated constructor signature. Please refer to the previous verification script to ensure all occurrences of
Sql::newin the codebase match the new signature.
53-53: Ohayo sensei! The modifiedSql::newinstantiation looks good.The changes are consistent with the updated constructor signature. Just make sure to update all other occurrences of
Sql::newin the codebase to match the new signature.Run the following script to verify the constructor usage:
Verification successful
Ohayo sensei! I've thoroughly examined the usage of
Sql::newacross the codebase, and I'm pleased to report some good news!All instances of
Sql::neware consistently using the new signature.Here's a quick summary of my findings:
- 13 occurrences of
Sql::newwere found across different files.- All instances use the new three-parameter signature:
Sql::new(pool.clone(), <some_value>, &HashMap::new()).- The third parameter is consistently
&HashMap::new(), which aligns perfectly with the change we're verifying.No further action is required. The codebase appears to be in harmony with the new
Sql::newsignature. Excellent work, sensei!Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `Sql::new` match the new signature. # Test: Search for the constructor usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'Sql::new'Length of output: 7515
crates/torii/grpc/src/server/tests/entities_test.rs (4)
1-1: Ohayo sensei! The import statement looks good.The
HashMapimport is necessary for the code changes in this file.
108-109: Ohayo sensei! The addition of new event processors looks good.The
RegisterModelProcessorandStoreSetRecordProcessorare added correctly to the list of event processors.
121-121: Ohayo sensei! Thefetch_rangemethod call looks good.The additional
HashMapparameter is consistent with the method signature change.Run the following script to verify the method usage:
#!/bin/bash # Description: Verify all method calls to `fetch_range` match the new signature. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'fetch_range'
99-99: Ohayo sensei! TheSql::newfunction call looks good.The additional
HashMapparameter is consistent with the function signature change.Run the following script to verify the function usage:
Verification successful
Ohayo sensei! It seems our previous attempt didn't quite hit the mark. Let's try again with a corrected script to find what we're looking for!
Ohayo sensei! I've got some good news for you about our
Sql::newfunction!The
Sql::newfunction is being used correctly with the new signature throughout the codebase.After a thorough investigation, I can confirm that:
- All instances of
Sql::neware called with three parameters: pool, world_address (or Felt::ZERO), and a HashMap.- The new HashMap parameter is consistently passed as
&HashMap::new(), creating an empty HashMap in all cases.- There are no occurrences of the old signature (without the HashMap parameter) being used.
This consistency across the codebase indicates that the change has been properly implemented and there are no lingering issues with the function usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Sql::new` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'Sql::new'Length of output: 2280
Script:
#!/bin/bash # Description: Verify all function calls to `Sql::new` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 'Sql::new'Length of output: 7515
crates/torii/core/src/sql/query_queue.rs (1)
8-8: Ohayo sensei! The new import looks good.The
felt_to_sql_stringfunction from theutilsmodule will likely be used to convert Felt values, such as token IDs or addresses, into SQL-friendly string representations. This is a crucial step in indexing ERC20 and ERC721 tokens, as it allows for efficient storage and retrieval of token-related data in the database.By converting Felt values to SQL strings, you can streamline the process of constructing SQL queries and managing data interactions with the database when dealing with token indexing. Keep up the great work, sensei!
crates/torii/graphql/src/tests/mod.rs (3)
1-1: Ohayo sensei! The import looks good to me.The
HashMapimport fromstd::collectionsis valid and necessary for later usage.
354-354: Ohayo sensei! TheSqlinstantiation changes look good.The additional parameter, a reference to a new
HashMap, suggests improved configurability for theSqlinstance. This could enable more flexible state management or data handling. The changes are valid and do not introduce any apparent issues.
363-365: Ohayo sensei! The processor andfetch_rangechanges look valid, but please verify the performance impact.The shift from using
Arc::newtoBox::newfor processor instances indicates a change in ownership semantics and memory management. While this is a valid approach, it's worth verifying if this change has any unintended performance implications, considering the trade-offs between reference counting and heap allocation.Additionally, passing a new
HashMapreference toengine.fetch_rangealigns with the earlier modifications toSql::new, maintaining consistency.Overall, the changes appear correct, but it's prudent to assess the performance impact of the processor memory management change.
Also applies to: 373-373, 377-377
crates/torii/core/src/sql/test.rs (2)
131-131: Ohayo sensei! The code changes look good to me.The update to the
Sql::newfunction call, including the new parameter of a reference to an emptyHashMap, is a valid change that ensures a consistent empty state is passed to the function during initialization.
291-291: Ohayo sensei! The code changes look good to me.The updates to the
Sql::newfunction calls in bothtest_load_from_remote_delandtest_update_with_set_recordfunctions, including the new parameter of a reference to an emptyHashMap, are valid changes that ensure a consistent empty state is passed to the function during initialization.Also applies to: 381-381
crates/torii/graphql/src/tests/subscription_test.rs (2)
3-3: Ohayo sensei! TheHashMapimport looks good to me.The
HashMaptype is a useful data structure for storing key-value pairs and is commonly used in Rust projects. The import statement follows Rust's naming conventions and is valid.
25-25: Ohayo sensei! TheSqlinstantiation with the newHashMapparameter looks good.The additional
&HashMap::new()parameter allows for more flexibility in configuring theSqlobject. This change suggests that theSqlobject now supports additional configuration or metadata through theHashMapparameter.To ensure consistency, please run the following script to verify that all instantiations of the
Sqlobject have been updated with the new parameter:Verification successful
Ohayo sensei! The
Sql::newupdate is consistently applied across the codebase. Sugoi!All instantiations of
Sql::newnow include the newHashMapparameter, either as&HashMap::new()in test files or as&erc_contractsin the main application. This consistency ensures that the change is properly implemented throughout the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instantiations of the `Sql` object include the new `HashMap` parameter. # Test: Search for the `Sql::new` method usage. Expect: All occurrences to include the `HashMap` parameter. rg --type rust $'Sql::new\(' -A 1Length of output: 2776
crates/torii/core/src/engine.rs (9)
Line range hint
204-226: Ohayo sensei! The changes tofetch_datalook good to me.The function correctly handles fetching data for different scenarios based on the cursor head and latest block number. The logic is sound and the implementation is clean.
Line range hint
374-388: Ohayo sensei! Theprocessfunction looks good to me.It correctly dispatches the processing based on the type of fetched data. The logic is straightforward and easy to understand.
Line range hint
390-452: Ohayo sensei! Theprocess_pendingfunction looks solid.It correctly handles the processing of pending transactions, skipping the ones that have already been processed based on the cursor. The error handling for processing pending transactions is handled appropriately, with specific handling for the "TransactionHashNotFound" error.
The function also processes parallelized events and updates the database with the processed data.
Line range hint
454-493: Ohayo sensei! Theprocess_rangefunction looks great.It correctly processes transactions and blocks for a given range, handling the associated events and updating the database accordingly. Processing parallelized events and updating the database with the processed data is also handled correctly.
The database is executed in batches, which is a good performance optimization.
Line range hint
495-534: Ohayo sensei! Theprocess_tasksfunction is looking good.Using a semaphore to limit the number of concurrent tasks based on the configuration is a great way to control the parallelization. Spawning a task for each batch of parallelized events and processing them concurrently significantly improves performance.
The error handling for processing parallelized events is handled correctly, with errors being logged. Merging the results from each task into the main database ensures data consistency.
Line range hint
585-621: Ohayo sensei! Theprocess_transaction_with_receiptfunction looks solid.It correctly processes each event associated with the transaction if it belongs to the world contract or ERC tokens. Updating the cursor map for each processed event ensures proper tracking of the event processing progress.
Calling the
process_eventfunction to process each event with the necessary parameters is handled correctly.
Line range hint
667-749: Ohayo sensei! Theprocess_eventfunction is looking great.It correctly retrieves the appropriate event processors based on the event key and falls back to the catch-all event processor if no specific processor is found. This ensures that all events are handled, even if they don't have a dedicated processor.
Parallelizing the processing of certain events based on a task identifier is a smart optimization for improving performance for specific event types.
Processing the event using the retrieved event processors and handling errors with appropriate logging is implemented correctly.
752-777: Ohayo sensei! Theget_all_eventsfunction looks great.It correctly fetches events in chunks using pagination until all events are retrieved. Using a loop to continuously fetch event pages until the continuation token is
Noneensures that all events are fetched.Returning the contract address and the list of fetched event pages provides the necessary data for further processing.
112-113: Ohayo sensei! The changes to theEnginestruct look good.Adding the
tokensfield to store the ERC tokens to index is a useful addition. It allows the engine to focus on specific tokens of interest.The struct contains all the necessary fields and provides methods for the engine to function properly, including starting the engine, fetching data, processing data, and handling various types of events and transactions.
crates/torii/libp2p/src/tests.rs (1)
Line range hint
527-563: Ohayo sensei! The changes to thetest_client_messagingfunction look great!The test function thoroughly verifies the client messaging functionality by setting up the necessary components, registering the
Messagemodel, publishing a signed message, and waiting for it to be received within a timeout.A few additional insights:
- The use of an in-memory SQLite database ensures a clean and isolated testing environment.
- The Katana sequencer and JSON-RPC client provide a realistic setup for testing the messaging system.
- The test covers the essential aspects of message publishing, subscription, and reception.
- The timeout mechanism ensures that the test fails if the message is not received within the expected time frame.
Overall, this test function enhances the reliability and correctness of the client messaging system. Keep up the great work, sensei!
crates/torii/core/src/sql/mod.rs (2)
37-37: Ohayo sensei! Test module path update is acceptableChanging the test module's path to
test.rsimproves consistency. Ensure that all tests are still recognized and executed by the test runner.
125-132: Ohayo sensei! Update all calls tohead()to match the new signatureThe
headmethod now requires acontractparameter. Ensure that all calls to this method throughout the codebase have been updated to include the new parameter to prevent potential runtime errors.To ensure that all calls are updated, you can run the following script:
Verification successful
Ohayo sensei! Let's dig a bit deeper to make sure we're not missing anything. I'll run a couple more searches to verify the
head()method calls.
Ohayo sensei! I've found some interesting information, but let's double-check to make sure we're not missing anything important.
Ohayo sensei! After our thorough investigation, I have some good news for you!
The
head()method call has been correctly updatedWe found one occurrence of the
head()method being called incrates/torii/core/src/engine.rs, and it's already using the correct signature with a parameter:let (head, _, _) = self.db.head(self.world.address).await?;This call matches the new signature we were looking for. We didn't find any other calls to the
head()method in the codebase that would need updating.However, I couldn't locate the actual method definition for
head(). While this is a bit unusual, it doesn't affect our conclusion about the method calls being up-to-date.To be extra thorough, you might want to manually verify the
head()method definition and check if there are any other calls to this method that our search might have missed. But based on our findings, it appears that the necessary updates have been made.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `head()` that need to be updated. rg --type rust $'\.head\(\)'Length of output: 29
Script:
#!/bin/bash # Search for any occurrences of 'head' as a method call rg --type rust '\w+\.head\b' # Search for the 'head' method definition ast-grep --lang rust --pattern 'fn head($$$) $$$'Length of output: 605
Script:
#!/bin/bash # Check the context of the head() call we found rg --type rust -C 5 'self\.db\.head\(self\.world\.address\)' # Search for the head method definition with a more specific pattern ast-grep --lang rust --pattern 'pub async fn head(&self, contract: Felt) -> Result<$$$>'Length of output: 966
commit-id:5231a946
lambda-0x
force-pushed
the
spr/main/5231a946
branch
from
83ac6d1
to
3414843
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (26)
- bin/torii/src/main.rs (7 hunks)
- bin/torii/torii.toml (1 hunks)
- crates/torii/core/Cargo.toml (1 hunks)
- crates/torii/core/src/engine.rs (24 hunks)
- crates/torii/core/src/lib.rs (0 hunks)
- crates/torii/core/src/processors/erc20_legacy_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc20_transfer.rs (1 hunks)
- crates/torii/core/src/processors/erc721_transfer.rs (1 hunks)
- crates/torii/core/src/processors/mod.rs (2 hunks)
- crates/torii/core/src/processors/store_set_record.rs (1 hunks)
- crates/torii/core/src/sql/erc.rs (1 hunks)
- crates/torii/core/src/sql/mod.rs (9 hunks)
- crates/torii/core/src/sql/query_queue.rs (1 hunks)
- crates/torii/core/src/sql/test.rs (5 hunks)
- crates/torii/core/src/sql/utils.rs (1 hunks)
- crates/torii/core/src/types.rs (2 hunks)
- crates/torii/graphql/src/tests/metadata_test.rs (3 hunks)
- crates/torii/graphql/src/tests/mod.rs (3 hunks)
- crates/torii/graphql/src/tests/subscription_test.rs (6 hunks)
- crates/torii/grpc/src/server/tests/entities_test.rs (3 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
- crates/torii/libp2p/src/tests.rs (2 hunks)
- crates/torii/migrations/20240913104418_add_erc.sql (1 hunks)
- crates/torii/migrations/20240918200125_rename_column_contracts_table.sql (1 hunks)
- scripts/deploy_erc20_katana.sh (1 hunks)
- scripts/send_erc20_transfer.sh (1 hunks)
Files not reviewed due to no reviewable changes (1)
- crates/torii/core/src/lib.rs
Files skipped from review due to trivial changes (1)
- bin/torii/torii.toml
Files skipped from review as they are similar to previous changes (22)
- crates/torii/core/Cargo.toml
- crates/torii/core/src/processors/erc20_legacy_transfer.rs
- crates/torii/core/src/processors/erc20_transfer.rs
- crates/torii/core/src/processors/erc721_transfer.rs
- crates/torii/core/src/processors/mod.rs
- crates/torii/core/src/processors/store_set_record.rs
- crates/torii/core/src/sql/erc.rs
- crates/torii/core/src/sql/mod.rs
- crates/torii/core/src/sql/query_queue.rs
- crates/torii/core/src/sql/test.rs
- crates/torii/core/src/sql/utils.rs
- crates/torii/core/src/types.rs
- crates/torii/graphql/src/tests/metadata_test.rs
- crates/torii/graphql/src/tests/mod.rs
- crates/torii/graphql/src/tests/subscription_test.rs
- crates/torii/grpc/src/server/tests/entities_test.rs
- crates/torii/libp2p/src/server/mod.rs
- crates/torii/libp2p/src/tests.rs
- crates/torii/migrations/20240913104418_add_erc.sql
- crates/torii/migrations/20240918200125_rename_column_contracts_table.sql
- scripts/deploy_erc20_katana.sh
- scripts/send_erc20_transfer.sh
Additional comments not posted (12)
crates/torii/core/src/engine.rs (12)
9-9: Ohayo sensei! Thejoin_allimport looks good to me.This import is necessary for the changes made in the
fetch_rangefunction to concurrently fetch events from multiple contracts.
12-14: Ohayo sensei! The additional imports fromstarknet::core::typesare looking good.These imports are necessary for the changes made to the
Enginestruct and related functions to support indexing ERC tokens.
26-27: Ohayo sensei! The imports fromcrate::sqlandcrate::typesare looking good.These imports are necessary for the changes made to the
Enginestruct and related functions to support indexing ERC tokens and managing database cursors.
33-33: Ohayo sensei! The change to theeventfield in theProcessorsstruct is a great enhancement.Allowing multiple event processors to be associated with a single event key improves the flexibility of event handling. This enables more complex processing scenarios where different processors can handle the same event type in different ways.
82-82: Ohayo sensei! The comment about theLinkedListorder is a helpful addition.The comment clarifies that the blocks in the
LinkedListmight not be in a specific order. This information is valuable for developers working with theFetchRangeResultstruct to set the right expectations about the block order.
112-113: Ohayo sensei! The addition of thetokensfield to theEnginestruct is a key change.The
tokensfield allows the engine to store and manage the ERC tokens that need to be indexed. This is a fundamental requirement for the new functionality of indexing specific ERC tokens.
Line range hint
123-132: Ohayo sensei! The modification to theEngineconstructor to accept thetokensparameter is a necessary change.By accepting the
tokensparameter, the constructor allows the caller to provide the ERC tokens that need to be indexed. This ensures that the engine is initialized with the correct set of tokens to index.
Line range hint
204-298: Ohayo sensei! The changes in thefetch_rangefunction to fetch events concurrently are a significant performance improvement.By creating separate tasks for fetching events from each contract and using
join_allto execute them concurrently, the function can efficiently retrieve events from multiple contracts in parallel. This can greatly reduce the overall time required to fetch events, especially when dealing with a large number of contracts.The code segment also handles the processing of the fetched events, ensuring that already processed events are filtered out based on the last processed transaction for each contract. This prevents duplicate processing of events.
Overall, these changes enhance the performance and correctness of the event fetching process.
Line range hint
397-448: Ohayo sensei! The modifications to theprocess_pendingfunction enhance the processing of pending transactions and their events.The function now iterates over the transactions in the pending block and processes each transaction using the
process_transaction_with_receiptfunction. This ensures that each transaction is properly handled and its events are processed individually.The
last_pending_block_txandcursor_mapvariables are updated based on the processed transactions, allowing the function to keep track of the last processed transaction and the cursor for each contract. This is important for maintaining the correct state and avoiding duplicate processing.Error handling is also implemented to handle scenarios where a transaction receipt is not found. In such cases, the function sets the appropriate cursors and returns early, preventing the processing from being halted.
These changes improve the robustness and reliability of the pending transaction processing logic.
Line range hint
457-478: Ohayo sensei! The updates to theprocess_rangefunction enhance the processing of blocks and their transactions.The function now iterates over the transactions in the
data.transactionsmap and processes each transaction using theprocess_transaction_with_eventsfunction. This ensures that each transaction and its associated events are properly handled.Additionally, the function processes each block using the
process_blockfunction. To avoid processing the same block multiple times, it maintains aprocessed_blocksset. This optimization prevents unnecessary duplicate processing of blocks.If the
block_txchannel is provided, the function sends the block number through the channel. This allows other parts of the system to be notified about the processed blocks, enabling further actions or synchronization based on the processed blocks.These changes improve the efficiency and correctness of the block and transaction processing logic.
489-489: Ohayo sensei! The modification to reset the cursors in theprocess_rangefunction is a necessary update.After processing a range of blocks, it is important to reset the cursors to reflect the latest processed block. By calling the
reset_cursorsmethod of thedbobject with thelatest_block_number, the function ensures that the cursors are properly updated.This change maintains the consistency between the actual progress of block processing and the cursors stored in the database. It prevents any discrepancies and keeps the system in a valid state.
512-521: Ohayo sensei! The updates to theprocess_tasksfunction for handling parallelized events are a great addition.Processing parallelized events allows for efficient utilization of system resources by leveraging multiple event processors concurrently. This can significantly improve the throughput and performance of event processing.
The function iterates over the
processors.eventmap to retrieve the event processors associated with each event key. This ensures that each event is processed by its corresponding event processors, enabling customized processing logic for different event types.By calling the
processmethod on each event processor, the function triggers the specific processing logic defined for each event type. This flexibility allows for tailored handling of events based on their characteristics.Furthermore, the function logs any errors that occur during event processing using the
errormacro. This aids in identifying and debugging issues, making it easier to diagnose and fix problems in the event processing pipeline.Overall, these changes enhance the efficiency, customization, and observability of parallelized event processing.
| let contract_address = Felt::from_str(address).unwrap(); | ||
| let start_block = start_block.parse::<u64>()?; |
There was a problem hiding this comment.
Ohayo sensei! Let's handle those parsing errors gracefully.
Using unwrap when parsing the contract address can cause the application to panic if an invalid address is provided. It's better to propagate the error using the ? operator for a smoother experience.
Here's a suggestion:
- let contract_address = Felt::from_str(address).unwrap();
+ let contract_address = Felt::from_str(address)?;Apply the same change to line 365 for consistency.
Also applies to: 365-366
Usually, ERC1155 are used as consumables. A single token can have multiple copies owned by multiple accounts |
There was a problem hiding this comment.
For now due to legacy format of the tokens, we don't know if it's ERC721 or ERC20 for the transfer as only the selector of the event is used.
We should see how complex it is to leverage the token type (world/erc20/erc721) to actually scope the map of processors.
|
and don't forget that |
|
@rsodre thanks for pointing this out, we already handle this. |
Stack: