feat: add ERC1155 support with balance fetching and transfer function… #133
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request introduces comprehensive support for ERC-1155 tokens in the Dart-based blockchain library. The changes include adding a new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
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: 3
🧹 Nitpick comments (9)
test/ci/evm/evm_rpc_test.dart (2)
73-94: Optimize batch balance test efficiencyThe test uses duplicate wallet addresses and could be more efficient.
Consider refactoring:
test('test erc1155 batch balance of tokens', () async { + final testAccounts = List.generate( + 5, + (i) => "0x${i.toRadixString(16).padLeft(40, '0')}" + ); + final testTokenIds = List.generate( + 5, + (i) => BigInt.from(i) + ); + final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( - accounts: [ - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet - ], - tokenIDs: [ - BigInt.from(0), - BigInt.from(1), - BigInt.from(2), - BigInt.from(3), - BigInt.from(4) - ], + accounts: testAccounts, + tokenIDs: testTokenIds, contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", ); print('Balances: $balances'); expect(balances, isNotNull); + expect(balances.length, equals(5)); + expect(balances.every((b) => b >= BigInt.zero), isTrue); });
96-104: Add error handling test for URI fetchingThe URI test lacks error handling cases.
Add error case testing:
test('test uri of erc1155 tokens', () async { + // Test successful case final uri = await ethereumRPC.fetchERC1155UriOfToken( tokenID: BigInt.from(1), contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa", ); print('URI: $uri'); expect(uri, isNotNull); + expect(uri, contains('{id}')); // Most ERC1155 implementations use {id} placeholder + + // Test error case with invalid contract + expect( + () => ethereumRPC.fetchERC1155UriOfToken( + tokenID: BigInt.from(1), + contractAddress: "0x0000000000000000000000000000000000000000", + ), + throwsException, + ); });lib/src/domain/entities/coin_entity.dart (1)
150-177: Add documentation and validation to ERC1155EntityThe implementation looks good but could benefit from documentation and validation.
Consider these improvements:
+/// Represents an ERC1155 token with its contract address and token ID. +/// ERC1155 tokens are multi-token standard that can represent both +/// fungible and non-fungible tokens in a single contract. class ERC1155Entity extends EvmCoinEntity { final String contractAddress; final BigInt tokenId; + /// Creates a new ERC1155Entity instance. + /// + /// [name] - The name of the token collection + /// [symbol] - The symbol of the token collection + /// [chainID] - The chain ID where the contract is deployed + /// [contractAddress] - The address of the ERC1155 contract + /// [tokenId] - The ID of the specific token within the contract const ERC1155Entity({ required super.name, required super.symbol, required super.chainID, required this.contractAddress, required this.tokenId, - }) : super(decimals: 0); + }) : assert(contractAddress.length == 42, 'Invalid contract address length'), + assert(contractAddress.startsWith('0x'), 'Contract address must start with 0x'), + assert(tokenId >= BigInt.zero, 'Token ID must be non-negative'), + super(decimals: 0); + /// Returns the lowercase version of the contract address String get lowerCaseAddress => contractAddress.toLowerCase(); + @override + String get identifier => "$name:$symbol:$decimals:$contractAddress:$tokenId:$chainID"; @override int get hashCode => lowerCaseAddress.hashCode ^ chainID.hashCode ^ tokenId.hashCode; @override bool operator ==(Object other) { if (identical(this, other)) return true; return other is ERC1155Entity && other.lowerCaseAddress == lowerCaseAddress && other.chainID == chainID && other.tokenId == tokenId; } + + @override + String toString() => 'ERC1155($symbol #$tokenId)'; }lib/src/crypto/evm/entities/abi/erc/erc1155.dart (2)
330-353: Consider adding input validation for account addresses.While the length validation in
balanceOfBatchis good, consider adding validation for account addresses to ensure they are valid Ethereum addresses.Apply this diff to add address validation:
Future<BigInt> balanceOf({ required String account, required BigInt tokenID, }) async { + if (!isValidAddress(account)) { + throw ArgumentError('Invalid account address'); + } final function = abi.functions[0]; final response = await readSafe( function: function.addValues(values: [account, tokenID]), ); return response.outputs.first.castValue<BigInt>(); } Future<List<BigInt>> balanceOfBatch({ required List<String> accounts, required List<BigInt> tokenIDs, }) async { if (accounts.length != tokenIDs.length) { throw ArgumentError('accounts and tokenIDs must have the same length'); } + if (!accounts.every(isValidAddress)) { + throw ArgumentError('Invalid account address in the list'); + } final function = abi.functions[1]; final response = await readSafe( function: function.addValues(values: [accounts, tokenIDs]), ); return response.outputs.first.castValue<List<BigInt>>(); }
365-384: Consider adding input validation for addresses in safeTransferFrom.Add validation for sender and recipient addresses to ensure they are valid Ethereum addresses.
Apply this diff to add address validation:
Future<String> safeTransferFrom({ required String sender, required String to, required BigInt tokenID, required BigInt amount, required Uint8List seed, Uint8List? data, EvmFeeInformation? feeInfo, List<AccessListItem>? accessList, }) async { + if (!isValidAddress(sender)) { + throw ArgumentError('Invalid sender address'); + } + if (!isValidAddress(to)) { + throw ArgumentError('Invalid recipient address'); + } final function = abi.functions[4]; return await interact( function: function.addValues( values: [sender, to, tokenID, amount, data ?? Uint8List(0)]), sender: sender, seed: seed, feeInfo: feeInfo, accessList: accessList, ); }lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (4)
109-124: Consider adding documentation for the return value.The method is well-implemented, but the documentation should clarify what the returned balance represents (e.g., number of tokens owned).
Apply this diff to enhance the documentation:
/// /// Fetch Balance of ERC1155 Token /// +/// Returns the number of tokens of token type `tokenID` owned by `account`. +/// The balance is returned as a [BigInt] to handle large numbers accurately. Future<BigInt> fetchERC1155BalanceOfToken({
129-145: Consider adding documentation for error cases.The method relies on
balanceOfBatchwhich throws an error for length mismatch. This should be documented.Apply this diff to enhance the documentation:
/// /// Fetch Batch Balance of ERC1155 Tokens /// +/// Returns a list of token balances for the given accounts and token IDs. +/// +/// Throws [ArgumentError] if: +/// - The length of [accounts] doesn't match the length of [tokenIDs] +/// - Any address in [accounts] is invalid Future<List<BigInt>> fetchERC1155BatchBalanceOfTokens({
150-164: Fix typo in documentation comment.There's a typo in the documentation comment: "ERC115" should be "ERC1155".
Apply this diff to fix the typo:
/// -/// Fetch Uri of ERC115 Token +/// Fetch Uri of ERC1155 Token ///
278-299: Consider adding balance check before transfer.The method should verify that the sender has sufficient balance before attempting the transfer to prevent unnecessary gas costs.
Apply this diff to add balance check:
Future<String> sendERC1155Token({ required TransferIntent<EvmFeeInformation> intent, required String contractAddress, required BigInt tokenID, required String from, required Uint8List seed, }) async { final erc1155Contract = ERC1155Contract( contractAddress: contractAddress, rpc: this, ); + final balance = await erc1155Contract.balanceOf( + account: from, + tokenID: tokenID, + ); + + if (balance < intent.amount.value) { + throw Failure("Insufficient token balance for transfer"); + } + return erc1155Contract.safeTransferFrom( sender: from, to: intent.recipient, tokenID: tokenID, amount: intent.amount.value, seed: seed, feeInfo: intent.feeInfo, accessList: intent.accessList, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/src/crypto/evm/entities/abi/erc/erc1155.dart(1 hunks)lib/src/crypto/evm/entities/abi/gameItemsContract.dart(1 hunks)lib/src/crypto/evm/evm.dart(1 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(3 hunks)lib/src/domain/entities/coin_entity.dart(1 hunks)test/ci/evm/evm_rpc_test.dart(2 hunks)test/no_ci/arb_test.dart(1 hunks)
🔇 Additional comments (3)
lib/src/crypto/evm/entities/abi/erc/erc1155.dart (2)
5-320: LGTM! The ABI definition follows the ERC1155 standard.The contract ABI is well-structured and includes all required functions and events as per the ERC1155 standard:
- Core functions: balanceOf, balanceOfBatch, safeTransferFrom, safeBatchTransferFrom
- Approval functions: setApprovalForAll, isApprovedForAll
- Metadata function: uri
- Interface detection: supportsInterface
- Events: TransferSingle, TransferBatch, ApprovalForAll, URI
322-328: LGTM! The contract class is properly structured.The
ERC1155Contractclass correctly extendsInternalContractand requires the essential parameters:
- contractAddress: The address of the ERC1155 contract
- rpc: The RPC interface for network communication
lib/src/crypto/evm/entities/abi/gameItemsContract.dart (1)
3-490: LGTM! The game items contract ABI is well-structured.The ABI correctly implements:
- ERC1155 standard functions and events
- Game-specific token identifiers (GOLD, SHIELD, SILVER, SWORD, THORS_HAMMER)
- Comprehensive error handling with detailed error types
Comment on lines
+31
to
+33
| export 'entities/abi/erc/erc20_contract.dart'; | ||
| // ERC721 | ||
| export 'entities/abi/erc721_contract.dart'; | ||
| export 'entities/abi/erc/erc721_contract.dart'; |
There was a problem hiding this comment.
Add missing export for ERC1155 contract
While the ERC20 and ERC721 contracts are properly exported, the newly added ERC1155 contract is missing from the exports. This could prevent other parts of the codebase from accessing the ERC1155 functionality.
Add the following export:
export 'entities/abi/erc/erc20_contract.dart';
export 'entities/abi/erc/erc721_contract.dart';
+export 'entities/abi/erc/erc1155_contract.dart';📝 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.
Suggested change
| export 'entities/abi/erc/erc20_contract.dart'; | |
| // ERC721 | |
| export 'entities/abi/erc721_contract.dart'; | |
| export 'entities/abi/erc/erc721_contract.dart'; | |
| export 'entities/abi/erc/erc20_contract.dart'; | |
| // ERC721 | |
| export 'entities/abi/erc/erc721_contract.dart'; | |
| export 'entities/abi/erc/erc1155_contract.dart'; |
Comment on lines
+42
to
+59
| test('test to transfer erc1155 asset', () async { | ||
| final intent = TransferIntent<EvmFeeInformation>( | ||
| recipient: arbitrumTestWallet, | ||
| amount: Amount.convert(value: 100, decimals: 0), | ||
| token: nullToken, | ||
| memo: null, | ||
| ); | ||
|
|
||
| final hash = await zeniqSmartChainRPC.sendERC1155Token( | ||
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | ||
| from: arbitrumTestWallet, | ||
| tokenID: BigInt.from(0), | ||
| intent: intent, | ||
| seed: testSeed, | ||
| ); | ||
|
|
||
| print("Hash: $hash"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and readability
The test has several areas for improvement:
- Hardcoded values should be moved to constants or test fixtures
- Using
nullTokenfor ERC1155 transfer seems incorrect - Missing assertions to verify transfer success
Consider refactoring like this:
+const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86";
+const TEST_TOKEN_ID = BigInt.from(0);
test('test to transfer erc1155 asset', () async {
+ final testToken = ERC1155Entity(
+ name: "Test Token",
+ symbol: "TEST",
+ chainID: zeniqSmartChainRPC.chainId,
+ contractAddress: TEST_ERC1155_CONTRACT,
+ tokenId: TEST_TOKEN_ID,
+ );
final intent = TransferIntent<EvmFeeInformation>(
recipient: arbitrumTestWallet,
amount: Amount.convert(value: 100, decimals: 0),
- token: nullToken,
+ token: testToken,
memo: null,
);
final hash = await zeniqSmartChainRPC.sendERC1155Token(
- contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86",
+ contractAddress: TEST_ERC1155_CONTRACT,
from: arbitrumTestWallet,
- tokenID: BigInt.from(0),
+ tokenID: TEST_TOKEN_ID,
intent: intent,
seed: testSeed,
);
print("Hash: $hash");
+ expect(hash, isNotNull);
+
+ // Verify the transfer was successful
+ final newBalance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken(
+ account: arbitrumTestWallet,
+ tokenID: TEST_TOKEN_ID,
+ contractAddress: TEST_ERC1155_CONTRACT,
+ );
+ expect(newBalance, equals(BigInt.from(100)));
});Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
61
to
71
| test('test erc1155 balance of token', () async { | ||
| final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken( | ||
| account: arbitrumTestWallet, | ||
| tokenID: BigInt.from(2), | ||
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | ||
| ); | ||
|
|
||
| print('Balance: $balance'); | ||
| expect(balance, isNotNull); | ||
| expect(balance, greaterThanOrEqualTo(BigInt.zero)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve single balance test robustness
The test uses hardcoded values and has limited assertions.
Consider adding:
- Constants for test values
- Error case testing
- More specific assertions
+const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86";
+const TEST_TOKEN_ID = BigInt.from(2);
test('test erc1155 balance of token', () async {
+ // Test successful case
final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken(
account: arbitrumTestWallet,
- tokenID: BigInt.from(2),
+ tokenID: TEST_TOKEN_ID,
- contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86",
+ contractAddress: TEST_ERC1155_CONTRACT,
);
print('Balance: $balance');
expect(balance, isNotNull);
expect(balance, greaterThanOrEqualTo(BigInt.zero));
+
+ // Test error case with invalid contract
+ expect(
+ () => zeniqSmartChainRPC.fetchERC1155BalanceOfToken(
+ account: arbitrumTestWallet,
+ tokenID: TEST_TOKEN_ID,
+ contractAddress: "0x0000000000000000000000000000000000000000",
+ ),
+ throwsException,
+ );
});📝 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.
Suggested change
| test('test erc1155 balance of token', () async { | |
| final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken( | |
| account: arbitrumTestWallet, | |
| tokenID: BigInt.from(2), | |
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | |
| ); | |
| print('Balance: $balance'); | |
| expect(balance, isNotNull); | |
| expect(balance, greaterThanOrEqualTo(BigInt.zero)); | |
| }); | |
| const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86"; | |
| const TEST_TOKEN_ID = BigInt.from(2); | |
| test('test erc1155 balance of token', () async { | |
| // Test successful case | |
| final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken( | |
| account: arbitrumTestWallet, | |
| tokenID: TEST_TOKEN_ID, | |
| contractAddress: TEST_ERC1155_CONTRACT, | |
| ); | |
| print('Balance: $balance'); | |
| expect(balance, isNotNull); | |
| expect(balance, greaterThanOrEqualTo(BigInt.zero)); | |
| // Test error case with invalid contract | |
| expect( | |
| () => zeniqSmartChainRPC.fetchERC1155BalanceOfToken( | |
| account: arbitrumTestWallet, | |
| tokenID: TEST_TOKEN_ID, | |
| contractAddress: "0x0000000000000000000000000000000000000000", | |
| ), | |
| throwsException, | |
| ); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/ci/evm/evm_rpc_test.dart (1)
61-71: 🛠️ Refactor suggestionImprove test robustness and fix parameter name mismatch.
The test case needs improvement in several areas:
- Parameter name mismatch: The test uses
addressbut the contract method expectsaccount- Hardcoded values should be constants
- Limited test coverage - missing error cases
Apply this diff to improve the test:
+const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86"; +const TEST_TOKEN_ID = BigInt.from(2); test('test erc1155 balance of token', () async { final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken( - address: arbitrumTestWallet, + account: arbitrumTestWallet, - tokenID: BigInt.from(2), + tokenID: TEST_TOKEN_ID, - contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", + contractAddress: TEST_ERC1155_CONTRACT, ); print('Balance: $balance'); expect(balance, isNotNull); expect(balance, greaterThanOrEqualTo(BigInt.zero)); + + // Test error case with invalid contract + expect( + () => zeniqSmartChainRPC.fetchERC1155BalanceOfToken( + account: arbitrumTestWallet, + tokenID: TEST_TOKEN_ID, + contractAddress: "0x0000000000000000000000000000000000000000", + ), + throwsException, + ); });
🧹 Nitpick comments (2)
lib/src/crypto/evm/entities/abi/erc/erc1155.dart (2)
330-353: Replace magic numbers with named constants and add input validation.The methods use magic numbers for function indices and could benefit from additional input validation.
Apply this diff to improve the implementation:
+ static const _BALANCE_OF_INDEX = 0; + static const _BALANCE_OF_BATCH_INDEX = 1; + Future<BigInt> balanceOf({ required String address, required BigInt tokenID, }) async { + if (!isValidAddress(address)) { + throw ArgumentError('Invalid address format'); + } + if (tokenID < BigInt.zero) { + throw ArgumentError('Token ID must be non-negative'); + } - final function = abi.functions[0]; + final function = abi.functions[_BALANCE_OF_INDEX]; final response = await readSafe( function: function.addValues(values: [address, tokenID]), ); return response.outputs.first.castValue<BigInt>(); } Future<List<BigInt>> balanceOfBatch({ required List<String> accounts, required List<BigInt> tokenIDs, }) async { if (accounts.length != tokenIDs.length) { throw ArgumentError('accounts and tokenIDs must have the same length'); } + if (accounts.isEmpty || tokenIDs.isEmpty) { + throw ArgumentError('accounts and tokenIDs must not be empty'); + } + if (!accounts.every(isValidAddress)) { + throw ArgumentError('Invalid address format in accounts'); + } + if (tokenIDs.any((id) => id < BigInt.zero)) { + throw ArgumentError('Token IDs must be non-negative'); + } - final function = abi.functions[1]; + final function = abi.functions[_BALANCE_OF_BATCH_INDEX]; final response = await readSafe( function: function.addValues(values: [accounts, tokenIDs]), ); return response.outputs.first.castValue<List<BigInt>>(); }
355-363: Add input validation and URI format check.The method uses a magic number for function index and lacks input validation.
Apply this diff to improve the implementation:
+ static const _URI_INDEX = 7; + Future<String> getUri({ required BigInt tokenID, }) async { + if (tokenID < BigInt.zero) { + throw ArgumentError('Token ID must be non-negative'); + } - final function = abi.functions[7]; + final function = abi.functions[_URI_INDEX]; final response = await readSafe( function: function.addValues(values: [tokenID]), ); - return response.outputs.first.castValue<String>(); + final uri = response.outputs.first.castValue<String>(); + if (!uri.startsWith('ipfs://') && !uri.startsWith('http://') && !uri.startsWith('https://')) { + throw FormatException('Invalid URI format: $uri'); + } + return uri; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/src/crypto/evm/entities/abi/erc/erc1155.dart(1 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(3 hunks)test/ci/evm/evm_rpc_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart
🔇 Additional comments (4)
test/ci/evm/evm_rpc_test.dart (2)
96-104: 🛠️ Refactor suggestionEnhance URI test coverage and validation.
The test case needs improvement in several areas:
- Limited assertions - doesn't verify URI format
- Missing error cases for invalid token IDs
- No validation of URI content format
Apply this diff to improve the test:
+const TEST_ERC1155_URI_CONTRACT = "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa"; + test('test uri of erc1155 tokens', () async { final uri = await ethereumRPC.fetchERC1155UriOfToken( - tokenID: BigInt.from(1), - contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa", + tokenID: BigInt.from(1), + contractAddress: TEST_ERC1155_URI_CONTRACT, ); print('URI: $uri'); expect(uri, isNotNull); + expect(uri, matches(r'^(ipfs://|https?://).*')); + + // Test error case - invalid token ID + expect( + () => ethereumRPC.fetchERC1155UriOfToken( + tokenID: BigInt.from(-1), + contractAddress: TEST_ERC1155_URI_CONTRACT, + ), + throwsException, + ); });Likely invalid or redundant comment.
106-119: 🛠️ Refactor suggestionImprove ERC1155 interface detection.
The current implementation has several issues:
- Using balance check to determine interface support is not reliable
- No assertions to verify the result
- Should use
supportsInterfacefunction with ERC1155 interface ID (0xd9b67a26)Apply this diff to improve the test:
-test("is erc1155", () async { - bool isERC1155 = false; - try { - await ethereumRPC.fetchERC1155BalanceOfToken( - address: arbitrumTestWallet, - tokenID: BigInt.from(0), - contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa"); - isERC1155 = true; - } catch (e) { - isERC1155 = false; - } - - print('isERC1155: $isERC1155'); +test("supports erc1155 interface", () async { + final supportsERC1155 = await ethereumRPC.supportsInterface( + contractAddress: TEST_ERC1155_URI_CONTRACT, + interfaceId: "0xd9b67a26", + ); + + expect(supportsERC1155, isTrue); + + // Test with non-ERC1155 contract + final supportsERC1155NonERC = await ethereumRPC.supportsInterface( + contractAddress: "0x0000000000000000000000000000000000000000", + interfaceId: "0xd9b67a26", + ); + + expect(supportsERC1155NonERC, isFalse); });Likely invalid or redundant comment.
lib/src/crypto/evm/entities/abi/erc/erc1155.dart (2)
5-320: LGTM! Complete ERC1155 ABI implementation.The contract ABI includes all required functions and events as per the ERC1155 standard specification.
322-329: LGTM! Clean class declaration.The class properly extends InternalContract and follows the project's patterns for contract implementations.
Comment on lines
73
to
94
| test('test erc1155 batch balance of tokens', () async { | ||
| final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( | ||
| accounts: [ | ||
| arbitrumTestWallet, | ||
| arbitrumTestWallet, | ||
| arbitrumTestWallet, | ||
| arbitrumTestWallet, | ||
| arbitrumTestWallet | ||
| ], | ||
| tokenIDs: [ | ||
| BigInt.from(0), | ||
| BigInt.from(1), | ||
| BigInt.from(2), | ||
| BigInt.from(3), | ||
| BigInt.from(4) | ||
| ], | ||
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | ||
| ); | ||
|
|
||
| print('Balances: $balances'); | ||
| expect(balances, isNotNull); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance batch balance test coverage.
The test case needs improvement in several areas:
- Uses same account for all tokens, which doesn't fully test the batch functionality
- Limited assertions - only checks if result is not null
- Missing error cases for length mismatch and invalid inputs
Apply this diff to improve the test:
test('test erc1155 batch balance of tokens', () async {
+ final testAccounts = [
+ arbitrumTestWallet,
+ "0x1234567890123456789012345678901234567890",
+ "0x2345678901234567890123456789012345678901",
+ ];
+ final testTokenIds = [
+ BigInt.from(0),
+ BigInt.from(1),
+ BigInt.from(2),
+ ];
+
final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens(
- accounts: [
- arbitrumTestWallet,
- arbitrumTestWallet,
- arbitrumTestWallet,
- arbitrumTestWallet,
- arbitrumTestWallet
- ],
- tokenIDs: [
- BigInt.from(0),
- BigInt.from(1),
- BigInt.from(2),
- BigInt.from(3),
- BigInt.from(4)
- ],
+ accounts: testAccounts,
+ tokenIDs: testTokenIds,
contractAddress: TEST_ERC1155_CONTRACT,
);
print('Balances: $balances');
expect(balances, isNotNull);
+ expect(balances.length, equals(testAccounts.length));
+ expect(balances.every((balance) => balance >= BigInt.zero), isTrue);
+
+ // Test error case - length mismatch
+ expect(
+ () => zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens(
+ accounts: testAccounts,
+ tokenIDs: [BigInt.from(0)],
+ contractAddress: TEST_ERC1155_CONTRACT,
+ ),
+ throwsArgumentError,
+ );
});📝 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.
Suggested change
| test('test erc1155 batch balance of tokens', () async { | |
| final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( | |
| accounts: [ | |
| arbitrumTestWallet, | |
| arbitrumTestWallet, | |
| arbitrumTestWallet, | |
| arbitrumTestWallet, | |
| arbitrumTestWallet | |
| ], | |
| tokenIDs: [ | |
| BigInt.from(0), | |
| BigInt.from(1), | |
| BigInt.from(2), | |
| BigInt.from(3), | |
| BigInt.from(4) | |
| ], | |
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | |
| ); | |
| print('Balances: $balances'); | |
| expect(balances, isNotNull); | |
| }); | |
| test('test erc1155 batch balance of tokens', () async { | |
| final testAccounts = [ | |
| arbitrumTestWallet, | |
| "0x1234567890123456789012345678901234567890", | |
| "0x2345678901234567890123456789012345678901", | |
| ]; | |
| final testTokenIds = [ | |
| BigInt.from(0), | |
| BigInt.from(1), | |
| BigInt.from(2), | |
| ]; | |
| final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( | |
| accounts: testAccounts, | |
| tokenIDs: testTokenIds, | |
| contractAddress: TEST_ERC1155_CONTRACT, | |
| ); | |
| print('Balances: $balances'); | |
| expect(balances, isNotNull); | |
| expect(balances.length, equals(testAccounts.length)); | |
| expect(balances.every((balance) => balance >= BigInt.zero), isTrue); | |
| // Test error case - length mismatch | |
| expect( | |
| () => zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( | |
| accounts: testAccounts, | |
| tokenIDs: [BigInt.from(0)], | |
| contractAddress: TEST_ERC1155_CONTRACT, | |
| ), | |
| throwsArgumentError, | |
| ); | |
| }); |
Comment on lines
+365
to
+384
| Future<String> safeTransferFrom({ | ||
| required String sender, | ||
| required String to, | ||
| required BigInt tokenID, | ||
| required BigInt amount, | ||
| required Uint8List seed, | ||
| Uint8List? data, | ||
| EvmFeeInformation? feeInfo, | ||
| List<AccessListItem>? accessList, | ||
| }) async { | ||
| final function = abi.functions[4]; | ||
| return await interact( | ||
| function: function.addValues( | ||
| values: [sender, to, tokenID, amount, data ?? Uint8List(0)]), | ||
| sender: sender, | ||
| seed: seed, | ||
| feeInfo: feeInfo, | ||
| accessList: accessList, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive input validation for safe transfer.
The method uses a magic number for function index and lacks important input validations.
Apply this diff to improve the implementation:
+ static const _SAFE_TRANSFER_FROM_INDEX = 4;
+ static const _ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
+
Future<String> safeTransferFrom({
required String sender,
required String to,
required BigInt tokenID,
required BigInt amount,
required Uint8List seed,
Uint8List? data,
EvmFeeInformation? feeInfo,
List<AccessListItem>? accessList,
}) async {
+ if (!isValidAddress(sender) || !isValidAddress(to)) {
+ throw ArgumentError('Invalid address format');
+ }
+ if (to == _ZERO_ADDRESS) {
+ throw ArgumentError('Cannot transfer to zero address');
+ }
+ if (tokenID < BigInt.zero) {
+ throw ArgumentError('Token ID must be non-negative');
+ }
+ if (amount <= BigInt.zero) {
+ throw ArgumentError('Amount must be positive');
+ }
+ if (seed.isEmpty) {
+ throw ArgumentError('Seed cannot be empty');
+ }
- final function = abi.functions[4];
+ final function = abi.functions[_SAFE_TRANSFER_FROM_INDEX];
return await interact(
function: function.addValues(
values: [sender, to, tokenID, amount, data ?? Uint8List(0)]),
sender: sender,
seed: seed,
feeInfo: feeInfo,
accessList: accessList,
);
}📝 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.
Suggested change
| Future<String> safeTransferFrom({ | |
| required String sender, | |
| required String to, | |
| required BigInt tokenID, | |
| required BigInt amount, | |
| required Uint8List seed, | |
| Uint8List? data, | |
| EvmFeeInformation? feeInfo, | |
| List<AccessListItem>? accessList, | |
| }) async { | |
| final function = abi.functions[4]; | |
| return await interact( | |
| function: function.addValues( | |
| values: [sender, to, tokenID, amount, data ?? Uint8List(0)]), | |
| sender: sender, | |
| seed: seed, | |
| feeInfo: feeInfo, | |
| accessList: accessList, | |
| ); | |
| } | |
| static const _SAFE_TRANSFER_FROM_INDEX = 4; | |
| static const _ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"; | |
| Future<String> safeTransferFrom({ | |
| required String sender, | |
| required String to, | |
| required BigInt tokenID, | |
| required BigInt amount, | |
| required Uint8List seed, | |
| Uint8List? data, | |
| EvmFeeInformation? feeInfo, | |
| List<AccessListItem>? accessList, | |
| }) async { | |
| if (!isValidAddress(sender) || !isValidAddress(to)) { | |
| throw ArgumentError('Invalid address format'); | |
| } | |
| if (to == _ZERO_ADDRESS) { | |
| throw ArgumentError('Cannot transfer to zero address'); | |
| } | |
| if (tokenID < BigInt.zero) { | |
| throw ArgumentError('Token ID must be non-negative'); | |
| } | |
| if (amount <= BigInt.zero) { | |
| throw ArgumentError('Amount must be positive'); | |
| } | |
| if (seed.isEmpty) { | |
| throw ArgumentError('Seed cannot be empty'); | |
| } | |
| final function = abi.functions[_SAFE_TRANSFER_FROM_INDEX]; | |
| return await interact( | |
| function: function.addValues( | |
| values: [sender, to, tokenID, amount, data ?? Uint8List(0)]), | |
| sender: sender, | |
| seed: seed, | |
| feeInfo: feeInfo, | |
| accessList: accessList, | |
| ); | |
| } |
…de optional token ID in TokenInfo
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/src/domain/entities/token_info.dart (1)
10-10: Add documentation for theidfield.Consider adding a doc comment explaining that this field is used for ERC1155 token identification. This would help other developers understand its purpose and when it should be populated.
final String contractAddress; + /// The token ID for ERC1155 tokens. This field is only relevant for ERC1155 tokens + /// and remains null for other token types. final int? id;lib/src/crypto/wallet_utils.dart (1)
50-54: Add input validation for contract address.The function should validate the contract address format before making the RPC call.
Add validation like this:
Future<bool> isErc1155({ required String contractAddress, required EvmRpcInterface rpc, - required String address, }) async { + if (!contractAddress.startsWith('0x') || contractAddress.length != 42) { + throw ArgumentError('Invalid contract address format'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/wallet_utils.dart(1 hunks)lib/src/domain/entities/token_info.dart(1 hunks)
🔇 Additional comments (1)
lib/src/domain/entities/token_info.dart (1)
10-19: LGTM! Well-structured implementation of ERC1155 support.The addition of an optional
idfield is a clean way to extendTokenInfofor ERC1155 support while maintaining backward compatibility with existing token types. The nullable type is appropriate since this field is only relevant for ERC1155 tokens.
Comment on lines
+50
to
+67
| Future<bool> isErc1155({ | ||
| required String contractAddress, | ||
| required EvmRpcInterface rpc, | ||
| required String address, | ||
| }) async { | ||
| bool isErc1155 = false; | ||
| try { | ||
| await rpc.fetchERC1155BalanceOfToken( | ||
| address: address, | ||
| tokenID: BigInt.from(0), | ||
| contractAddress: contractAddress, | ||
| ); | ||
| isErc1155 = true; | ||
| } catch (e) { | ||
| isErc1155 = false; | ||
| } | ||
| return isErc1155; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ERC165 for interface detection instead of balance check.
The current implementation attempts to detect ERC1155 support by checking the balance of token ID 0, which may not exist. A more reliable approach would be to use ERC165's supportsInterface method to check for the ERC1155 interface ID (0xd9b67a26).
Consider this implementation:
Future<bool> isErc1155({
required String contractAddress,
required EvmRpcInterface rpc,
- required String address,
}) async {
bool isErc1155 = false;
try {
- await rpc.fetchERC1155BalanceOfToken(
- address: address,
- tokenID: BigInt.from(0),
- contractAddress: contractAddress,
- );
+ // ERC1155 interface ID: 0xd9b67a26
+ isErc1155 = await rpc.supportsInterface(
+ contractAddress: contractAddress,
+ interfaceId: '0xd9b67a26',
+ );
} catch (e) {
+ print('Error checking ERC1155 interface: $e');
isErc1155 = false;
}
return isErc1155;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
test/ci/evm/evm_rpc_test.dart (2)
61-71: 🛠️ Refactor suggestionImprove single balance test robustness.
The test uses hardcoded values and has limited assertions.
Consider adding:
- Constants for test values
- Error case testing
- More specific assertions
+const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86"; +const TEST_TOKEN_ID = BigInt.from(2); test('test erc1155 balance of token', () async { + // Test successful case final balance = await zeniqSmartChainRPC.fetchERC1155BalanceOfToken( account: arbitrumTestWallet, - tokenID: BigInt.from(2), + tokenID: TEST_TOKEN_ID, - contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", + contractAddress: TEST_ERC1155_CONTRACT, ); print('Balance: $balance'); expect(balance, isNotNull); expect(balance, greaterThanOrEqualTo(BigInt.zero)); + + // Test error case with invalid contract + expect( + () => zeniqSmartChainRPC.fetchERC1155BalanceOfToken( + account: arbitrumTestWallet, + tokenID: TEST_TOKEN_ID, + contractAddress: "0x0000000000000000000000000000000000000000", + ), + throwsException, + ); });
73-94: 🛠️ Refactor suggestionEnhance batch balance test coverage.
The test case needs improvement in several areas:
- Uses same account for all tokens, which doesn't fully test the batch functionality
- Limited assertions - only checks if result is not null
- Missing error cases for length mismatch and invalid inputs
Apply this diff to improve the test:
test('test erc1155 batch balance of tokens', () async { + final testAccounts = [ + arbitrumTestWallet, + "0x1234567890123456789012345678901234567890", + "0x2345678901234567890123456789012345678901", + ]; + final testTokenIds = [ + BigInt.from(0), + BigInt.from(1), + BigInt.from(2), + ]; + final balances = await zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( - accounts: [ - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet, - arbitrumTestWallet - ], - tokenIDs: [ - BigInt.from(0), - BigInt.from(1), - BigInt.from(2), - BigInt.from(3), - BigInt.from(4) - ], + accounts: testAccounts, + tokenIDs: testTokenIds, contractAddress: TEST_ERC1155_CONTRACT, ); print('Balances: $balances'); expect(balances, isNotNull); + expect(balances.length, equals(testAccounts.length)); + expect(balances.every((balance) => balance >= BigInt.zero), isTrue); + + // Test error case - length mismatch + expect( + () => zeniqSmartChainRPC.fetchERC1155BatchBalanceOfTokens( + accounts: testAccounts, + tokenIDs: [BigInt.from(0)], + contractAddress: TEST_ERC1155_CONTRACT, + ), + throwsArgumentError, + ); });
🧹 Nitpick comments (1)
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (1)
174-307: Consider extracting common JSON patterns.The function has two similar pattern matches that could be simplified by extracting common fields into a shared pattern.
Apply this diff to reduce code duplication:
factory EtherscanTransaction.fromJsonErc1155( Json json, { required EvmCoinEntity currency, required String address, }) { EtherscanTransaction _createTransaction({ required String block_s, required String timeStamp_s, required String hash, required String from, required String to, required String gas_s, required String gasUsed_s, required String gasPrice_s, required String contractAddress, required String? symbol, required String? name, required String tokenID_s, required String input, String? tokenValue, }) { // ... existing implementation ... } + // Common pattern for both cases + if (json case { + 'blockNumber': String block_s, + 'timeStamp': String timeStamp_s, + 'hash': String hash, + 'from': String from, + 'to': String to, + 'gas': String gas_s, + 'gasUsed': String gasUsed_s, + 'gasPrice': String gasPrice_s, + 'contractAddress': String contractAddress, + 'tokenSymbol': String? symbol, + 'tokenName': String? name, + 'tokenID': String tokenID_s, + }) { + final input = (json['input'] as String?) ?? ""; + final tokenValue = json['tokenValue'] as String?; + + return _createTransaction( + block_s: block_s, + timeStamp_s: timeStamp_s, + hash: hash, + from: from, + to: to, + gas_s: gas_s, + gasUsed_s: gasUsed_s, + gasPrice_s: gasPrice_s, + contractAddress: contractAddress, + symbol: symbol, + name: name, + tokenID_s: tokenID_s, + input: input, + tokenValue: tokenValue, + ); + } - if (json case { - 'blockNumber': String block_s, - // ... existing pattern 1 ... - }) { - return _createTransaction( - // ... existing implementation ... - ); - } - if (json case { - 'blockNumber': String block_s, - // ... existing pattern 2 ... - }) { - return _createTransaction( - // ... existing implementation ... - ); - } throw UnsupportedError("Invalid JSON for EtherscanTransaction"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart(1 hunks)lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart(3 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(3 hunks)lib/src/domain/entities/coin_entity.dart(6 hunks)test/ci/evm/evm_explorer_test.dart(1 hunks)test/ci/evm/evm_rpc_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart
- lib/src/domain/entities/coin_entity.dart
🔇 Additional comments (2)
lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart (2)
74-88: LGTM!The endpoint builder follows the established pattern and correctly handles both required and optional parameters.
121-147: LGTM!The implementation follows the established pattern for transaction fetching and correctly handles the response parsing.
Comment on lines
+57
to
+65
| test('test erc1155 fetching zsc', () async { | ||
| final transactions = await zeniqScan.fetchERC1155Transactions( | ||
| address: arbitrumTestWallet, | ||
| contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86", | ||
| ); | ||
|
|
||
| print('ERC1155 Transactions: $transactions'); | ||
| expect(transactions, isNotEmpty); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for ERC1155 transaction fetching.
The test case needs improvement in several areas:
- Uses hardcoded contract address
- Limited assertions - only checks if transactions list is not empty
- Missing error cases
Apply this diff to improve the test:
+const TEST_ERC1155_CONTRACT = "0xB868a4d85c3f7207106145eB41444c5313C97D86";
+
test('test erc1155 fetching zsc', () async {
+ // Test successful case
final transactions = await zeniqScan.fetchERC1155Transactions(
address: arbitrumTestWallet,
- contractAddress: "0xB868a4d85c3f7207106145eB41444c5313C97D86",
+ contractAddress: TEST_ERC1155_CONTRACT,
);
- print('ERC1155 Transactions: $transactions');
expect(transactions, isNotEmpty);
+
+ // Verify transaction properties
+ for (final tx in transactions) {
+ expect(tx.token, isA<ERC1155Entity>());
+ expect((tx.token as ERC1155Entity).contractAddress, equals(TEST_ERC1155_CONTRACT));
+ expect(tx.hash, isNotEmpty);
+ expect(tx.block, greaterThan(0));
+ }
+
+ // Test error case with invalid contract
+ expect(
+ () => zeniqScan.fetchERC1155Transactions(
+ address: arbitrumTestWallet,
+ contractAddress: "0x0000000000000000000000000000000000000000",
+ ),
+ throwsException,
+ );
});Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+358
to
+374
| @override | ||
| String buildERC1155TransactionEndpoint({ | ||
| required String address, | ||
| required String contractAddress, | ||
| int? startblock, | ||
| int? endblock, | ||
| int? page, | ||
| int? offset, | ||
| Sorting? sorting, | ||
| }) { | ||
| return "$base&module=account&action=tokentx&address=$address&contractaddress=$contractAddress" | ||
| .addOptionalParameter('start_block', startblock) | ||
| .addOptionalParameter('end_block', endblock) | ||
| .addOptionalParameter('page', page) | ||
| .addOptionalParameter('offset', offset) | ||
| .addOptionalParameter('sort', sorting?.name); | ||
| } |
There was a problem hiding this comment.
Fix incorrect action in ZeniqScan endpoint builder.
The endpoint builder uses 'tokentx' action instead of 'token1155tx', which is inconsistent with the parent class implementation. This could lead to incorrect transaction fetching.
Apply this diff to fix the action:
@override
String buildERC1155TransactionEndpoint({
required String address,
required String contractAddress,
int? startblock,
int? endblock,
int? page,
int? offset,
Sorting? sorting,
}) {
- return "$base&module=account&action=tokentx&address=$address&contractaddress=$contractAddress"
+ return "$base&module=account&action=token1155tx&address=$address&contractaddress=$contractAddress"
.addOptionalParameter('start_block', startblock)
.addOptionalParameter('end_block', endblock)
.addOptionalParameter('page', page)
.addOptionalParameter('offset', offset)
.addOptionalParameter('sort', sorting?.name);
}📝 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.
Suggested change
| @override | |
| String buildERC1155TransactionEndpoint({ | |
| required String address, | |
| required String contractAddress, | |
| int? startblock, | |
| int? endblock, | |
| int? page, | |
| int? offset, | |
| Sorting? sorting, | |
| }) { | |
| return "$base&module=account&action=tokentx&address=$address&contractaddress=$contractAddress" | |
| .addOptionalParameter('start_block', startblock) | |
| .addOptionalParameter('end_block', endblock) | |
| .addOptionalParameter('page', page) | |
| .addOptionalParameter('offset', offset) | |
| .addOptionalParameter('sort', sorting?.name); | |
| } | |
| @override | |
| String buildERC1155TransactionEndpoint({ | |
| required String address, | |
| required String contractAddress, | |
| int? startblock, | |
| int? endblock, | |
| int? page, | |
| int? offset, | |
| Sorting? sorting, | |
| }) { | |
| return "$base&module=account&action=token1155tx&address=$address&contractaddress=$contractAddress" | |
| .addOptionalParameter('start_block', startblock) | |
| .addOptionalParameter('end_block', endblock) | |
| .addOptionalParameter('page', page) | |
| .addOptionalParameter('offset', offset) | |
| .addOptionalParameter('sort', sorting?.name); | |
| } |
Comment on lines
+96
to
+104
| test('test uri of erc1155 tokens', () async { | ||
| final uri = await ethereumRPC.fetchERC1155UriOfToken( | ||
| tokenID: BigInt.from(1), | ||
| contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa", | ||
| ); | ||
|
|
||
| print('URI: $uri'); | ||
| expect(uri, isNotNull); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URI test coverage.
The test case needs improvement in several areas:
- Uses hardcoded values
- Limited assertions - only checks if URI is not null
- Missing error cases
Apply this diff to improve the test:
+const TEST_ERC1155_URI_CONTRACT = "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa";
+const TEST_URI_TOKEN_ID = BigInt.from(1);
test('test uri of erc1155 tokens', () async {
+ // Test successful case
final uri = await ethereumRPC.fetchERC1155UriOfToken(
- tokenID: BigInt.from(1),
+ tokenID: TEST_URI_TOKEN_ID,
- contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa",
+ contractAddress: TEST_ERC1155_URI_CONTRACT,
);
- print('URI: $uri');
expect(uri, isNotNull);
+ expect(uri, isA<String>());
+ expect(uri, contains('{id}')); // ERC1155 URIs typically contain {id} placeholder
+
+ // Test error case with invalid contract
+ expect(
+ () => ethereumRPC.fetchERC1155UriOfToken(
+ tokenID: TEST_URI_TOKEN_ID,
+ contractAddress: "0x0000000000000000000000000000000000000000",
+ ),
+ throwsException,
+ );
});📝 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.
Suggested change
| test('test uri of erc1155 tokens', () async { | |
| final uri = await ethereumRPC.fetchERC1155UriOfToken( | |
| tokenID: BigInt.from(1), | |
| contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa", | |
| ); | |
| print('URI: $uri'); | |
| expect(uri, isNotNull); | |
| }); | |
| const TEST_ERC1155_URI_CONTRACT = "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa"; | |
| const TEST_URI_TOKEN_ID = BigInt.from(1); | |
| test('test uri of erc1155 tokens', () async { | |
| // Test successful case | |
| final uri = await ethereumRPC.fetchERC1155UriOfToken( | |
| tokenID: TEST_URI_TOKEN_ID, | |
| contractAddress: TEST_ERC1155_URI_CONTRACT, | |
| ); | |
| expect(uri, isNotNull); | |
| expect(uri, isA<String>()); | |
| expect(uri, contains('{id}')); // ERC1155 URIs typically contain {id} placeholder | |
| // Test error case with invalid contract | |
| expect( | |
| () => ethereumRPC.fetchERC1155UriOfToken( | |
| tokenID: TEST_URI_TOKEN_ID, | |
| contractAddress: "0x0000000000000000000000000000000000000000", | |
| ), | |
| throwsException, | |
| ); | |
| }); |
Comment on lines
+106
to
+119
| test("is erc1155", () async { | ||
| bool isERC1155 = false; | ||
| try { | ||
| await ethereumRPC.fetchERC1155BalanceOfToken( | ||
| address: arbitrumTestWallet, | ||
| tokenID: BigInt.from(0), | ||
| contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa"); | ||
| isERC1155 = true; | ||
| } catch (e) { | ||
| isERC1155 = false; | ||
| } | ||
|
|
||
| print('isERC1155: $isERC1155'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve ERC1155 detection test.
The test uses try-catch for control flow and lacks comprehensive test cases.
Apply this diff to improve the test:
-test("is erc1155", () async {
+test("test ERC1155 contract detection", () async {
+ // Test valid ERC1155 contract
bool isERC1155 = false;
try {
await ethereumRPC.fetchERC1155BalanceOfToken(
address: arbitrumTestWallet,
tokenID: BigInt.from(0),
- contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa");
+ contractAddress: TEST_ERC1155_URI_CONTRACT);
isERC1155 = true;
} catch (e) {
isERC1155 = false;
}
+ expect(isERC1155, isTrue, reason: "Valid ERC1155 contract not detected");
- print('isERC1155: $isERC1155');
+ // Test ERC20 contract (should not be detected as ERC1155)
+ isERC1155 = false;
+ try {
+ await ethereumRPC.fetchERC1155BalanceOfToken(
+ address: arbitrumTestWallet,
+ tokenID: BigInt.from(0),
+ contractAddress: usdtToken.contractAddress,
+ );
+ isERC1155 = true;
+ } catch (e) {
+ isERC1155 = false;
+ }
+ expect(isERC1155, isFalse, reason: "ERC20 contract incorrectly detected as ERC1155");
+
+ // Test invalid contract
+ isERC1155 = false;
+ try {
+ await ethereumRPC.fetchERC1155BalanceOfToken(
+ address: arbitrumTestWallet,
+ tokenID: BigInt.from(0),
+ contractAddress: "0x0000000000000000000000000000000000000000",
+ );
+ isERC1155 = true;
+ } catch (e) {
+ isERC1155 = false;
+ }
+ expect(isERC1155, isFalse, reason: "Invalid contract incorrectly detected as ERC1155");
});📝 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.
Suggested change
| test("is erc1155", () async { | |
| bool isERC1155 = false; | |
| try { | |
| await ethereumRPC.fetchERC1155BalanceOfToken( | |
| address: arbitrumTestWallet, | |
| tokenID: BigInt.from(0), | |
| contractAddress: "0x1ca3262009b21F944e6b92a2a88D039D06F1acFa"); | |
| isERC1155 = true; | |
| } catch (e) { | |
| isERC1155 = false; | |
| } | |
| print('isERC1155: $isERC1155'); | |
| }); | |
| test("test ERC1155 contract detection", () async { | |
| // Test valid ERC1155 contract | |
| bool isERC1155 = false; | |
| try { | |
| await ethereumRPC.fetchERC1155BalanceOfToken( | |
| address: arbitrumTestWallet, | |
| tokenID: BigInt.from(0), | |
| contractAddress: TEST_ERC1155_URI_CONTRACT); | |
| isERC1155 = true; | |
| } catch (e) { | |
| isERC1155 = false; | |
| } | |
| expect(isERC1155, isTrue, reason: "Valid ERC1155 contract not detected"); | |
| // Test ERC20 contract (should not be detected as ERC1155) | |
| isERC1155 = false; | |
| try { | |
| await ethereumRPC.fetchERC1155BalanceOfToken( | |
| address: arbitrumTestWallet, | |
| tokenID: BigInt.from(0), | |
| contractAddress: usdtToken.contractAddress, | |
| ); | |
| isERC1155 = true; | |
| } catch (e) { | |
| isERC1155 = false; | |
| } | |
| expect(isERC1155, isFalse, reason: "ERC20 contract incorrectly detected as ERC1155"); | |
| // Test invalid contract | |
| isERC1155 = false; | |
| try { | |
| await ethereumRPC.fetchERC1155BalanceOfToken( | |
| address: arbitrumTestWallet, | |
| tokenID: BigInt.from(0), | |
| contractAddress: "0x0000000000000000000000000000000000000000", | |
| ); | |
| isERC1155 = true; | |
| } catch (e) { | |
| isERC1155 = false; | |
| } | |
| expect(isERC1155, isFalse, reason: "Invalid contract incorrectly detected as ERC1155"); | |
| }); |
…herscanTransaction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (4)
174-178: Add documentation for the factory method.Please add documentation comments explaining:
- The expected JSON structure
- The purpose of the factory method
- Example usage
179-239: Consider refactoring the helper function.The
_createTransactionhelper function:
- Could be moved outside as a private static method for better testability
- Has many parameters that could be grouped into a data class
Example refactor:
class _Erc1155TransactionData { final String block_s; final String timeStamp_s; // ... other fields const _Erc1155TransactionData({ required this.block_s, required this.timeStamp_s, // ... other fields }); } static EtherscanTransaction _createErc1155Transaction( _Erc1155TransactionData data, EvmCoinEntity currency, ) { // ... existing logic }
198-204: Improve token creation robustness.Consider these improvements:
- Validate contractAddress format
- Define constants for fallback values instead of magic strings
const _UNAVAILABLE_TOKEN_NAME = "N.Av"; const _UNAVAILABLE_TOKEN_SYMBOL = "N.Av"; // Add validation if (!isValidEthereumAddress(contractAddress)) { throw FormatException('Invalid contract address format'); }
241-306: Improve JSON parsing robustness and maintainability.Consider these improvements:
- Add logging for parsing failures to help with debugging
- Consider unifying the patterns to reduce code duplication
Example improvement:
try { if (json case {'tokenValue': String value_s, ...final rest}) { return _createTransaction( tokenValue: value_s, ...mapJsonToTransactionParams(rest), ); } else if (json case {...final rest}) { return _createTransaction( ...mapJsonToTransactionParams(rest), ); } } catch (e, stack) { Logger.logError( e, s: stack, hint: "Failed to parse ERC1155 transaction JSON", ); rethrow; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart(1 hunks)
🔇 Additional comments (1)
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (1)
236-236: Verify transaction confirmation status handling.The status is derived from confirmations count instead of using
txreceipt_statuslike in ERC20/native transactions. This might lead to inconsistent status reporting.
dev2-nomo
added a commit
that referenced
this pull request
Jan 30, 2025
* feat: add ERC1155 support with balance fetching and transfer function… (#133) * feat: add ERC1155 support with balance fetching and transfer functionality * feat: add ERC1155 export to EVM module * refactor: rename 'account' parameter to 'address' for consistency in ERC1155 balance functions * feat: add isErc1155 function to check ERC1155 token support and include optional token ID in TokenInfo * feat: add ERC1155 transaction fetching and balance handling in EVM module * fix: update tokenValue parsing to default to zero instead of -1 in EtherscanTransaction * Add Hex Normalization to prevent leading Zeros in RLP (#135) * Add Hex Normalization to prevent leading Zeros in RLP * Start of Reimplementing RLP & Testing * Implement all Test Cases and Decoding * Finish RawEvmTx Encoding and Decode and fix lint * Refactor Buffer to int/BigInt and vice versa * Refactor BigInt Conversion * Refactor RLP encoding to use sublist for length extraction * Remove unused import * Fix Access List Encoding & add isHex Parameter to RLPString to indicate hex * Add RLP test directory to GitHub Actions workflow * Add decodeRLPCheck function for input validation and refactor length extraction (check for leading 0) --------- Co-authored-by: dev2 <[email protected]> Co-authored-by: thomas.fercher <[email protected]> * FunctionParamWithValue JsonEncoding (#136) * initial * feat: implement JSON serialization and deserialization for function parameter types * Fix Lint --------- Co-authored-by: dev2 <[email protected]> * fix: update toJson method for EvmType2GasPrice to use proper serialization (#138) * Initial * Update PipeTests * Fix Tests * Fix Lint * Update GitHub Actions to use upload-artifact@v4 * Update Tests * Fix Lints --------- Co-authored-by: The Nomo App's core developer team <[email protected]> Co-authored-by: thomas.fercher <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ality
Summary by CodeRabbit
New Features
Refactor
Tests