test(torii-indexer): add tests for erc tokens #3033
Conversation
Larkooo
changed the title
|
Ohayo, sensei! Here’s the detailed breakdown: WalkthroughThis pull request introduces multiple targeted changes. The test module now includes new functions for testing ERC20, ERC721, and ERC1155 token operations along with an updated signature for the bootstrap engine. The gRPC server code has been refactored to streamline SQL binding logic using a consolidated vector. In the examples, a new external function Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as ERC1155Token (ExternalImpl)
participant E as erc1155 updater
U->>T: transfer_from(from, to, token_id, value)
T->>E: update(transfer span)
E-->>T: update confirmation
T-->>U: transfer confirmation
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/test.rs (3)
238-267: Consider adding validation for failed transactions.Sensei, while the happy path testing is good, consider adding test cases for:
- Minting with insufficient permissions
- Transfer with insufficient balance
- Transfer to invalid addresses
293-319: Consider adding decimal handling validation.The test verifies token properties and balance, but it might be good to add explicit tests for:
- Balance formatting with different decimal places
- Handling of very large numbers
assert_eq!(token.decimals, 18); + +// Test balance formatting with decimals +let formatted_balance = format_with_decimals(balance, token.decimals); +assert_eq!(formatted_balance, "1000000000000111444.444");
444-446: Consider addressing the TODO comment.Ohayo sensei! The TODO comment about chronological testing with Torii re-syncing seems important for ensuring data consistency. Would you like me to help implement this test case?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(3 hunks)
🔇 Additional comments (3)
crates/torii/indexer/src/test.rs (3)
16-16: LGTM! Clean import additions.Ohayo! The new imports for
U256andTokentypes are properly organized and align well with the ERC20 testing requirements.Also applies to: 25-25
208-237: LGTM! Well-structured test setup.The test setup follows the established pattern from other tests in the file, maintaining consistency in the testing approach.
285-285:✅ Verification successful
Verify ERC20 contract type usage.
The contract is registered with
ContractType::ERC20. Let's verify that this type is properly handled throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for ERC20 contract type handling rg -A 5 "ContractType::ERC20"Length of output: 3595
Ohayo, Sensei — ERC20 Usage Confirmed
After searching the codebase, the usage of
ContractType::ERC20is consistent and correctly handled across multiple components:
- CLI Parsing: Seen in
crates/torii/cli/src/args.rswhere contracts are registered.- SQLite Operations: Verified in both caching (
crates/torii/sqlite/src/erc.rs) and type conversions (crates/torii/sqlite/src/types.rs).- Event Processing: Confirmed in the executor (
crates/torii/sqlite/src/executor/erc.rs) and the indexer engine (crates/torii/indexer/src/engine.rs).- Testing: The snippet in
crates/torii/indexer/src/test.rsusesContractType::ERC20as expected.No issues were found regarding the handling of ERC20 contracts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/indexer/src/test.rs (4)
209-321: Ohayo! Consider adding edge cases to strengthen the ERC20 test coverage, sensei.The test covers basic minting and transfer operations well, but could be more robust with additional test cases:
- Minting zero tokens
- Transferring zero tokens
- Transferring more tokens than available balance
640-782: Consider testing with non-sequential token IDs, sensei.While the test is thorough, using sequential token IDs (1..=5) might not catch edge cases. Consider testing with arbitrary token IDs to ensure the contract handles them correctly.
- for token_id in 1..=5 { + // Test with non-sequential token IDs + for token_id in [1, 100, 1000, 10000, 100000] {
784-939: Ohayo! Let's enhance ERC1155 test coverage with batch operations and URI tests, sensei.The test covers single token operations well, but ERC1155 has unique features that should be tested:
- Batch transfers (transferring multiple token types in one transaction)
- URI functionality for token metadata
Would you like me to provide example code for testing batch transfers and URI functionality?
209-939: Ohayo! Let's reduce code duplication with helper functions, sensei.Consider extracting common patterns into helper functions:
- Database setup and initialization
- Token property verification
- Balance checking logic
Example helper function:
async fn setup_test_db() -> (SqlitePool, Executor, Sender) { let tempfile = NamedTempFile::new().unwrap(); let path = tempfile.path().to_string_lossy(); let options = SqliteConnectOptions::from_str(&path).unwrap().create_if_missing(true); let pool = SqlitePoolOptions::new().connect_with(options).await.unwrap(); sqlx::migrate!("../migrations").run(&pool).await.unwrap(); let (shutdown_tx, _) = broadcast::channel(1); let (executor, sender) = Executor::new(pool.clone(), shutdown_tx, Arc::clone(&provider), 100).await.unwrap(); (pool, executor, sender) }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/test.rs (3)
209-321: Ohayo! Consider adding more test cases for ERC20 operations, sensei.The test covers basic mint and transfer operations well, but could be enhanced with additional scenarios:
- Zero amount transfers
- Maximum token amount tests
- Failed transfer scenarios (insufficient balance)
640-797: Ohayo! Consider adding approval testing for NFT transfers, sensei.The test thoroughly covers minting and direct transfers, but could be enhanced with:
- Approval and transferFrom scenarios
- Testing transfer to zero address
- Testing unauthorized transfers
799-960: Ohayo! Consider adding batch operation tests for ERC1155, sensei.The test covers single token operations well, but ERC1155 is designed for batch operations. Consider adding:
- Batch mint testing
- Batch transfer testing
- Batch balance checks
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(4 hunks)
🔇 Additional comments (1)
crates/torii/indexer/src/test.rs (1)
962-975: Ohayo! The helper function looks great, sensei!The count_table function is well-documented and correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/indexer/src/test.rs (4)
209-324: Consider adding more edge cases to the ERC20 test, sensei!While the test covers basic minting and transfer operations well, consider adding these scenarios:
- Transfer amount exceeding balance
- Transfer to zero address
- Transfer of zero amount
643-800: Enhance NFT test robustness, sensei!The test covers happy paths well, but consider adding these error scenarios:
- Attempt to transfer a non-existent token
- Attempt to transfer a token you don't own
- Attempt to mint a token with an existing ID
802-966: Add batch operation tests for ERC1155, sensei!The current implementation tests individual token operations well. Consider adding:
- Batch mint test (multiple tokens in one transaction)
- Batch transfer test (multiple tokens in one transaction)
- Test for batch balance queries
209-966: Add documentation for the test functions, sensei!While the test implementations are solid, adding documentation would improve maintainability:
- Add doc comments explaining the test strategy
- Document the test setup requirements
- Explain the significance of the test values used
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (1)
crates/torii/indexer/src/test.rs (1)
12-12: Ohayo! The new imports look good, sensei!The added imports are well-chosen and necessary for handling token operations and numeric conversions.
Also applies to: 17-17, 26-26
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/test.rs (3)
242-242: Consider moving balance initialization closer to its first usage, sensei.The
balancevariable is initialized far from its first usage. Consider moving it closer to line 256 where it's first used.
209-324: Consider adding edge case tests, sensei!The test covers basic minting and transfer scenarios but could be more comprehensive by adding tests for:
- Zero amount transfers
- Transfers exceeding balance
- Transfers to zero address
753-775: Verify original owner's balance for transferred tokens, sensei!While checking the recipient's balance is good, consider also verifying that the original owner's balance for transferred tokens (IDs 1 and 2) is zero. This ensures complete ownership transfer.
+ // Check original owner's balance for transferred tokens + for token_id in 1..=2 { + let balance = sqlx::query_scalar::<_, String>( + format!( + "SELECT balance FROM token_balances WHERE account_address = '{:#x}' AND \ + contract_address = '{:#x}' AND token_id = '{:#x}'", + account.address(), + badge_address, + Felt::from(token_id) + ) + .as_str(), + ) + .fetch_one(&pool) + .await + .unwrap(); + + let balance = crypto_bigint::U256::from_be_hex(balance.trim_start_matches("0x")); + assert_eq!( + U256::from(balance), + U256::from(0u8), + "Original owner should have zero balance for transferred tokens" + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (2)
crates/torii/indexer/src/test.rs (2)
12-12: Ohayo! The imports look good, sensei!The new imports are well-chosen and necessary for the token-related test functions.
Also applies to: 17-17, 26-26
802-972: Excellent implementation of ERC1155 tests, sensei!The test is comprehensive and well-structured:
- Tests multiple token IDs with varying amounts
- Verifies balances for both sender and recipient
- Includes proper token metadata verification
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/test.rs (2)
505-678: Excellent ERC1155 test coverage, sensei!The test comprehensively validates:
- Minting different amounts for various token IDs
- Transfer operations with amount splitting
- Balance verification for both sender and recipient
However, consider adding a test case for batch transfers since ERC1155 supports them.
// Add after line 578 + // Test batch transfer functionality + let batch_tx = &account + .execute_v1(vec![Call { + to: rewards_address, + selector: get_selector_from_name("batch_transfer").unwrap(), + calldata: vec![ + Felt::ONE, // to + Felt::ZERO, + Felt::from(3), // number of transfers + // Transfer details for token 1 + Felt::from(1), + Felt::ZERO, + Felt::from(10), + // Transfer details for token 2 + Felt::from(2), + Felt::ZERO, + Felt::from(20), + // Transfer details for token 3 + Felt::from(3), + Felt::ZERO, + Felt::from(30), + ], + }]) + .send() + .await + .unwrap(); + + TransactionWaiter::new(batch_tx.transaction_hash, &provider).await.unwrap();
805-807: Consider addressing the TODO comment.The comment suggests a need for chronological testing with Torii re-syncing.
Would you like me to help implement a test that validates intermediate states during re-syncing?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(9 hunks)
🔇 Additional comments (3)
crates/torii/indexer/src/test.rs (3)
31-59: LGTM! Clean modification to support ERC token testing.The changes to
bootstrap_enginefunction properly integrate contract information into the engine initialization.
212-341: Ohayo sensei! The ERC20 test implementation looks solid.The test thoroughly validates token minting, transfer operations, and balance tracking. Good job on including balance verification after each operation.
343-503: Nice work on the ERC721 test implementation, sensei!The test effectively covers:
- Multiple NFT minting
- Token transfers
- Balance verification for both transferred and non-transferred tokens
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (17)
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/utils/layout.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(1 hunks)crates/dojo/core-cairo-test/src/world.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(2 hunks)crates/dojo/core/src/storage/packing.cairo(1 hunks)crates/dojo/core/src/utils/snf_test.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/torii/indexer/src/test.rs(8 hunks)examples/simple/src/lib.cairo(1 hunks)examples/spawn-and-move/src/actions.cairo(3 hunks)
✅ Files skipped from review due to trivial changes (14)
- crates/dojo/core/src/utils/snf_test.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/dojo/core-cairo-test/src/tests/world/world.cairo
- crates/dojo/core-cairo-test/src/tests/benchmarks.cairo
- crates/dojo/core/src/storage/packing.cairo
- examples/spawn-and-move/src/actions.cairo
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
- crates/dojo/core-cairo-test/src/world.cairo
- crates/dojo/core-cairo-test/src/tests/utils/layout.cairo
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- examples/simple/src/lib.cairo
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/test.rs
[warning] 52-52: Code formatting issues detected. Please ensure proper formatting.
[warning] 129-129: Code formatting issues detected. Please ensure proper formatting.
[warning] 277-277: Code formatting issues detected. Please ensure proper formatting.
[warning] 395-395: Code formatting issues detected. Please ensure proper formatting.
[warning] 573-573: Code formatting issues detected. Please ensure proper formatting.
[warning] 741-741: Code formatting issues detected. Please ensure proper formatting.
[warning] 858-858: Code formatting issues detected. Please ensure proper formatting.
[warning] 957-957: Code formatting issues detected. Please ensure proper formatting.
🔇 Additional comments (8)
crates/dojo/core/src/world/world_contract.cairo (2)
3-18: Ohayo! Clean and well-structured implementation, sensei!The
Permissionenum and itsDisplaytrait implementation are well-designed with clear variants and proper formatting.
725-730: Ohayo! Clean salt computation, sensei!The salt computation elegantly combines the transaction hash and namespace hash to ensure uniqueness, preventing duplicate class hash registrations in the same namespace.
crates/dojo/core/src/meta/introspect.cairo (2)
24-315: Ohayo! The type upgrade matrix looks great, sensei!The restructured
ALLOWED_PRIMITIVE_UPGRADESconstant is now more maintainable with proper indentation and clear type annotations in comments.
811-823: Clean formatting for Option introspection, sensei!The improved formatting makes the code more readable while maintaining the same functionality.
crates/torii/indexer/src/test.rs (4)
31-61: Ohayo sensei! The bootstrap_engine changes look good!The function has been enhanced with proper mutability and contract handling. The flush and cache operations are correctly implemented.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 52-52: Code formatting issues detected. Please ensure proper formatting.
214-324: Excellent ERC20 test implementation, sensei!The test thoroughly covers:
- Token minting and transfer operations
- Balance tracking
- Token metadata verification
🧰 Tools
🪛 GitHub Actions: ci
[warning] 277-277: Code formatting issues detected. Please ensure proper formatting.
326-486: Well-structured ERC721 test, sensei!The test effectively validates:
- Multiple NFT minting
- Token transfers
- Balance verification for both transferred and non-transferred tokens
🧰 Tools
🪛 GitHub Actions: ci
[warning] 395-395: Code formatting issues detected. Please ensure proper formatting.
488-661: Comprehensive ERC1155 test implementation!The test thoroughly covers:
- Batch minting with different amounts
- Transfer operations
- Balance verification for both sender and recipient
🧰 Tools
🪛 GitHub Actions: ci
[warning] 573-573: Code formatting issues detected. Please ensure proper formatting.
| @@ -46,12 +47,14 @@ where | |||
| EngineConfig::default(), | |||
| shutdown_tx, | |||
| None, | |||
| &[Contract { address: world_address, r#type: ContractType::WORLD }], | |||
| contracts, | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ohayo sensei! Please address the formatting issues in async blocks.
The CI pipeline has detected formatting issues in multiple async blocks. Consider running the formatter to resolve these issues.
Also applies to: 129-129, 277-277, 395-395, 573-573, 741-741, 858-858, 957-957
🏁 Script executed:
#!/bin/bash
# Check formatting issues in the specified lines
rg -A 2 -B 2 "async move" crates/torii/indexer/src/test.rsLength of output: 1555
Ohayo sensei! Please fix the formatting in your async blocks.
The CI pipeline has flagged several formatting issues in the tokio::spawn(async move { ... }) constructs within the file. Specifically, the issues occur at these lines in crates/torii/indexer/src/test.rs:
- Line 52
- Line 129
- Line 277
- Line 395
- Line 573
- Line 741
- Line 858
- Line 957
Kindly run the formatter (e.g., rustfmt) to automatically correct these inconsistencies. Once these changes are applied, the code should meet our formatting standards.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 52-52: Code formatting issues detected. Please ensure proper formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
816-850: Consider adding error handling for empty bind values, sensei.While the code works correctly, it might be good to add a check for empty bind values to provide better error messages.
async fn retrieve_tokens( &self, contract_addresses: Vec<Felt>, token_ids: Vec<U256>, ) -> Result<RetrieveTokensResponse, Status> { let mut query = "SELECT * FROM tokens".to_string(); let mut bind_values = Vec::new(); let mut conditions = Vec::new(); + + // Early return with empty response if no filters are provided + if contract_addresses.is_empty() && token_ids.is_empty() { + return Ok(RetrieveTokensResponse { tokens: Vec::new() }); + } if !contract_addresses.is_empty() {
852-894: Similar improvement could be made here too, sensei!The
retrieve_token_balancesfunction could benefit from the same early return pattern for empty inputs.async fn retrieve_token_balances( &self, account_addresses: Vec<Felt>, contract_addresses: Vec<Felt>, token_ids: Vec<U256>, ) -> Result<RetrieveTokenBalancesResponse, Status> { let mut query = "SELECT * FROM token_balances".to_string(); let mut bind_values = Vec::new(); let mut conditions = Vec::new(); + + // Early return with empty response if no filters are provided + if account_addresses.is_empty() && contract_addresses.is_empty() && token_ids.is_empty() { + return Ok(RetrieveTokenBalancesResponse { balances: Vec::new() }); + } if !account_addresses.is_empty() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
841-843: Ohayo! The code looks good, sensei!The iteration over bind values is clean and straightforward. The modification maintains the same functionality while being more readable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/test.rs (2)
327-489: Ohayo sensei! Consider extracting balance verification logic.The test is comprehensive but contains duplicate code for balance verification. Consider extracting the balance check logic into a helper function.
+ async fn verify_nft_balance( + pool: &sqlx::Pool<sqlx::Sqlite>, + account_address: Felt, + badge_address: Felt, + token_id: u32, + expected_balance: u8, + message: &str, + ) -> Result<(), Box<dyn std::error::Error>> { + let balance = sqlx::query_scalar::<_, String>( + format!( + "SELECT balance FROM token_balances WHERE account_address = '{:#x}' AND \ + contract_address = '{:#x}' AND token_id = '{:#x}:{}'", + account_address, + badge_address, + badge_address, + u256_to_sql_string(&U256::from(token_id)) + ) + .as_str(), + ) + .fetch_one(pool) + .await?; + + let balance = crypto_bigint::U256::from_be_hex(balance.trim_start_matches("0x")); + assert_eq!( + U256::from(balance), + U256::from(expected_balance), + "{}", + message + ); + Ok(()) + }Then use it in the test:
- let balance = sqlx::query_scalar::<_, String>(...) - .fetch_one(&pool) - .await - .unwrap(); - let balance = crypto_bigint::U256::from_be_hex(balance.trim_start_matches("0x")); - assert_eq!( - U256::from(balance), - U256::from(0u8), - "Sender should have balance of 0 for transferred tokens" - ); + verify_nft_balance( + &pool, + account.address(), + badge_address, + token_id, + 0, + "Sender should have balance of 0 for transferred tokens", + ) + .await + .unwrap();
491-667: Ohayo sensei! Similar balance verification pattern detected.The ERC1155 test could benefit from a similar helper function for balance verification. Consider creating a generic balance verification helper that works for both ERC721 and ERC1155 tokens.
+ async fn verify_token_balance<T: ToString>( + pool: &sqlx::Pool<sqlx::Sqlite>, + account_address: Felt, + contract_address: Felt, + token_id: T, + expected_balance: u32, + message: &str, + ) -> Result<(), Box<dyn std::error::Error>> { + let balance = sqlx::query_scalar::<_, String>( + format!( + "SELECT balance FROM token_balances WHERE account_address = '{:#x}' AND \ + contract_address = '{:#x}' AND token_id = '{:#x}:{}'", + account_address, + contract_address, + contract_address, + u256_to_sql_string(&U256::from(token_id.to_string().parse::<u32>().unwrap())) + ) + .as_str(), + ) + .fetch_one(pool) + .await?; + + let balance = crypto_bigint::U256::from_be_hex(balance.trim_start_matches("0x")); + assert_eq!( + U256::from(balance), + U256::from(expected_balance), + "{}", + message + ); + Ok(()) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(8 hunks)
🔇 Additional comments (3)
crates/torii/indexer/src/test.rs (3)
12-12: Ohayo sensei! The import changes and bootstrap_engine modifications look good!The new imports and bootstrap_engine parameter changes are well-aligned with the token functionality requirements.
Also applies to: 17-17, 26-27, 34-34, 35-35, 36-36
215-325: Ohayo sensei! The ERC20 test implementation is thorough!The test covers essential ERC20 operations:
- Token minting and transfers
- Balance tracking
- Token metadata verification
129-131: Ohayo sensei! Please fix the formatting in your async blocks.The CI pipeline has flagged several formatting issues in the
tokio::spawn(async move { ... })constructs.Also applies to: 277-279, 395-397, 573-578, 741-748, 858-865, 957-964
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/spawn-and-move/src/externals/erc1155.cairo (2)
70-74: Ohayo sensei! Consider adding parameter validation.The implementation looks good, but consider adding validation for the
valueparameter to ensure it's not zero and that the sender has sufficient balance.Apply this diff to add parameter validation:
#[external(v0)] fn transfer_from(ref self: ContractState, from: ContractAddress, to: ContractAddress, token_id: u256, value: u256) { + assert(value > 0, 'Value must be greater than 0'); + let balance = self.erc1155.balance_of(from, token_id); + assert(balance >= value, 'Insufficient balance'); self.erc1155.update(from, to, array![token_id].span(), array![value].span()); // safe transfer from does not support default account since they dont implement receiver. }
73-73: Ohayo sensei! Please enhance the comment for better clarity.The comment about safe transfer could be more descriptive to help users understand why it's not supported and what alternatives are available.
Apply this diff to improve the comment:
- // safe transfer from does not support default account since they dont implement receiver. + // Note: Safe transfer is not used here because default accounts don't implement the ERC1155 receiver interface. + // This implementation uses a basic transfer mechanism that doesn't check if the recipient can handle the tokens.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/test.rs(8 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
examples/spawn-and-move/src/externals/erc1155.cairo
[warning] 68-68: Function 'transfer_from' has inconsistent formatting. Consider using a consistent style for function parameters.
🔇 Additional comments (3)
crates/torii/indexer/src/test.rs (3)
206-307: Ohayo sensei! The ERC20 test looks good!The test thoroughly validates ERC20 token operations, including minting, transferring, and balance verification.
309-462: Ohayo sensei! The ERC721 test is well-structured!The test provides comprehensive coverage of ERC721 token operations, including minting multiple NFTs, transferring tokens, and verifying balances for both transferred and non-transferred tokens.
464-633: Ohayo sensei! The ERC1155 test is comprehensive!The test thoroughly validates ERC1155 token operations, including minting different amounts for various token IDs, transferring tokens, and verifying balances for both the sender and recipient.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/spawn-and-move/src/actions.cairo(1 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/src/actions.cairo
🧰 Additional context used
🪛 GitHub Actions: ci
examples/spawn-and-move/src/externals/erc1155.cairo
[warning] 94-94: No newline at end of file.
| @@ -81,4 +94,4 @@ mod ERC1155Token { | |||
| // self.erc1155.batch_mint_with_acceptance_check(account, token_ids, values, data); | |||
| } | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Ohayo! Quick formatting fix needed, sensei! ✨
The pipeline is warning about a missing newline at the end of the file.
Add a newline after the closing brace:
}
+📝 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.
| } | |
| } | |
| #[external(v0)] | ||
| fn transfer_from( | ||
| ref self: ContractState, | ||
| from: ContractAddress, | ||
| to: ContractAddress, | ||
| token_id: u256, | ||
| value: u256, | ||
| ) { | ||
| self.erc1155.update(from, to, array![token_id].span(), array![value].span()); | ||
| // safe transfer from does not support default account since they dont implement | ||
| // receiver. | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Consider adding access control checks, sensei! 🛡️
The transfer_from implementation looks clean, but it might need some access control to ensure only authorized addresses can initiate transfers. Consider checking if the caller is either the token owner or an approved operator.
Here's a suggested implementation:
#[external(v0)]
fn transfer_from(
ref self: ContractState,
from: ContractAddress,
to: ContractAddress,
token_id: u256,
value: u256,
) {
+ let caller = starknet::get_caller_address();
+ assert(
+ from == caller || self.erc1155.is_approved_for_all(from, caller),
+ 'ERC1155: caller is not owner nor approved'
+ );
self.erc1155.update(from, to, array![token_id].span(), array![value].span());
// safe transfer from does not support default account since they dont implement
// receiver.
}📝 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.
| #[external(v0)] | |
| fn transfer_from( | |
| ref self: ContractState, | |
| from: ContractAddress, | |
| to: ContractAddress, | |
| token_id: u256, | |
| value: u256, | |
| ) { | |
| self.erc1155.update(from, to, array![token_id].span(), array![value].span()); | |
| // safe transfer from does not support default account since they dont implement | |
| // receiver. | |
| } | |
| #[external(v0)] | |
| fn transfer_from( | |
| ref self: ContractState, | |
| from: ContractAddress, | |
| to: ContractAddress, | |
| token_id: u256, | |
| value: u256, | |
| ) { | |
| let caller = starknet::get_caller_address(); | |
| assert( | |
| from == caller || self.erc1155.is_approved_for_all(from, caller), | |
| 'ERC1155: caller is not owner nor approved' | |
| ); | |
| self.erc1155.update(from, to, array![token_id].span(), array![value].span()); | |
| // safe transfer from does not support default account since they dont implement | |
| // receiver. | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/test.rs (2)
206-307: Ohayo sensei! The ERC20 test looks good but could use more edge cases.The test effectively verifies basic ERC20 functionality, including minting and transfers. Consider adding tests for:
- Zero amount transfers
- Transfers exceeding balance
- Multiple transfers to different accounts
464-633: Ohayo sensei! The ERC1155 test is comprehensive but could be more DRY.The test effectively verifies multi-token functionality. However, there's some code duplication in the balance checking logic.
Consider extracting the balance checking logic into a helper function:
async fn check_token_balance( pool: &sqlx::Pool<sqlx::Sqlite>, account: &Felt, contract: &Felt, token_id: u32, expected_balance: u32, message: &str, ) { let balance = sqlx::query_scalar::<_, String>( format!( "SELECT balance FROM token_balances WHERE account_address = '{:#x}' AND \ contract_address = '{:#x}' AND token_id = '{:#x}:{}'", account, contract, contract, u256_to_sql_string(&U256::from(token_id)) ) .as_str(), ) .fetch_one(pool) .await .unwrap(); let balance = crypto_bigint::U256::from_be_hex(balance.trim_start_matches("0x")); assert_eq!( U256::from(balance), U256::from(expected_balance), "{}", message ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/test.rs(8 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/src/externals/erc1155.cairo
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (3)
crates/torii/indexer/src/test.rs (3)
32-62: LGTM! The bootstrap_engine function has been updated correctly.The function signature has been enhanced to accept contracts as a parameter, and the db parameter is now mutable to reflect state changes. The implementation properly handles the new parameters.
309-462: Ohayo sensei! The ERC721 test is well-structured.The test thoroughly covers NFT minting and transfers, with good balance verification. The sequential token ID testing is particularly good.
129-129: Ohayo sensei! Please address the formatting issues in async blocks.The CI pipeline has detected formatting issues in multiple async blocks. Consider running the formatter to resolve these issues.
Also applies to: 268-270, 377-379, 551-553
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/test.rs (2)
309-462: Ohayo sensei! The ERC721 test is well-structured!The test comprehensively covers NFT operations:
- Minting multiple tokens
- Transfer verification
- Balance checks for both transferred and non-transferred tokens
However, consider adding tests for edge cases:
- Test minting with token ID 0
- Test transferring to zero address
- Test transferring non-existent tokens
464-632: Ohayo sensei! The ERC1155 test is comprehensive!The test effectively covers multi-token operations:
- Minting different amounts for various token IDs
- Batch transfers
- Balance verification for both sender and recipient
One suggestion to enhance the test coverage:
Consider adding tests for:
- Batch minting operation
- Transfer to zero address
- Transfer of non-existent token IDs
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: ensure-wasm
🔇 Additional comments (4)
crates/torii/indexer/src/test.rs (4)
32-37: Ohayo sensei! The bootstrap_engine function update looks good!The addition of the
contractsparameter enhances flexibility in contract management during engine initialization.Also applies to: 51-51
206-307: Ohayo sensei! The ERC20 test implementation is thorough!The test covers essential ERC20 operations:
- Token minting
- Token transfer
- Balance verification
- Token metadata validation
715-715: Ohayo sensei! The contract initialization is consistent across tests!The contract type specification is uniform across all test functions, maintaining code consistency.
Also applies to: 823-823, 913-913
268-270: Ohayo sensei! Please address the formatting issues in async blocks.The formatting in
tokio::spawn(async move { ... })blocks needs to be fixed.Also applies to: 377-379, 550-552, 711-713, 819-821, 909-911
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3033 +/- ##
==========================================
+ Coverage 56.29% 57.67% +1.38%
==========================================
Files 437 437
Lines 58950 59215 +265
==========================================
+ Hits 33184 34154 +970
+ Misses 25766 25061 -705 ☔ View full report in Codecov by Sentry. |
Larkooo
changed the title
Summary by CodeRabbit
New Features
Testing
Refactor