refactor: migrate to etherscan v2 api #118
Conversation
WalkthroughThe changes in this pull request focus on modifying the URL construction logic for API endpoints in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (10)
test/utils.dart (2)
84-99: Improve code formatting for better readability.The explorer initializations can be formatted more consistently for better readability.
Consider this format:
final polygonscan = EtherscanExplorer( - etherscanBaseEndpoint, loadListFromEnv("ETHERSCAN_API_KEYS"), polygon); + etherscanBaseEndpoint, + loadListFromEnv("ETHERSCAN_API_KEYS"), + polygon, );
84-99: Consider adding error handling for missing API keys.The
loadListFromEnvfunction assumes the environment variable exists and contains valid data. Missing or invalid API keys could cause runtime errors.Consider adding a fallback or error handling:
List<String> loadListFromEnv(String key) { var env = DotEnv(includePlatformEnvironment: true)..load(); - final seedString = env[key]!.split(","); - return seedString; + final value = env[key]; + if (value == null || value.isEmpty) { + return []; // or throw a more specific exception + } + return value.split(","); }lib/src/crypto/evm/repositories/etherscan/etherscan_repository.dart (2)
Line range hint
64-71: Improve URL construction robustness.The current implementation has several potential issues:
- It doesn't properly handle URLs that already contain query parameters
- Lacks validation of URL components
- Manual string concatenation is error-prone
Consider this more robust implementation:
- return '$baseUrl&module=$module&action=$action'; + final uriBuilder = Uri.parse(baseUrl); + final newParams = Map<String, String>.from(uriBuilder.queryParameters) + ..addAll({ + 'module': module, + 'action': action, + }); + return uriBuilder.replace(queryParameters: newParams).toString();This approach:
- Properly handles existing query parameters
- Uses Uri builder methods for safe URL construction
- Maintains all existing parameters while adding/updating module and action
Line range hint
6-21: Add explicit API version handling.Since this PR migrates to Etherscan v2 API, consider making the version explicit in the code to improve maintainability and prevent accidental mixing of v1/v2 endpoints.
Consider adding version handling:
class EtherscanRepository { + static const apiVersion = 'v2'; final String base; final List<String> apiKeys; final Map<String, bool> endpointNeedsApiKey = {}; final Map<String, DateTime> apiKeyExcludedUntil = {}; final List<String> invalidApiKeys = []; final Duration noApiKeyRetryIntervall; final Duration apiKeyRetryIntervall; EtherscanRepository( - this.base, + String baseUrl, this.apiKeys, { this.noApiKeyRetryIntervall = const Duration(seconds: 5), this.apiKeyRetryIntervall = const Duration(seconds: 3), - }); + }) : base = '$baseUrl/$apiVersion/api';This makes the API version explicit and ensures consistent version usage across all endpoints.
test/ci/evm/evm_explorer_test.dart (3)
114-114: Remove debug print statements.Debug print statements should be removed from test cases as they add noise to test output and don't provide value in CI environments.
- print('BNB Balance: $balance'); - print('Zeniq BSC Balance: $zeniqBSCBalance'); - print('BNB Transactions: $transactions'); - print('Zeniq BSC Transactions: $zeniqBSCTokenTransactions'); - print('ERC721: $erc721');Also applies to: 119-119, 131-131, 140-140, 148-148
116-117: Fix method parameter formatting.The method parameters should be consistently formatted across the codebase. Add a line break for the second parameter to match the style used in other similar calls.
final zeniqBSCBalance = await bscscan.fetchTokenBalance( - address: rejectEVM, contractAddress: zeniqBSCToken.contractAddress); + address: rejectEVM, + contractAddress: zeniqBSCToken.contractAddress, + );
204-226: Improve test structure and documentation.The test follows good practices with proper assertions but could benefit from:
- Consistent spacing between test sections (add empty lines)
- Documentation comments to separate test sections like other tests
test('Test Base Fetching', () async { + /// + /// Balances + /// final balance = await basescan.fetchBalance(address: arbitrumTestWallet); expect(balance, greaterThanOrEqualTo(BigInt.zero)); + final transactions = await basescan.fetchTransactions( address: arbitrumTestWallet, ); expect(transactions, isNotEmpty); + /// + /// ERC20 Transactions + /// final erc20Transactions = await basescan.fetchERC20Transactions( address: arbitrumTestWallet, contractAddress: mathToken.contractAddress, ); expect(erc20Transactions, isNotEmpty); + final erc20balance = await basescan.fetchTokenBalance( address: arbitrumTestWallet, contractAddress: mathToken.contractAddress); expect(erc20balance, greaterThan(BigInt.zero)); });lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart (3)
225-226: Add error handling for invalid chainID.While the gas oracle endpoint construction is correct, consider adding validation for the chainID parameter to handle cases where it might be null or invalid.
Future<EvmNetworkFees> fetchGasPrice() async { + if (currency.chainID <= 0) { + throw ArgumentError('Invalid chainID: ${currency.chainID}'); + } final gasOracleEndpoint = "${base}&module=gastracker&action=gasoracle";
238-238: Update error message for fetchEstimatedTime.The error message doesn't match the function's purpose.
if (result is! String) { - throw Exception("Failed to fetch gas price"); + throw Exception("Failed to fetch estimated time"); }
Line range hint
1-310: Consider adding API version documentation and improving error handling.While the changes successfully support the v2 API migration, consider these architectural improvements:
- Add documentation about the minimum required API versions for each explorer
- Consider implementing a common error handling strategy across all explorers
- Add validation for required parameters (chainID, addresses, etc.)
Would you like help implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart(8 hunks)lib/src/crypto/evm/repositories/etherscan/etherscan_repository.dart(1 hunks)lib/src/domain/constants.dart(1 hunks)test/ci/evm/evm_explorer_test.dart(2 hunks)test/utils.dart(1 hunks)
🔇 Additional comments (7)
test/utils.dart (1)
84-99:
Verify the use of Etherscan v2 endpoint for all networks.
While standardizing the explorer initialization is good for maintainability, using etherscanBaseEndpoint for all networks (Polygon, BSC, Arbitrum, etc.) raises concerns:
- Each network typically has its own explorer API endpoint (e.g., api.polygonscan.com, api.bscscan.com)
- Using a single "ETHERSCAN_API_KEYS" environment variable for all networks might not be correct
Let's verify the API endpoints for these networks:
Consider:
- Using network-specific endpoints for each explorer
- Using separate environment variables for each network's API keys (e.g., "POLYGONSCAN_API_KEYS", "BSCSCAN_API_KEYS")
Example fix for Polygonscan:
final polygonscan = EtherscanExplorer(
- etherscanBaseEndpoint, loadListFromEnv("ETHERSCAN_API_KEYS"), polygon);
+ polygonScanBaseEndpoint, loadListFromEnv("POLYGONSCAN_API_KEYS"), polygon);lib/src/crypto/evm/repositories/etherscan/etherscan_repository.dart (1)
Line range hint 1-180: Verify v2 API migration completeness.
Please ensure the following aspects of the v2 migration are covered:
- All API endpoint callers are updated for v2 compatibility
- Test coverage includes URL construction scenarios
- Documentation reflects v2 API usage
Let's verify the migration completeness:
lib/src/domain/constants.dart (2)
201-203: Verify the impact of removed blockchain explorer endpoints.
Several blockchain explorer endpoints were removed according to the AI summary. Let's verify that no code still depends on these removed constants.
✅ Verification successful
Since the search results show no references to either the removed endpoint constants or their corresponding hardcoded URLs in any Dart files, I can confidently generate the final response:
Removal of blockchain explorer endpoints is safe
The removed endpoints (bnbScanBaseEndpoint, arbitrumScanBaseEndpoint, moonbeamScanBaseEndpoint, baseScanEndpoint, avalancheAPIEndpoint, optimismScanEndpoint, zksyncAPIEndpoint) are not referenced anywhere else in the Dart codebase, and there are no hardcoded versions of these URLs either. The cleanup appears to be complete and safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the removed endpoints
# to ensure they've been properly cleaned up
echo "Searching for references to removed endpoints:"
REMOVED_ENDPOINTS=(
"bnbScanBaseEndpoint"
"arbitrumScanBaseEndpoint"
"moonbeamScanBaseEndpoint"
"baseScanEndpoint"
"avalancheAPIEndpoint"
"optimismScanEndpoint"
"zksyncAPIEndpoint"
)
for endpoint in "${REMOVED_ENDPOINTS[@]}"; do
echo -e "\nChecking for references to $endpoint:"
rg "$endpoint" --type dart
done
# Also check for hardcoded URLs
echo -e "\nChecking for hardcoded URLs:"
rg -i "api\.(bscscan|arbiscan|moonscan|basescan|routescan|optimistic\.etherscan|era\.zksync)\." --type dart
Length of output: 1833
201-201: Verify the impact of Etherscan API v2 migration.
The update to Etherscan API v2 is a potentially breaking change. Let's verify that all code using this endpoint has been updated to handle any v2-specific changes.
✅ Verification successful
No breaking changes from Etherscan API v2 migration
The implementation is safe for the v2 API migration because:
- The EtherscanExplorer and EtherscanRepository classes use version-agnostic endpoint construction
- API calls are built using query parameters that remain consistent across v1 and v2
- The endpoint is only used in test utilities where multiple blockchain explorers are instantiated
- The API version is transparent to the implementation as it handles responses based on standard fields like error messages and result format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that use the etherscanBaseEndpoint constant
# to ensure they've been updated to handle v2 API changes
# Test 1: Find direct references to etherscanBaseEndpoint
echo "Files referencing etherscanBaseEndpoint:"
rg -l "etherscanBaseEndpoint"
# Test 2: Find any remaining v1 API URLs that might need updating
echo -e "\nPotential v1 API references:"
rg -i "api\.etherscan\.io/api" --type dart
Length of output: 284
Script:
#!/bin/bash
# Let's examine the actual usage in the test file and check for any API-related code
echo "Content of test/utils.dart where etherscanBaseEndpoint is used:"
rg -A 5 -B 5 "etherscanBaseEndpoint" test/utils.dart
echo -e "\nLet's also check for any HTTP client or API call implementations:"
rg -l "http|client|api|etherscan" --type dart
echo -e "\nExamine removed lines in constants.dart to check for other endpoint removals:"
git log -p lib/src/domain/constants.dart | rg -A 5 -B 5 "etherscan"
Length of output: 6184
Script:
#!/bin/bash
# Let's examine the EtherscanExplorer implementation to understand how the endpoint is used
echo "Checking EtherscanExplorer implementation:"
rg -A 10 "class EtherscanExplorer" lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart
echo -e "\nLet's also check for any API version specific handling:"
rg -i "v2|version|api" lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart
rg -i "v2|version|api" lib/src/crypto/evm/repositories/etherscan/etherscan_repository.dart
Length of output: 3463
lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart (3)
19-19: LGTM: Consistent endpoint parameter formatting.
The endpoint construction has been updated consistently to use & for all parameters, which aligns with the new base URL structure that includes the chainID parameter.
Also applies to: 23-23, 34-34, 50-50, 66-66
Line range hint 286-292: Ensure parameter naming consistency between explorers.
The ZeniqScanExplorer uses snake_case for block parameters (start_block, end_block) while the parent EtherscanExplorer uses camelCase (startblock, endblock). This inconsistency should be addressed:
- Verify the expected parameter format for each API
- Consider using a consistent format if both APIs support it
#!/bin/bash
# Search for other block parameter usages
rg -g '*.dart' '(start|end)_?block' --no-filenameAlso applies to: 304-310
15-16: Verify the chainID parameter placement in the URL.
The base URL now always includes the chainID parameter. While this change supports the v2 API migration, it's a breaking change that affects all derived endpoints.
Consider:
- Adding documentation about this URL structure change
- Verifying that all blockchain explorers support this chainID parameter format
✅ Verification successful
The previous search wasn't successful, so I'm trying a broader approach to understand:
- The explorer class hierarchy
- How base URLs are constructed across the codebase
- Any direct usage of etherscan API
This will help us verify if the chainID parameter change is safe and properly implemented within the inheritance structure.
Based on the results, I can now generate a final response as we have sufficient information about the class hierarchy and URL construction patterns.
The chainID parameter addition in base URL is correctly implemented and safe
The change is part of a proper v2 API migration where:
- The base class hierarchy (EtherscanRepository -> EtherscanExplorer -> ZeniqScanExplorer) is maintained
- All explorer instances are initialized with the new v2 base endpoint (
etherscanBaseEndpoint = "https://api.etherscan.io/v2/api") - The chainID parameter is consistently added through the base getter override
The implementation is safe because:
- All EVM chain explorers (etherscan, polygonscan, bscscan, etc.) are properly instantiated with their respective currencies
- The chainID parameter is derived from the currency entity, ensuring correct chain identification
- ZeniqScanExplorer inherits this behavior appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom URL construction that might bypass the base getter
rg -g '*.dart' 'api\.etherscan\.io' --no-filename
Length of output: 2029
Script:
#!/bin/bash
# Let's search for base URL construction and inheritance patterns
rg -g '*.dart' -B 2 -A 2 'class.*Explorer.*extends' --no-filename
# Also search for any URL construction patterns
rg -g '*.dart' -B 2 -A 2 'get base.*=>' --no-filename
# And check for any direct etherscan API usage
rg -g '*.dart' -B 2 -A 2 'etherscan.*api' -i --no-filename
Length of output: 1915
| @@ -65,7 +65,7 @@ class EtherscanRepository { | |||
| String action = queryParams['action']!; | |||
|
|
|||
| // Construct the base endpoint | |||
| return '$baseUrl?module=$module&action=$action'; | |||
| return '$baseUrl&module=$module&action=$action'; | |||
There was a problem hiding this comment.
Fix incorrect URL parameter separator.
The current implementation always uses & as the parameter separator, which is incorrect for the first parameter in a URL. The first parameter should use ? while subsequent parameters should use &.
Apply this fix:
- return '$baseUrl&module=$module&action=$action';
+ return '$baseUrl?module=$module&action=$action';This ensures proper URL construction following the standard format: baseUrl?param1=value1¶m2=value2
📝 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.
| return '$baseUrl&module=$module&action=$action'; | |
| return '$baseUrl?module=$module&action=$action'; |
| final balance = await bscscan.fetchBalance( | ||
| address: rejectEVM, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add balance value assertions.
The test verifies that transactions are not empty but doesn't validate the balance values. Consider adding assertions to verify that the balances are greater than or equal to zero, following the pattern used in other explorer tests.
final balance = await bscscan.fetchBalance(
address: rejectEVM,
);
+ expect(balance, greaterThanOrEqualTo(BigInt.zero));
final zeniqBSCBalance = await bscscan.fetchTokenBalance(
address: rejectEVM, contractAddress: zeniqBSCToken.contractAddress);
+ expect(zeniqBSCBalance, greaterThanOrEqualTo(BigInt.zero));Also applies to: 116-117
…e Decoding with name and type
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
test/ci/parsing/contract_function_fromString_test.dart (2)
5-20: Consider enhancing test coverage.While the current test case correctly validates parameter types and names, consider adding:
- Assertion for the function name ("transfer")
- Test cases for invalid signatures to verify error handling
Example additions:
expect(contractFunction.name, "transfer"); test("External Contract Function fromString Invalid", () { expect( () => ExternalContractFunction.fromString(textSignature: "invalid("), throwsA(isA<FormatException>()), ); });
22-37: Consider adding edge case tests.The test case effectively validates named parameters. Consider adding edge cases:
- Mixed named and unnamed parameters
- Parameters with special characters in names
- Parameters with whitespace variations
Example addition:
test("External Contract Function fromString Mixed Parameters", () { const mixedSignature = "transfer(address _to, uint256)"; final contractFunction = ExternalContractFunction.fromString( textSignature: mixedSignature, ); expect(contractFunction.parameters.first.name, "_to"); expect(contractFunction.parameters.last.name, null); });lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (2)
84-85: Enhance error logging with additional contextIncluding the
functionNamein the error log can provide more context and aid in debugging when decoding fails.Apply this diff to improve the log message:
} catch (e, s) { - Logger.logError(e, s: s, hint: "Failed to decode contract function"); + Logger.logError( + e, + s: s, + hint: "Failed to decode contract function '${functionName}'" + ); }
Line range hint
30-108: Refactor to reduce code duplication between Etherscan V2 and V1 parsingThe parsing logic for Etherscan V2 and V1 responses shares significant duplicate code, such as parsing fields and constructing the
EtherscanTransactionobject. Consider extracting common code into helper methods to improve maintainability and reduce potential errors.lib/src/crypto/evm/entities/contract/contract_function.dart (1)
505-515: Consider naming the extension onStringTo enhance code clarity and avoid potential naming conflicts, it is recommended to provide a name for the extension on
String. For example:extension StringParameterParsing on String { (String type, String? name) splitParam() { // ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
lib/src/crypto/evm/entities/contract/contract_function.dart(9 hunks)lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart(1 hunks)lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart(5 hunks)test/ci/parsing/contract_function_fromString_test.dart(1 hunks)
🔇 Additional comments (6)
test/ci/parsing/contract_function_fromString_test.dart (2)
1-2: LGTM!
The imports are clean and appropriate for the test file.
1-38: Verify test coverage for ExternalContractFunction.
The test implementation is clean and well-structured. Let's verify coverage of the ExternalContractFunction class.
✅ Verification successful
Test coverage for ExternalContractFunction is comprehensive
The test coverage is adequate with tests covering:
- Basic function parsing with and without parameter names
- Complex parameter types including nested arrays and tuples
- Edge cases with complex signatures like
cancelTrades((bytes32,uint256,uint256)[])andg(uint256[][],string[])
The implementation in contract_function.dart properly handles parameter extraction and type parsing, with corresponding test cases verifying the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for additional test cases and implementation details
# Find the implementation file
echo "Searching for ExternalContractFunction implementation..."
fd -e dart -e "contract_function.dart"
# Look for other test cases
echo "Checking for other test cases..."
rg -l "ExternalContractFunction" test/
Length of output: 11151
Script:
#!/bin/bash
# Let's examine the implementation and other test cases
# Check the implementation of ExternalContractFunction
echo "Checking ExternalContractFunction implementation..."
rg -A 10 "class ExternalContractFunction" lib/
# Check other test cases
echo "Checking other test cases..."
rg -A 10 "ExternalContractFunction" test/ci/parsing/fetch_function_signature_test.dart
Length of output: 2882
Script:
#!/bin/bash
# Let's check for error handling and edge cases in the implementation
# Check the fromString implementation
echo "Checking fromString implementation..."
rg -B 5 -A 15 "fromString" lib/src/crypto/evm/entities/contract/contract_function.dart
# Look for additional test cases with complex scenarios
echo "Checking for complex test scenarios..."
rg -A 5 "ExternalContractFunction.fromString" test/
Length of output: 3329
lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart (1)
153-153: LGTM! Verify callers are updated for the new record type.
The change to use record types (e.$1) improves type safety by properly structuring parameter information. However, this is a breaking change that requires updates to all callers.
Let's verify that all callers have been updated to handle the new record type:
Consider adding a comment documenting the record type structure returned by extractParams to improve code maintainability.
✅ Verification successful
Let me check the implementation of extractParams and its callers to verify the record type handling.
The record type change is properly handled across the codebase
The verification shows that:
extractParamsis correctly implemented to returnList<(String type, String? name)>records- All callers properly handle the record type:
- In
ContractFunction, the record is destructured usingfor (final (type_s, name) in params_s) - The
fromStringcalls inFunctionParamfactories are unaffected as they work with map values
- In
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to extractParams to ensure they handle the new record type
rg "extractParams" --type dart
# Search for potential direct calls to fromString that might need updates
rg "FunctionParamType\.fromString" --type dart
Length of output: 902
Script:
#!/bin/bash
# Check the implementation of extractParams and its usage
rg "extractParams" -A 10 -B 2 lib/src/crypto/evm/entities/contract/contract_function.dart
# Check how fromString is used in the contract function implementations
rg "FunctionParamType\.fromString" -A 3 -B 3 lib/src/crypto/evm/entities/contract/contract_function.dart
rg "FunctionParamType\.fromString" -A 3 -B 3 lib/src/crypto/evm/entities/contract/contract_function_param.dart
Length of output: 1776
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (1)
1-3: Imports are correctly included
All necessary imports are appropriately added, ensuring that dependencies are available.
lib/src/crypto/evm/entities/contract/contract_function.dart (2)
Line range hint 61-100: Ensure test coverage for updated parameter parsing logic
The extractParams method has been significantly modified. Please verify that unit tests cover various scenarios, including edge cases with complex types, nested tuples, and parameters with and without names.
48-51:
Ensure project compatibility with Dart 3 features
The code utilizes Dart 3 features such as record types and pattern matching (e.g., tuple destructuring in the for-loop and method returns). Please confirm that the project environment supports Dart 3 to prevent any compatibility issues.
Also applies to: 61-100, 505-515
| final contractFunction = switch (functionName) { | ||
| "" => null, | ||
| _ => ExternalContractFunction.fromString(textSignature: functionName) | ||
| }; |
There was a problem hiding this comment.
Handle exceptions when parsing functionName
The call to ExternalContractFunction.fromString(textSignature: functionName) may throw an exception if functionName is invalid or malformed. To prevent runtime errors, consider wrapping this call in a try-catch block.
Apply this diff to handle potential exceptions:
final contractFunction = switch (functionName) {
"" => null,
- _ => ExternalContractFunction.fromString(textSignature: functionName)
+ _ => {
+ try {
+ return ExternalContractFunction.fromString(textSignature: functionName);
+ } catch (e, s) {
+ Logger.logError(e, s: s, hint: "Failed to parse contract function from functionName");
+ return null;
+ }
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| for (final (type_s, name) in params_s) | ||
| FunctionParam( | ||
| name: null, | ||
| type: FunctionParamType.fromString(param), | ||
| name: name, | ||
| type: FunctionParamType.fromString(type_s), |
There was a problem hiding this comment.
Handle potential null name in FunctionParam constructor
The name parameter in FunctionParam is required and expects a non-null String, but name may be null based on the destructuring of params_s. Please ensure that name is non-null or adjust FunctionParam to accept a nullable name.
| List<ContractABI>? abis, | ||
| }) { |
There was a problem hiding this comment.
Undefined variable abiList in decodeRaw method
The variable abiList is used but not defined within the scope of the decodeRaw method. Please ensure that abiList is properly defined or passed as a parameter to the method.
Also applies to: 292-294
| @@ -251,7 +254,7 @@ | |||
| final function_selector = data.sublist(0, 4).toHex; | |||
|
|
|||
| if (function != null) { | |||
| return _decodeExternal(data: data, function: function); | |||
| return decodeExternal(data: data, function: function); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace assert statements with explicit error handling
Using assert statements for input validation may not be effective in production environments where assertions are disabled. Consider throwing exceptions or handling errors explicitly to ensure robust error handling at runtime.
Also applies to: 271-271, 286-286, 324-324, 337-337
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (25)
pubspec.yaml (3)
Line range hint
13-14: Address TODO comments for package replacementsThe TODO comments indicate that
bs58checkanddart_bech32packages need to be replaced.Would you like me to:
- Research and suggest alternative packages?
- Open GitHub issues to track these replacement tasks?
23-24: Consider publishing bip39 forkUsing a local path dependency for
bip39could make it harder for others to use this package. Consider publishing your fork to pub.dev or using a Git dependency if the changes are temporary.
Fixed version constraint for
grpcpackage should be updatedThe review comment is partially correct. While most dependencies use flexible version constraints (^), the
grpcpackage uses a fixed version 3.2.4 which is significantly behind the latest version 4.0.1. However, the specific package mentioned in the review comment (crypto: ^3.0.3) already uses a flexible version constraint.
grpc: 3.2.4should be changed to use a flexible version constraint and be updated, as it's multiple major versions behind- No security vulnerabilities were found for the grpc package
- Other dependencies are using flexible constraints and are relatively up to date
🔗 Analysis chain
Line range hint
11-12: Verify security of pinned dependency versionsSome dependencies use fixed versions (e.g., grpc: 3.2.4) which might miss security updates. Consider using flexible version constraints (^) for these packages.
Also applies to: 16-16
Let me verify the actual dependency versions in the pubspec.yaml file to compare with the latest versions we found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions # Check latest versions on pub.dev for pkg in "grpc" "http" "crypto"; do echo "Checking $pkg..." curl -s "https://pub.dev/api/packages/$pkg" | jq -r '.latest.version' done # Check GitHub advisories gh api graphql -f query=' query { securityVulnerabilities(first: 5, ecosystem: PUB, package:"grpc") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 807
Script:
#!/bin/bash # Check the actual dependency versions in pubspec.yaml cat pubspec.yamlLength of output: 692
lib/src/crypto/evm/entities/contract/contract_function_param.dart (2)
25-31: Add null safety handling in toJson methodThe current implementation might include null values in the JSON output. Consider handling null values explicitly or excluding them from the output.
Here's a suggested improvement:
Json toJson() { - return { - 'name': name, - 'indexed': indexed, - 'type': type.name, - }; + final json = <String, dynamic>{ + 'type': type.name, + }; + if (name != null) json['name'] = name; + if (indexed != null) json['indexed'] = indexed; + return json; }
Based on the code analysis, I can now generate the final response since I have enough context. The FunctionParamType implementations show that each type already has proper encode/decode methods with type validation, but these aren't being utilized in FunctionParamWithValue's serialization.
Improve type safety in FunctionParamWithValue serialization
The current implementation bypasses the type-safe encode/decode methods available in FunctionParamType. Each parameter type (address, int, bytes, etc.) has its own validation and proper serialization logic that should be used.
Json toJson() { + final serializedValue = type.encode(value); return { 'name': name, 'indexed': indexed, 'type': type.name, - 'value': value.toString(), + 'value': serializedValue.toHex, }; } factory FunctionParamWithValue.fromJson(Map map) { + final paramType = FunctionParamType.fromString(map['type']); + final rawValue = (map['value'] as String).hexToBytes; + return FunctionParamWithValue( - value: map['value'], + value: paramType.decode(rawValue), name: map['name'], indexed: map['indexed'] as bool? ?? false, - type: FunctionParamType.fromString(map['type']), + type: paramType, ); }🔗 Analysis chain
Line range hint
74-89: Improve type safety in FunctionParamWithValue serializationThe current implementation has several potential issues:
- Converting value to string in toJson may lose type information
- fromJson accepts the raw value without type validation
- No error handling for invalid value formats
Consider implementing proper type validation and serialization:
Json toJson() { + dynamic serializedValue; + // Handle different types appropriately + if (value is BigInt) { + serializedValue = value.toString(); + } else if (value is DateTime) { + serializedValue = value.toIso8601String(); + } else { + serializedValue = value; + } return { 'name': name, 'indexed': indexed, 'type': type.name, - 'value': value.toString(), + 'value': serializedValue, }; } factory FunctionParamWithValue.fromJson(Map map) { + final paramType = FunctionParamType.fromString(map['type']); + final rawValue = map['value']; + + // Parse value based on parameter type + dynamic parsedValue; + try { + parsedValue = paramType.parseValue(rawValue); + } catch (e) { + throw FormatException('Failed to parse value for type ${paramType.name}: $e'); + } + return FunctionParamWithValue( - value: map['value'], + value: parsedValue, name: map['name'], indexed: map['indexed'] as bool? ?? false, - type: FunctionParamType.fromString(map['type']), + type: paramType, ); }Let's check if FunctionParamType has proper value parsing support:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if FunctionParamType has parseValue method ast-grep --pattern 'class FunctionParamType { $$$ parseValue($_) { $$$ } $$$ }'Length of output: 93
Script:
#!/bin/bash # Let's first check the FunctionParamType class definition and available methods rg -A 10 "class FunctionParamType" # Also check for any value parsing related methods with different names rg -A 5 "value.*parse|parse.*value" --ignore-case # Let's also check the actual usage of FunctionParamWithValue to understand the value types rg -A 3 "FunctionParamWithValue\("Length of output: 12955
Script:
#!/bin/bash # Let's check the FunctionParamType implementations to understand available types rg -A 5 "class.*extends FunctionParamType" # Also check for any value validation or conversion methods in the codebase rg -A 5 "FunctionParam.*validate|convert.*value" # Let's see the actual implementations of different parameter types fd -e dart -x cat {} \; -p "parameter_type"Length of output: 52653
Script:
#!/bin/bash # Let's check for any encode/decode methods in FunctionParamType implementations rg -A 5 "encode\(.*value.*\)|decode\(" lib/src/crypto/evm/entities/contract/parameter_type/ # Also check for any value parsing or validation in the contract function param class rg -A 5 "class FunctionParam|parseValue|validateValue" lib/src/crypto/evm/entities/contract/Length of output: 98237
lib/src/crypto/evm/entities/contract/contract_abi.dart (2)
Line range hint
13-65: Consider refactoring thefromAbifactory methodThe method is quite long and handles multiple concerns (parsing functions, events, and parameters). Consider breaking it down into smaller, focused methods for better maintainability.
factory ContractABI.fromAbi(String jsonAbi) { + List<LocalContractFunction> _parseFunctions(List<dynamic> abi) { + final functions = <LocalContractFunction>[]; + for (final item in abi) { + if (_isFunction(item)) { + functions.add(_parseFunction(item)); + } + } + return functions; + } + + List<ContractEvent> _parseEvents(List<dynamic> abi) { + final events = <ContractEvent>[]; + for (final item in abi) { + if (_isEvent(item)) { + events.add(_parseEvent(item)); + } + } + return events; + } + final abi = jsonDecode(jsonAbi); - final functions = <LocalContractFunction>[]; - final events = <ContractEvent>[]; - for (final item in abi) { - // ... existing logic ... - } + final functions = _parseFunctions(abi); + final events = _parseEvents(abi); return ContractABI(functions, events); }
Line range hint
16-22: Improve error handling for invalid ABI inputThe current implementation silently skips invalid entries. Consider adding validation and throwing appropriate exceptions for malformed ABI input.
+class InvalidABIException implements Exception { + final String message; + InvalidABIException(this.message); +} + if (name == null) { - continue; + throw InvalidABIException('ABI entry is missing required "name" field'); }lib/src/crypto/evm/entities/transactions/evm_transaction.dart (2)
Line range hint
35-35: Critical: Type mismatch in copyWith method parameterThe
copyWithmethod's parameter type still uses the oldExternalContractFunctionWithValues?while the field has been updated toContractFunctionWithValues?. This mismatch could cause runtime errors.Apply this fix:
- EVMTransaction copyWith({ExternalContractFunctionWithValues? decodedInput}) { + EVMTransaction copyWith({ContractFunctionWithValues? decodedInput}) {
117-118: Fix indentation for better readabilityThe indentation of the ternary operation could be improved for better code readability.
Apply this formatting fix:
- ? ContractFunctionWithValues.fromJson(decodedInput) - : null, + ? ContractFunctionWithValues.fromJson(decodedInput) + : null,lib/src/crypto/evm/repositories/function_selector_repository.dart (1)
125-127: Consider adding error handling for invalid signaturesThe
fromTextSignatureconstructor might throw if the text signature is malformed. Consider adding try-catch block to handle potential parsing errors gracefully.for (final { "id": int _, "created_at": _, "text_signature": String text_signature, "hex_signature": _, "bytes_signature": _, } in results) { + try { return ContractFunction.fromTextSignature( textSignature: text_signature, ); + } catch (e) { + // Log the error and continue to next signature + continue; + } }lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_factory.dart (2)
220-223: Consider adding bounds checking for BigInt return valueWhile the change to
readSafeimproves reliability, we should also handle potential overflow scenarios for the returned length.Consider adding validation:
Future<BigInt> allPairsLength() async { final function = abi.getFunction("allPairsLength")!; - final result = await readSafe( - function: function.addValues(values: []), - ); - return result.outputs.first.value as BigInt; + try { + final result = await readSafe( + function: function.addValues(values: []), + ); + final length = result.outputs.first.value as BigInt; + if (length < BigInt.zero) { + throw Exception('Invalid pairs length: $length'); + } + return length; + } catch (e) { + throw Exception('Failed to get pairs length: $e'); + } }
Line range hint
211-231: Document the readSafe behavior and standardize error handlingThe migration to
readSafeimproves reliability, but we should:
- Document the expected behavior of
readSafevsread- Standardize error handling across all methods
- Consider adding retry logic for transient failures
Consider adding class-level documentation:
/// UniswapV2Factory provides a type-safe interface to interact with the Uniswap V2 Factory contract. /// All methods use [readSafe] which provides improved error handling and reliability compared to [read]. /// /// Throws: /// - [ContractReadException] when the contract call fails /// - [ArgumentError] when input validation fails /// - [Exception] for other unexpected errorslib/src/crypto/evm/entities/abi/erc20_contract.dart (1)
305-307: Document the readSafe migration and its implicationsThe consistent migration from
readtoreadSafeacross all view functions suggests an architectural improvement. However, the implications of this change should be documented.Consider adding:
- A comment explaining the benefits of
readSafeoverread- Documentation about any error handling differences
- Migration notes if this is a breaking change
class ERC20Contract extends InternalContract { + /// This implementation uses readSafe for all view functions to provide improved error handling. + /// readSafe differs from read in the following ways: + /// - <document differences here> ERC20Contract({ required super.contractAddress, required super.rpc, }) : super(Also applies to: 315-317, 325-327, 334-336, 343-345, 355-357
test/ci/parsing/reverse-hash-computation_test.dart (3)
40-46: Add test documentation for complex data structureThis test case handles a complex claim event with multiple uint256 values. Consider adding documentation to explain the data structure and expected format.
test('Test for data field with claim event', () { + // This test validates the decoding of a claim event with an array of token IDs + // Data format: claim(uint256[]) where the array contains 34 token IDs const String dataString =
100-105: Consider breaking down complex test into smaller casesThis test validates multiple parameter types in a single test case. Consider splitting it into smaller, focused test cases for better maintainability and clearer failure diagnosis.
Example refactor:
group('Complex mint function', () { late ContractFunction functionSignature; setUp(() { // Setup code here functionSignature = ContractFunction.decodeRaw(data: data, functionMap: functionMap); }); test('validates basic parameters', () { expect(functionSignature.name, "mint"); expect(functionSignature.parameters[0].name, "_to"); // ... address and amount checks }); test('validates array parameters', () { // ... bytes array and address array checks }); test('validates boolean and integer parameters', () { // ... bool and int256 checks }); });
Line range hint
1-146: Consider adding edge case testsWhile the current tests cover basic and complex function signatures, consider adding tests for:
- Invalid function signatures
- Function overloading cases
- Error cases (malformed data, missing signatures)
Would you like me to provide examples of these additional test cases?
lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_pair.dart (3)
706-708: Consider adding address validationWhile the switch to
readSafeimproves reading safety, consider adding input validation for the address parameter to prevent unnecessary contract calls with invalid addresses.Future<BigInt> balanceOf(String address) async { + if (!isValidEvmAddress(address)) { + throw ArgumentError('Invalid EVM address format'); + } final function = abi.getFunction('balanceOf')!; final response = await readSafe( function: function.addValues(values: [address]),
742-745: Consider address validation and consistent type conversion
- Consider adding input validation for both owner and spender addresses
- For consistency with other methods, consider using the same type conversion pattern as other methods
Future<BigInt> allowance({ required String owner, required String spender, }) async { + if (!isValidEvmAddress(owner) || !isValidEvmAddress(spender)) { + throw ArgumentError('Invalid EVM address format'); + } final function = abi.getFunction('allowance')!; final response = await readSafe( function: function.addValues(values: [owner, spender]), ); - return response.outputs.first.castValue<BigInt>(); + return response.outputs[0].value as BigInt; }
Line range hint
673-745: Consider implementing a base safety layerThe consistent migration to
readSafeimproves safety across all read operations. Consider implementing these safety improvements at a base contract level:
- Create a base safety layer that handles:
- Address validation
- Consistent type conversion
- Common error handling
This would ensure consistent safety measures across all contract interactions.
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (2)
Line range hint
391-407: Improve assertion error message for better debuggingThe state mutability validation logic is well-implemented, but the error message could be more descriptive to help developers understand the issue.
Consider updating the assertion message to be more specific:
- assert(valid, "Invalid value for state mutability of function"); + assert(valid, "Invalid value for function '${function.name}': ${function.stateMutability} functions ${function.stateMutability == StateMutability.payable ? 'require' : 'do not accept'} payment");
Line range hint
452-472: Verify ERC721 standard compliance and consider adding SafeTransferFromThe implementation looks good and follows the ERC721 standard for
transferFrom. However, consider also implementingsafeTransferFromas it's the recommended method for safer NFT transfers.Consider adding a
sendERC721NftSafemethod usingsafeTransferFrom:Future<String> sendERC721NftSafe({ required String recipient, required String from, required int tokenId, required String contractAddress, required Uint8List seed, Uint8List? data, }) async { final function = LocalContractFunctionWithValues( name: "safeTransferFrom", parameters: [ FunctionParamWithValue( name: "from", type: FunctionParamAddress(), value: from, ), FunctionParamWithValue( name: "to", type: FunctionParamAddress(), value: recipient, ), FunctionParamWithValue( name: "tokenId", type: FunctionParamInt(), value: tokenId, ), if (data != null) FunctionParamWithValue( name: "data", type: FunctionParamBytes(), value: data, ), ], stateMutability: StateMutability.nonpayable, outputTypes: [], ); return await interactWithContract( contractAddress: contractAddress, function: function, sender: from, seed: seed, feeInfo: EvmFeeInformation.zero, ); }lib/src/crypto/evm/entities/contract/contract_function.dart (2)
Line range hint
582-585: Handle potential UTF-8 decoding failuresThe
UTF8getter directly decodes the data without handling potential UTF-8 decoding errors. Consider wrapping the decode operation in a try-catch block to handle invalid UTF-8 data gracefully.String get UTF8 { - return utf8.decode(data); + try { + return utf8.decode(data); + } catch (e) { + return "Invalid UTF-8 data: ${data.toHex}"; + } }Also applies to: 637-640
664-716: Add documentation for complex parameter extraction logicThe parameter extraction function handles complex cases like nested tuples, but lacks documentation explaining:
- The expected input format
- How nested tuples are handled
- Edge cases and their handling
Add documentation like this:
+/// Extracts parameter types and names from a contract function signature. +/// +/// Handles complex cases including: +/// - Simple parameters: "uint256 amount" +/// - Multiple parameters: "uint256 amount, address recipient" +/// - Nested tuples: "tuple(uint256 amount, address recipient) data" +/// +/// Returns a list of tuples containing the parameter type and optional name. +/// +/// Example: +/// ```dart +/// final params = extractParams("uint256 amount, tuple(uint256 x, uint256 y) point"); +/// // Returns: [("uint256", "amount"), ("tuple(uint256,uint256)", "point")] +/// ``` List<(String type, String? name)> extractParams(String text) {lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_router.dart (1)
Line range hint
1309-1316: Consider adding gas estimation.The hardcoded gas limit and gas price in
addLiquidityTxmight not be optimal for all network conditions.Consider replacing the hardcoded values with dynamic gas estimation:
- feeInfo: EvmFeeInformation( - gasLimit: 3E5.toInt(), - gasPrice: Amount(value: 1E10.toInt().toBigInt, decimals: 18), - ), + feeInfo: await estimateGasFee(function, sender),lib/src/crypto/evm/entities/contract/internal_contract.dart (1)
Line range hint
112-128: Redundant code betweenreadandreadSafemethodsThe
readandreadSafemethods share similar logic, including state mutability checks, data preparation, RPC calls, and result decoding. This code duplication can make maintenance more challenging and increase the risk of inconsistencies.Consider refactoring the common logic into a private helper method to adhere to the DRY (Don't Repeat Yourself) principle. This enhances maintainability and readability.
For example, create a private method:
Future<T> _readInternal<T>({ required ContractFunctionWithValues function, required T Function(Uint8List resultBuffer) decodeFunction, BlockNum? atBlock, List<FunctionParam>? outputs, StateMutability? stateMutability, }) async { final _stateMutability = stateMutability ?? function.stateMutability; if (_stateMutability != StateMutability.pure && _stateMutability != StateMutability.view) { throw ArgumentError('Function is not view or pure'); } final data = function.buildDataField(); final String result = await rpc.call( contractAddress: contractAddress, data: data, atBlock: atBlock, ); final resultBuffer = result.hexToBytesWithPrefix; return decodeFunction(resultBuffer); }Then, update
readandreadSafeto use_readInternal:Future<ExternalContractFunctionWithValuesAndOutputs> read({ required ExternalContractFunctionWithValues function, BlockNum? atBlock, required List<FunctionParam> outputs, StateMutability? stateMutability, }) async { return await _readInternal( function: function, outputs: outputs, stateMutability: stateMutability, atBlock: atBlock, decodeFunction: (resultBuffer) { return ExternalContractFunctionWithValuesAndOutputs.decode( data: resultBuffer, function: function, outputs: outputs, stateMutability: stateMutability, ); }, ); } Future<LocalContractFunctionWithValuesAndOutputs> readSafe({ required LocalContractFunctionWithValues function, BlockNum? atBlock, }) async { return await _readInternal( function: function, atBlock: atBlock, decodeFunction: (resultBuffer) { return LocalContractFunctionWithValuesAndOutputs.decode( data: resultBuffer, function: function, ); }, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
lib/src/crypto/evm/entities/abi/ens/ens_registry_contract.dart(1 hunks)lib/src/crypto/evm/entities/abi/ens/ens_resolver_contract.dart(1 hunks)lib/src/crypto/evm/entities/abi/erc20_contract.dart(7 hunks)lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_factory.dart(2 hunks)lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_pair.dart(6 hunks)lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_router.dart(3 hunks)lib/src/crypto/evm/entities/contract/contract_abi.dart(3 hunks)lib/src/crypto/evm/entities/contract/contract_function.dart(7 hunks)lib/src/crypto/evm/entities/contract/contract_function_param.dart(1 hunks)lib/src/crypto/evm/entities/contract/internal_contract.dart(2 hunks)lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart(1 hunks)lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart(5 hunks)lib/src/crypto/evm/entities/transactions/evm_transaction.dart(2 hunks)lib/src/crypto/evm/repositories/function_selector_repository.dart(2 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(4 hunks)pubspec.yaml(1 hunks)test/ci/parsing/contract_function_fromString_test.dart(1 hunks)test/ci/parsing/evm_function_encoding_test.dart(1 hunks)test/ci/parsing/fetch_function_signature_test.dart(8 hunks)test/ci/parsing/reverse-hash-computation_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart
- lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart
- test/ci/parsing/contract_function_fromString_test.dart
🔇 Additional comments (25)
pubspec.yaml (1)
22-22: LGTM: Collection package addition
The addition of the collection package with a flexible version constraint is appropriate for improved data management capabilities.
lib/src/crypto/evm/entities/contract/contract_function_param.dart (1)
17-24: LGTM: Factory method properly handles JSON deserialization
The implementation correctly handles nullable fields and provides appropriate default values.
lib/src/crypto/evm/entities/contract/contract_abi.dart (2)
Line range hint 69-77: LGTM! Well-implemented getter methods
The getter methods are well-implemented with:
- Clear and descriptive naming
- Proper null-safety handling using
singleWhereOrNull - Consistent return type with the class property
6-6: Verify the impact of switching to LocalContractFunction
The change from ContractFunction to LocalContractFunction is a breaking change that could affect dependent code.
✅ Verification successful
Based on the search results, I can now generate the final response:
The change to LocalContractFunction is part of a well-structured type hierarchy
The codebase shows a consistent and intentional migration where:
ContractFunctionis the base sealed classLocalContractFunctionis a specialized implementation extendingContractFunction- The change aligns with the existing type system where
LocalContractFunctionimplementsLocalContractFunctionMixin - All usages of
ContractFunctionvsLocalContractFunctionfollow a clear pattern where external/unknown functions use the base type while internal/known functions use the local variant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of ContractFunction type
rg -t dart "ContractFunction[^a-zA-Z]" --glob '!{test,lib/src/crypto/evm/entities/contract/contract_abi.dart}'
# Search for existing usage of LocalContractFunction to ensure consistency
rg -t dart "LocalContractFunction"
Length of output: 8771
lib/src/crypto/evm/entities/transactions/evm_transaction.dart (2)
8-8: LGTM: Type change aligns with API v2 migration
The field type change from ExternalContractFunctionWithValues? to ContractFunctionWithValues? is consistent with the PR's objective to migrate to Etherscan v2 API.
Line range hint 1-124: Verify the removal of function signature caching
The removal of getFunctionSignature method and _cachedFunctionSigs map might impact performance if the caching functionality isn't handled elsewhere.
Let's verify if the caching functionality exists in other files:
lib/src/crypto/evm/repositories/function_selector_repository.dart (1)
66-66: Verify the migration impact
The change from ExternalContractFunction.fromString to ContractFunction.fromTextSignature might affect other parts of the codebase that consume this repository.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to understand the impact better.
Based on the gathered information, I can now provide the final response:
Migration from ExternalContractFunction.fromString to ContractFunction.fromTextSignature is safe
The change is safe because:
ContractFunctionis the base class thatExternalContractFunctionextends fromContractFunction.fromTextSignatureactually returns anExternalContractFunctioninstance (as seen in lib/src/crypto/evm/entities/contract/contract_function.dart)- All consumers of
fetchSelectorexpect anExternalContractFunction?return type, which remains unchanged - The type hierarchy and contract remains consistent throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of ExternalContractFunction
rg "ExternalContractFunction" --type dart
# Search for places where fetchSelector is called to verify proper type handling
ast-grep --pattern 'fetchSelector($$$)'
Length of output: 4158
Script:
#!/bin/bash
# Check how fetchSelector is used in the codebase
rg "fetchSelector\(" -A 3 --type dart
# Check the inheritance hierarchy and usage of ContractFunction
ast-grep --pattern 'class $_ extends ContractFunction'
# Look for places where the return value of fetchSelector is used
ast-grep --pattern 'fetchSelector($$$).then($$$)'
Length of output: 1577
lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_factory.dart (1)
211-214: Verify error handling for readSafe implementation
The switch from read to readSafe suggests improved error handling, but we should ensure proper error handling is in place for failed reads.
Consider wrapping the call with try-catch and providing meaningful error messages:
Future<String> getPair({
required String tokenA,
required String tokenB,
}) async {
final function = abi.getFunction('getPair')!;
- final result = await readSafe(
- function: function.addValues(values: [tokenA, tokenB]),
- );
- return result.outputs.first.value as String;
+ try {
+ final result = await readSafe(
+ function: function.addValues(values: [tokenA, tokenB]),
+ );
+ return result.outputs.first.value as String;
+ } catch (e) {
+ throw Exception('Failed to get pair address: $e');
+ }
}Let's verify the impact of this change:
lib/src/crypto/evm/entities/abi/ens/ens_registry_contract.dart (1)
Line range hint 42-48: LGTM! Verify consistent implementation across related contracts.
The switch to readSafe improves reliability of contract interactions. The existing error handling remains intact, maintaining backward compatibility.
Let's verify if similar changes are consistently applied across related ENS contracts:
✅ Verification successful
Let me check the content of the ENS resolver contract to verify if there are any remaining read calls that need to be migrated.
Let me try a different approach to check both read and readSafe usage patterns with more context:
Migration to readSafe is consistently implemented across ENS contracts
The verification confirms that both ENS Registry and ENS Resolver contracts are using readSafe consistently, with no remaining instances of the old read method. The migration has been properly completed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent use of readSafe across ENS contracts
# Expected: All ENS-related contracts should use readSafe instead of read
# Search for any remaining uses of the read method in ENS contracts
rg -l "await read\(" "lib/src/crypto/evm/entities/abi/ens"
# Search for consistent use of readSafe
rg -l "await readSafe\(" "lib/src/crypto/evm/entities/abi/ens"
Length of output: 247
Script:
#!/bin/bash
# Check the content of ENS resolver contract for any remaining read calls
rg "await read\(" lib/src/crypto/evm/entities/abi/ens/ens_resolver_contract.dart -A 2
Length of output: 87
Script:
#!/bin/bash
# Show all method calls in the resolver contract
rg "await (read|readSafe)\(" lib/src/crypto/evm/entities/abi/ens/ens_resolver_contract.dart -B 1 -A 2
# Also check the registry contract to be thorough
rg "await (read|readSafe)\(" lib/src/crypto/evm/entities/abi/ens/ens_registry_contract.dart -B 1 -A 2
Length of output: 412
lib/src/crypto/evm/entities/abi/erc20_contract.dart (1)
294-297: Verify the behavior of readSafe across the codebase
The switch from read to readSafe suggests improved error handling. Let's verify the implementation and usage consistency.
✅ Verification successful
Let me gather more information about the implementation of readSafe since the previous search didn't show it.
Based on the search results, I can now provide a final response:
The switch from read to readSafe is consistent and safe
The change is part of a systematic update across the codebase where:
readSafeis implemented in the baseInternalContractclass and is used by all contract implementations (ERC20, UniswapV2, ENS)- The method performs additional validation for state mutability (pure/view functions)
- No legacy
readcalls remain in the codebase, indicating a complete migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of read/readSafe to ensure consistent usage
echo "Searching for read method calls:"
rg "await read\(" --type dart
echo -e "\nSearching for readSafe method calls:"
rg "await readSafe\(" --type dart
# Look for the implementation of readSafe
echo -e "\nSearching for readSafe implementation:"
ast-grep --pattern 'readSafe($$$) {
$$$
}'
Length of output: 2407
Script:
#!/bin/bash
# Search for the base contract class and readSafe definition
echo "Searching for InternalContract and Contract classes:"
rg "class (Internal)?Contract" --type dart -A 5
echo -e "\nSearching for readSafe method definition:"
rg "readSafe" --type dart -B 2 -A 5
Length of output: 19668
test/ci/parsing/reverse-hash-computation_test.dart (1)
22-26: Verify function signature decoding with mock data
The test expects a "mint" function but the provided functionMap doesn't contain the corresponding signature. This test might be passing incorrectly if the function name is being derived through other means.
lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_pair.dart (3)
686-686: LGTM: Consistent safety improvements for token address retrieval
The consistent application of readSafe for both token address retrievals maintains symmetry and improves reliability.
Also applies to: 696-696
716-716: LGTM: Consistent safety improvement
The change to readSafe aligns with other view method improvements.
673-673: Verify error handling behavior of readSafe
The switch to readSafe for reading reserves is a good safety improvement, as accurate reserve values are critical for pool operations. However, we should verify the error handling behavior.
✅ Verification successful
Switch to readSafe is a valid improvement
The change from read to readSafe in the UniswapV2Pair contract is a valid improvement. The readSafe method is implemented in the InternalContract class and is specifically designed for read-only operations (view/pure functions), providing better type safety and error handling for contract interactions. This change has been consistently applied across all read-only methods in the contract including getReserves, token0, token1, balanceOf, totalSupply, and allowance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for readSafe implementation and error handling
ast-grep --pattern 'readSafe($$$) {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Let's try searching for readSafe method using ripgrep for better coverage
rg "readSafe" -A 5 -B 5
Length of output: 20144
Script:
#!/bin/bash
# Let's search for the read method to understand the difference between read and readSafe
rg "read\(" -A 5 -B 5 lib/src/crypto/evm/entities/contract/internal_contract.dart
Length of output: 393
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (1)
370-370: Verify the impact of LocalContractFunctionWithValues usage
The change from ContractFunctionWithValues to LocalContractFunctionWithValues looks good, but we should ensure all callers have been updated accordingly.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The search shows that LocalContractFunctionWithValues is properly defined and used consistently in the codebase, particularly in the RPC interface and contract-related files. The type hierarchy and usage patterns indicate this is a deliberate design choice where LocalContractFunctionWithValues extends ContractFunctionWithValues.
Parameter type change is consistent with the codebase design
The change to LocalContractFunctionWithValues aligns with the type hierarchy in contract_function.dart where it's a specialized version of ContractFunctionWithValues. The codebase shows proper handling of both local and external contract functions with clear separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of ContractFunctionWithValues to ensure complete migration
rg "ContractFunctionWithValues" --type dart
# Search for readContract method calls to verify they've been updated
ast-grep --pattern 'readContract($$$function: $_$$$)'
Length of output: 6309
lib/src/crypto/evm/entities/abi/ens/ens_resolver_contract.dart (1)
694-696: LGTM! Consistent with the migration pattern.
The change from read to readSafe aligns with similar updates across other contract classes, improving the robustness of blockchain data retrieval.
Let's verify the consistent usage of readSafe across other contract files:
✅ Verification successful
Migration to readSafe is complete and consistent across the codebase
The verification shows that:
- No instances of the old
readmethod remain in the EVM contracts readSafeis consistently used across all contract classes including:- ERC20 Contract
- Uniswap V2 contracts (Pair, Router, Factory)
- ENS contracts (Registry, Resolver)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old `read` method
# that might have been missed during the migration.
# Search for remaining instances of the old read method in contract files
rg -g '*.dart' 'await\s+read\(' lib/src/crypto/evm/
# Search for the new readSafe method to confirm consistent adoption
rg -g '*.dart' 'await\s+readSafe\(' lib/src/crypto/evm/
Length of output: 2147
lib/src/crypto/evm/entities/contract/contract_function.dart (4)
Line range hint 9-74: Well-designed class hierarchy with proper encapsulation!
The sealed class hierarchy provides strong type safety and clear separation of concerns. The implementation follows SOLID principles, particularly the Interface Segregation Principle through the use of mixins.
Also applies to: 250-288, 290-383
201-246: Excellent error handling implementation!
The decoding logic includes comprehensive error handling with:
- Data length validation
- Function selector verification
- Proper exception handling with specific exception types
- Graceful fallback to NotDecodableContractFunction
86-89: Previous review comment is still valid
The name parameter in FunctionParam constructor is required but may be null when destructuring params_s.
46-53: Address commented-out type checking logic
The commented-out type checking code suggests there might be runtime type checking requirements that aren't being enforced. Consider either:
- Implementing the type checking with proper null safety
- Documenting why the type checking was deemed unnecessary
lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_router.dart (2)
994-997: LGTM! Migration to safer read operations.
The change from read to readSafe across these methods improves error handling for blockchain interactions.
Let's verify if there are any remaining unsafe read operations in related files:
Also applies to: 1009-1012, 1269-1276
✅ Verification successful
Let me try a different approach to verify the migration from read to readSafe in the codebase. This will help us understand if there are any remaining unsafe read operations and confirm the consistency of the migration.
Migration to readSafe is complete and consistent across the codebase
The verification shows that all contract interactions are using the safer readSafe method. There are no remaining instances of unsafe read calls, and the migration appears to be complete across all related contract files including ERC20, UniswapV2 (Factory, Pair, Router), and ENS contracts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining unsafe read operations in contract interactions
# Expected: No direct usage of 'read' method in contract interactions
# Search for remaining unsafe read operations
rg -l 'await\s+read\(' --type dart
# Search for readSafe usage to confirm migration
rg -l 'await\s+readSafe\(' --type dart
Length of output: 141
Script:
#!/bin/bash
# Search for read method calls in dart files
rg 'await\s+read\s*\(' --type dart
# Search for readSafe method calls in dart files
rg 'await\s+readSafe\s*\(' --type dart
# Search for method definitions to understand the context
ast-grep --pattern 'read($$$) {
$$$
}'
ast-grep --pattern 'readSafe($$$) {
$$$
}'
Length of output: 2203
Line range hint 1269-1276: Verify error handling in readSafe implementation.
The migration to readSafe suggests improved error handling, but we should verify the implementation.
Let's check the implementation of readSafe:
Also applies to: 994-997, 1009-1012
test/ci/parsing/fetch_function_signature_test.dart (3)
23-23: LGTM: Consistent method call updates
The changes consistently replace ContractFunctionWithValues.decodeRawWithFetch with ContractFunction.decodeRawWithFetch while maintaining the same parameters and expected test outcomes.
Also applies to: 40-40, 62-62
86-89: LGTM: Improved code formatting
The multi-line formatting of the method call improves readability while maintaining the same functionality.
151-155: Verify the function map entries
The addition of the functionMap parameter provides explicit function signature mappings. Ensure that these mappings are complete and accurate for all possible function signatures used in the tests.
Also applies to: 424-428
✅ Verification successful
Function map entries are correctly defined and used in tests
The verification shows that the function map entries in the test file are accurate and complete:
- The function signature
0x3f8fc233is correctly mapped tocancelTrades((bytes32,uint256,uint256)[])in the test case. - All other function signatures in the test file are either:
- Verified through direct decoding without explicit mapping
- Used as test data with expected results matching their actual implementations
The functionMap parameter is appropriately used only when needed for specific test cases, while allowing the default signature fetching mechanism to work for other cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify function signatures in the test file match the functionMap entries
# Search for all function signatures in hex format
echo "Function signatures in test file:"
rg -o '0x[0-9a-f]{8}' test/ci/parsing/fetch_function_signature_test.dart
# Search for all functionMap declarations
echo -e "\nFunction maps in test file:"
rg 'functionMap: \{.*?\}' test/ci/parsing/fetch_function_signature_test.dart
Length of output: 664
Script:
#!/bin/bash
# Let's try a different approach to get the function maps by searching for more specific patterns
# and including surrounding context
# Search for ContractFunction.decodeRawWithFetch calls with context
echo "Function map declarations in decodeRawWithFetch calls:"
rg -U "ContractFunction\.decodeRawWithFetch\([\s\S]*?\{[\s\S]*?\}" test/ci/parsing/fetch_function_signature_test.dart
# Also search for any other potential function signature mappings
echo -e "\nOther function signature mappings:"
rg "\"0x[0-9a-f]{8}\": \"[^\"]+\"" test/ci/parsing/fetch_function_signature_test.dart
Length of output: 19521
| @@ -25,7 +25,7 @@ void main() { | |||
| "d06ca61f00000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000200000000000000000000000074dc1c4ec10abe9f5c8a3eabf1a90b97cdc3ead8000000000000000000000000f1ca9cb74685755965c7458528a36934df52a3ef", | |||
| ); | |||
|
|
|||
| final decodedFunction = ContractFunctionWithValues.decode( | |||
| final decodedFunction = ContractFunction.decode( | |||
There was a problem hiding this comment.
💡 Codebase verification
API refactoring is incomplete - ContractFunctionWithValues still exists
The change from ContractFunctionWithValues.decode to ContractFunction.decode in the test file is part of an incomplete refactoring. The codebase still heavily uses ContractFunctionWithValues in multiple files:
lib/src/crypto/evm/entities/contract/contract_function.dart: Contains the core class definitions and implementationslib/src/crypto/evm/entities/transactions/evm_transaction.dart: Uses it for transaction input decodinglib/src/crypto/evm/entities/contract/internal_contract.dart: Uses it for contract interactions- Other files also contain dependencies on this class
🔗 Analysis chain
LGTM! Clean API refactoring.
The change from ContractFunctionWithValues.decode to ContractFunction.decode simplifies the API while maintaining the same functionality, as evidenced by the passing assertions.
Let's verify that this refactoring has been consistently applied across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of ContractFunctionWithValues
# that might have been missed during the refactoring
# Search for any remaining references to ContractFunctionWithValues
echo "Checking for remaining ContractFunctionWithValues references..."
rg "ContractFunctionWithValues"
# Search for all ContractFunction.decode usages to ensure consistent usage
echo "Checking ContractFunction.decode usage patterns..."
rg "ContractFunction\.decode"
Length of output: 8162
| @@ -63,7 +63,7 @@ class FunctionSelectorRepository { | |||
| if (name == null) return null; | |||
|
|
|||
| final function = | |||
| ExternalContractFunction.fromString(textSignature: name); | |||
| ContractFunction.fromTextSignature(textSignature: name); | |||
There was a problem hiding this comment.
Critical: Return type mismatch with implementation
The methods are declared to return ExternalContractFunction? but are now returning ContractFunction. This type mismatch could lead to runtime errors.
Apply these changes to fix the type declarations:
- static Future<ExternalContractFunction?> fetchSelector(
+ static Future<ContractFunction?> fetchSelector(
- static Future<ExternalContractFunction?> fetchSelectorOpenChain(
+ static Future<ContractFunction?> fetchSelectorOpenChain(
- static Future<ExternalContractFunction?> fetchSelector4Byte(
+ static Future<ContractFunction?> fetchSelector4Byte(
- static ExternalContractFunction? getLowestIdFunction(
+ static ContractFunction? getLowestIdFunction(Also update the cache type:
- Map<String, ExternalContractFunction> _functionCache = {};
+ Map<String, ContractFunction> _functionCache = {};Also applies to: 125-127
| final result = await readSafe( | ||
| function: function.addValues(values: [index]), | ||
| ); | ||
| return result.outputs.first.value as String; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for index parameter
The index parameter should be validated against the total pairs length to prevent out-of-bounds access.
Consider adding validation:
Future<String> allPairs(BigInt index) async {
+ final length = await allPairsLength();
+ if (index < BigInt.zero || index >= length) {
+ throw ArgumentError('Index out of bounds: $index (total pairs: $length)');
+ }
final function = abi.getFunction("allPairs")!;
try {
final result = await readSafe(
function: function.addValues(values: [index]),
);
return result.outputs.first.value as String;
} catch (e) {
throw Exception('Failed to get pair at index $index: $e');
}
}📝 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.
| final result = await readSafe( | |
| function: function.addValues(values: [index]), | |
| ); | |
| return result.outputs.first.value as String; | |
| final length = await allPairsLength(); | |
| if (index < BigInt.zero || index >= length) { | |
| throw ArgumentError('Index out of bounds: $index (total pairs: $length)'); | |
| } | |
| final function = abi.getFunction("allPairs")!; | |
| try { | |
| final result = await readSafe( | |
| function: function.addValues(values: [index]), | |
| ); | |
| return result.outputs.first.value as String; | |
| } catch (e) { | |
| throw Exception('Failed to get pair at index $index: $e'); | |
| } |
| final function = abi.functions[5]; | ||
| assert(function.functionSelectorHex == "70a08231"); | ||
| final response = await read( | ||
| final response = await readSafe( | ||
| function: function.addValues(values: []), | ||
| ); |
There was a problem hiding this comment.
Critical: balanceOf method is not using the address parameter
The balanceOf method is not passing the required address parameter to the contract function call, which will result in incorrect behavior.
Fix the parameter passing:
Future<BigInt> balanceOf({required String address}) async {
final function = abi.functions[5];
assert(function.functionSelectorHex == "70a08231");
final response = await readSafe(
- function: function.addValues(values: []),
+ function: function.addValues(values: [address]),
);
return response.outputs.first.castValue<BigInt>();
}📝 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.
| final function = abi.functions[5]; | |
| assert(function.functionSelectorHex == "70a08231"); | |
| final response = await read( | |
| final response = await readSafe( | |
| function: function.addValues(values: []), | |
| ); | |
| final function = abi.functions[5]; | |
| assert(function.functionSelectorHex == "70a08231"); | |
| final response = await readSafe( | |
| function: function.addValues(values: [address]), | |
| ); |
| @@ -4,6 +4,7 @@ import 'package:test/test.dart'; | |||
| import 'package:walletkit_dart/walletkit_dart.dart'; | |||
|
|
|||
| void main() { | |||
| const functionMap = {"asd": "asd"}; | |||
There was a problem hiding this comment.
Replace mock function map with realistic test data
The current functionMap contains placeholder values that don't match the actual function signatures being tested (e.g., "mint"). Consider using realistic function signatures to ensure proper testing.
- const functionMap = {"asd": "asd"};
+ const functionMap = {
+ "0x40c10f19": "mint(address,uint256)", // For the first test
+ "0x6ba4c138": "claim(uint256[])", // For the second test
+ "0xd63a47d6": "mint(address,uint256,bytes[],bool,address[],int256)" // For the third test
+ };📝 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.
| const functionMap = {"asd": "asd"}; | |
| const functionMap = { | |
| "0x40c10f19": "mint(address,uint256)", // For the first test | |
| "0x6ba4c138": "claim(uint256[])", // For the second test | |
| "0xd63a47d6": "mint(address,uint256,bytes[],bool,address[],int256)" // For the third test | |
| }; |
| return ExternalContractFunctionWithValuesAndOutputs.decode( | ||
| data: resultBuffer, | ||
| function: function, | ||
| outputs: _outputs, | ||
| stateMutability: stateMutability, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent stateMutability when decoding
You are passing the possibly null stateMutability to the decode method (line 108~), which may lead to issues if stateMutability is null. Since _stateMutability is guaranteed to be non-null after the earlier assignment and checks, it would be safer to use it instead.
Replace stateMutability with _stateMutability when calling decode:
return ExternalContractFunctionWithValuesAndOutputs.decode(
data: resultBuffer,
function: function,
outputs: _outputs,
- stateMutability: stateMutability,
+ stateMutability: _stateMutability,
);📝 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.
| return ExternalContractFunctionWithValuesAndOutputs.decode( | |
| data: resultBuffer, | |
| function: function, | |
| outputs: _outputs, | |
| stateMutability: stateMutability, | |
| return ExternalContractFunctionWithValuesAndOutputs.decode( | |
| data: resultBuffer, | |
| function: function, | |
| outputs: _outputs, | |
| stateMutability: _stateMutability, |
| final data = function.buildDataField(); | ||
|
|
||
| final String result = | ||
| await rpc.call(contractAddress: contractAddress, data: data); |
There was a problem hiding this comment.
atBlock parameter is not utilized in RPC call
The atBlock parameter is accepted in the method signature (line 79~) but is not used when invoking rpc.call (line 100~). This may lead to unintended behavior if the caller expects the call to be made at a specific block.
Pass the atBlock parameter to rpc.call to ensure the RPC call is made at the correct block number:
final String result =
- await rpc.call(contractAddress: contractAddress, data: data);
+ await rpc.call(contractAddress: contractAddress, data: data, atBlock: atBlock);Ensure that the rpc.call method is capable of handling the atBlock parameter.
Committable suggestion skipped: line range outside the PR's diff.
| required List<FunctionParam>? outputs, | ||
| StateMutability? stateMutability, | ||
| }) async { | ||
| assert( | ||
| function.stateMutability == StateMutability.pure || | ||
| function.stateMutability == StateMutability.view, | ||
| "Function is not view or pure", | ||
| final _stateMutability = stateMutability ?? function.stateMutability; | ||
|
|
||
| if (_stateMutability != StateMutability.pure && | ||
| _stateMutability != StateMutability.view) { | ||
| throw ArgumentError('Function is not view or pure'); | ||
| } | ||
|
|
||
| final _outputs = outputs ?? function.outputTypes; | ||
|
|
||
| if (_outputs == null || _outputs.isEmpty) { | ||
| throw ArgumentError('Outputs must be provided'); | ||
| } |
There was a problem hiding this comment.
Nullability inconsistency of outputs parameter
The outputs parameter is declared as List<FunctionParam>? (line 81~), but the method throws an ArgumentError when _outputs is null or empty (lines 93-95~). Since _outputs is assigned from outputs ?? function.outputTypes, both can be null, leading to potential runtime errors.
Consider making the outputs parameter non-nullable to ensure it is always provided:
- required List<FunctionParam>? outputs,
+ required List<FunctionParam> outputs,Alternatively, ensure that function.outputTypes is not null, or handle the null case appropriately within the method.
Committable suggestion skipped: line range outside the PR's diff.
| final _stateMutability = stateMutability ?? function.stateMutability; | ||
|
|
||
| if (_stateMutability != StateMutability.pure && | ||
| _stateMutability != StateMutability.view) { | ||
| throw ArgumentError('Function is not view or pure'); | ||
| } |
There was a problem hiding this comment.
Potential mismatch in state mutability handling
The _stateMutability variable (line 84~) defaults to function.stateMutability if stateMutability is null. The subsequent check (lines 86-89~) throws an error if _stateMutability is not pure or view. However, if both stateMutability and function.stateMutability are null, this will lead to a runtime error.
Ensure that function.stateMutability is not null, or provide a default value to prevent unexpected errors.
final _stateMutability = stateMutability ?? function.stateMutability;
+assert(_stateMutability != null, 'State mutability must be provided');Committable suggestion skipped: line range outside the PR's diff.
| required LocalContractFunctionWithValues function, | ||
| BlockNum? atBlock, | ||
| }) async { | ||
| if (function.stateMutability != StateMutability.pure && | ||
| function.stateMutability != StateMutability.view) { | ||
| throw ArgumentError('Function is not view or pure'); |
There was a problem hiding this comment.
atBlock parameter not used in readSafe method
The atBlock parameter is accepted in the readSafe method (lines 113-115~) but not utilized in the rpc.call invocation. This could lead to unexpected behavior for callers who specify this parameter.
Include the atBlock parameter in the RPC call to ensure the method behaves as expected:
final String result =
- await rpc.call(contractAddress: contractAddress, data: data);
+ await rpc.call(contractAddress: contractAddress, data: data, atBlock: atBlock);Ensure that rpc.call can handle the atBlock parameter appropriately.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/ci/parsing/reverse-hash-computation_test.dart (1)
45-46: LGTM! Consider enhancing array validation.While the test correctly validates the array contents, consider adding assertions for the array length and type to ensure complete validation of the decoded data structure.
expect(functionSignature, isNotNull); + expect(functionSignature.parameters.first.type, FunctionParamUint256Array()); + expect(functionSignature.parameters.first.value.length, 34); expect( functionSignature.parameters.first.value, [lib/src/crypto/evm/entities/contract/contract_function.dart (2)
Line range hint
591-594: Add error handling for UTF-8 decodingThe UTF8 getter doesn't handle invalid UTF-8 data which could throw FormatException.
Add error handling for both UnknownContractFunction and NotDecodableContractFunction:
String get UTF8 { - return utf8.decode(data); + try { + return utf8.decode(data); + } on FormatException { + return '<invalid UTF-8>'; + } }Also applies to: 645-648
674-726: Add documentation for the parameter extraction logicThe parameter extraction logic is complex and handles nested parentheses, but lacks documentation explaining the algorithm and edge cases.
Add comprehensive documentation:
+/// Extracts parameter types and names from a function signature string. +/// +/// Handles complex cases including: +/// - Nested parentheses in tuple types +/// - Optional parameter names +/// - Multiple parameters separated by commas +/// +/// Example: +/// ```dart +/// extractParams("uint256 amount, address recipient") +/// => [(uint256, amount), (address, recipient)] +/// ``` List<(String type, String? name)> extractParams(String text) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/src/crypto/evm/entities/contract/contract_function.dart(6 hunks)test/ci/parsing/fetch_function_signature_test.dart(8 hunks)test/ci/parsing/reverse-hash-computation_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ci/parsing/fetch_function_signature_test.dart
🔇 Additional comments (6)
test/ci/parsing/reverse-hash-computation_test.dart (3)
7-12: LGTM! Successfully addressed the previous review comment.
The functionMap now contains realistic function signatures that properly match the test cases, improving the test's reliability and documentation value.
27-28: LGTM! Clean migration to new decoding method.
The update to ContractFunction.decodeRaw with proper function map integration maintains test coverage while improving code structure.
105-106: LGTM! Comprehensive test coverage.
The test case thoroughly validates complex parameter decoding, including named parameters, arrays, and various data types, providing excellent coverage of the decoder's capabilities.
lib/src/crypto/evm/entities/contract/contract_function.dart (3)
91-94:
Handle potential null name in parameter extraction
The parameter extraction process might return null names, but FunctionParam constructor expects non-null names.
This issue was previously identified in the past reviews. Apply this fix:
- for (final (type_s, name) in params_s)
+ for (final (type_s, name) in params_s) {
+ final paramName = name ?? 'param${params.length}';
FunctionParam(
- name: name,
+ name: paramName,
type: FunctionParamType.fromString(type_s),
),
+ }268-271: 🛠️ Refactor suggestion
Replace assertion with explicit validation in buildDataField
Using assertions for runtime validation is not recommended as they may be disabled in production.
Replace the assertion with explicit validation:
Uint8List buildDataField() {
final dataFieldBuilder = DataFieldBuilder.fromFunction(function: this);
- return dataFieldBuilder.buildDataField();
+ final result = dataFieldBuilder.buildDataField();
+ if (result.isEmpty) {
+ throw StateError('Failed to build data field');
+ }
+ return result;
}46-53: 🛠️ Refactor suggestion
Uncomment and fix the type validation logic
The commented type checking code should either be properly implemented or removed. Having commented code suggests uncertainty about the type validation approach.
Let's check if there are any type-related issues reported in the codebase:
Consider implementing a more robust type validation using Dart's type system:
- // if (param.type.internalType != value.runtimeType) {
- // if (param.type.internalType == BigInt &&
- // value.runtimeType.toString() == "_BigIntImpl") {
- // } else
- // throw Exception(
- // "Invalid type for param: ${param.name}. Expected: ${param.type.internalType} Got: ${value.runtimeType}",
- // );
- // }
+ if (!_isValidType(param.type.internalType, value)) {
+ throw ArgumentError(
+ "Invalid type for param: ${param.name}. Expected: ${param.type.internalType} Got: ${value.runtimeType}",
+ );
+ }…pp/walletkit-dart into migrate-to-etherscanV2Api
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
lib/src/crypto/evm/entities/transactions/evm_transaction.dart (1)
117-118: Minor: Consider improving the indentation for better readability.While the logic is correct, the indentation could be improved:
- ? ContractFunctionWithValues.fromJson(decodedInput) - : null, + ? ContractFunctionWithValues.fromJson(decodedInput) + : null,test/ci/evm/evm_explorer_test.dart (3)
114-114: Remove debug print statements.Debug print statements should be removed from test cases as they add noise to test output. Consider using test logging frameworks if debugging information is needed.
- print('BNB Balance: $balance'); - print('Zeniq BSC Balance: $zeniqBSCBalance'); - print('BNB Transactions: $transactions'); - print('Zeniq BSC Transactions: $zeniqBSCTokenTransactions'); - print('ERC721: $erc721');Also applies to: 119-119, 131-131, 140-140, 148-148
204-226: Consider adding error handling.The test could benefit from proper error handling to provide better feedback when API calls fail. Consider wrapping the API calls in try-catch blocks.
test('Test Base Fetching', () async { + try { final balance = await basescan.fetchBalance(address: arbitrumTestWallet); expect(balance, greaterThanOrEqualTo(BigInt.zero)); // ... rest of the test ... + } catch (e) { + fail('Base API call failed: $e'); + } });
Line range hint
1-1: Consider adding rate limiting tests.The test suite would benefit from additional test cases that verify proper handling of API rate limiting scenarios, as blockchain explorer APIs typically have rate limits.
Would you like me to help create test cases that verify rate limiting behavior?
lib/src/crypto/evm/entities/contract/contract_function.dart (3)
280-304: Enhance error handling in fromJson factory methodThe error handling in the fromJson factory method could be more informative by providing specific error messages for different failure cases.
Consider enhancing the error handling:
factory ContractFunctionWithValues.fromJson(Map json) { if (json case { "name": String name, "parameters": List<dynamic> parameters, }) { return ExternalContractFunctionWithValues( name: name, parameters: [ for (final param in parameters) FunctionParamWithValue.fromJson(param), ], ); } if (json case {"name": String name, "data": String data}) { final dataBytes = data.hexToBytes; return switch (name) { "Unknown" => UnknownContractFunction(data: dataBytes), "NotDecodable" => NotDecodableContractFunction(data: dataBytes), - _ => throw Exception("Invalid json"), + _ => throw FormatException("Invalid contract function name: $name"), }; } - throw Exception("Invalid json"); + throw FormatException( + "Invalid JSON format. Expected 'name' and either 'parameters' or 'data' fields. Got: ${json.keys}", + ); }
333-348: Consider adding validation for decoded outputs lengthThe decode factory methods in both LocalContractFunctionWithValuesAndOutputs and ExternalContractFunctionWithValuesAndOutputs should validate that the number of decoded outputs matches the expected number of output types.
Add validation in both decode factory methods:
factory LocalContractFunctionWithValuesAndOutputs.decode({ required LocalContractFunctionWithValues function, required Uint8List data, }) { final decodedOutputs = decodeDataField( data: data, params: function.outputTypes, ); + + if (decodedOutputs.length != function.outputTypes.length) { + throw FunctionDecodingException( + "Decoded outputs length (${decodedOutputs.length}) does not match output types length (${function.outputTypes.length})", + ); + } return LocalContractFunctionWithValuesAndOutputs( name: function.name, parameters: function.parameters, outputs: decodedOutputs, stateMutability: function.stateMutability, ); }Apply similar changes to ExternalContractFunctionWithValuesAndOutputs.decode.
Also applies to: 373-391
681-733: Enhance parameter extraction robustnessThe extractParams function could benefit from additional validation and edge case handling:
- Empty parameter names
- Malformed nested tuples
- Unmatched parentheses
Consider adding these validations:
List<(String type, String? name)> extractParams(String text) { text = text.trim(); + if (text.isEmpty) { + return []; + } + + // Validate balanced parentheses + var openCount = 0; + for (var char in text.characters) { + if (char == '(') openCount++; + if (char == ')') openCount--; + if (openCount < 0) { + throw FormatException('Unmatched closing parenthesis in parameters'); + } + } + if (openCount != 0) { + throw FormatException('Unmatched opening parenthesis in parameters'); + } var opening = text.indexOf("("); // ... rest of the implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
lib/src/crypto/evm/entities/contract/contract_function.dart(6 hunks)lib/src/crypto/evm/entities/transactions/evm_transaction.dart(3 hunks)lib/src/crypto/evm/repositories/function_selector_repository.dart(2 hunks)test/ci/evm/evm_explorer_test.dart(3 hunks)test/ci/parsing/contract_function_fromString_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ci/parsing/contract_function_fromString_test.dart
🔇 Additional comments (7)
lib/src/crypto/evm/entities/transactions/evm_transaction.dart (3)
Line range hint 37-54: LGTM! Proper implementation of the immutable copy pattern.
The copyWith method correctly handles all fields and maintains immutability.
8-8: Verify the impact of the contract function type change.
The type change from ExternalContractFunctionWithValues? to ContractFunctionWithValues? is a breaking change. Let's ensure all consumers of this field are updated.
Line range hint 1-123: Verify the new caching strategy for function signatures.
The removal of _cachedFunctionSigs and getFunctionSignature() suggests a change in the caching strategy. Please ensure that function signature caching is handled appropriately elsewhere to maintain performance.
lib/src/crypto/evm/repositories/function_selector_repository.dart (2)
Line range hint 10-10: Critical: Type mismatch in cache and return types
The cache and method signatures are still using ExternalContractFunction while the implementation now returns ContractFunction. This type mismatch will cause runtime errors.
Apply these changes to fix the type declarations:
- Map<String, ExternalContractFunction> _functionCache = {};
- static Future<ExternalContractFunction?> fetchSelector(
+ Map<String, ContractFunction> _functionCache = {};
+ static Future<ContractFunction?> fetchSelector(Also applies to: 11-14
66-68:
Critical: Potential null dereference
The null check after fromTextSignature suggests this method can return null, but this isn't clear from the code. Consider adding explicit error handling or documentation about when null might be returned.
Let's verify the null return cases in ContractFunction:
test/ci/evm/evm_explorer_test.dart (2)
110-117: Add balance value assertions as previously suggested.
Following the pattern in other explorer tests and addressing the previous review comment, add assertions to verify the balance values.
final balance = await bscscan.fetchBalance(
address: rejectEVM,
);
+ expect(balance, greaterThanOrEqualTo(BigInt.zero));
final zeniqBSCBalance = await bscscan.fetchTokenBalance(
address: rejectEVM, contractAddress: zeniqBSCToken.contractAddress);
+ expect(zeniqBSCBalance, greaterThanOrEqualTo(BigInt.zero));301-301: LGTM! Appropriate assertion change.
The modified assertion using greaterThanOrEqualTo is more appropriate as it correctly handles the case where the balance could be zero.
| return ContractFunction.fromTextSignature( | ||
| textSignature: text_signature, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling parsing errors
The function directly returns the result of fromTextSignature without any error handling. If the text signature is malformed, this could silently fail.
Consider adding error handling:
- return ContractFunction.fromTextSignature(
- textSignature: text_signature,
- );
+ try {
+ return ContractFunction.fromTextSignature(
+ textSignature: text_signature,
+ );
+ } catch (e) {
+ // Log the error or handle malformed signatures
+ return null;
+ }Committable suggestion skipped: line range outside the PR's diff.
| static ContractFunctionWithValues decodeRaw({ | ||
| required Uint8List data, | ||
| required Map<String, String> functionMap, | ||
| }) { | ||
| assert(data.length >= 4, "Invalid data length"); | ||
| if (data.length < 4) return UnknownContractFunction(data: data); | ||
|
|
||
| final function_selector = data.sublist(0, 4).toHex; | ||
| final hex_signature = "0x${data.sublist(0, 4).toHex}"; | ||
|
|
||
| final validAbis = abiList.where( | ||
| (abi) => abi.getFunctionFromSelector(function_selector) != null, | ||
| ); | ||
| final text_signarure = functionMap[hex_signature]; | ||
|
|
||
| for (final abi in validAbis) { | ||
| final function = abi.getFunctionFromSelector(function_selector)!; | ||
| try { | ||
| return ContractFunctionWithValues.decode( | ||
| data: data, | ||
| function: function, | ||
| ); | ||
| } catch (e) { | ||
| Logger.logWarning( | ||
| "Error decoding function: ${function.name} for abi: ${abi.toString()}", | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
| if (text_signarure == null) return UnknownContractFunction(data: data); | ||
|
|
||
| return null; | ||
| final function = | ||
| ContractFunction.fromTextSignature(textSignature: text_signarure); | ||
|
|
||
| if (function == null) return UnknownContractFunction(data: data); | ||
|
|
||
| return ContractFunction.decode( | ||
| data: data, | ||
| function: function, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in decodeRaw method
The decodeRaw method should provide more detailed error information when decoding fails. Currently, it silently returns UnknownContractFunction for any failure.
Consider enhancing the error handling:
static ContractFunctionWithValues decodeRaw({
required Uint8List data,
required Map<String, String> functionMap,
}) {
- if (data.length < 4) return UnknownContractFunction(data: data);
+ if (data.length < 4) {
+ return NotDecodableContractFunction(
+ data: data,
+ error: FunctionDecodingException("Data length less than 4 bytes"),
+ );
+ }
final hex_signature = "0x${data.sublist(0, 4).toHex}";
final text_signarure = functionMap[hex_signature];
- if (text_signarure == null) return UnknownContractFunction(data: data);
+ if (text_signarure == null) {
+ return UnknownContractFunction(
+ data: data,
+ error: "Function signature not found in map",
+ );
+ }
final function = ContractFunction.fromTextSignature(textSignature: text_signarure);
- if (function == null) return UnknownContractFunction(data: data);
+ if (function == null) {
+ return NotDecodableContractFunction(
+ data: data,
+ error: "Failed to parse function signature",
+ );
+ }
return ContractFunction.decode(
data: data,
function: function,
);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
lib/src/crypto/evm/evm.dart (1)
41-42: Consider organizing the function selector repository export.For better code organization and maintainability, consider:
- Moving this export under a relevant category (e.g., under "Contract" since it deals with contract function selectors)
- Adding a comment header to create a new category if it doesn't fit existing ones
// ERC20 export 'entities/abi/erc20_contract.dart'; // ERC721 export 'entities/abi/erc721_contract.dart'; // ENS export 'entities/abi/ens/ens_registry_contract.dart'; export 'entities/abi/ens/ens_resolver_contract.dart'; // Uniswap V2 export 'entities/abi/uniswap_v2/uniswap_v2_factory.dart'; export 'entities/abi/uniswap_v2/uniswap_v2_pair.dart'; export 'entities/abi/uniswap_v2/uniswap_v2_router.dart'; -export 'repositories/function_selector_repository.dart'; +/// Function Selectors +export 'repositories/function_selector_repository.dart';lib/src/crypto/evm/repositories/function_selector_repository.dart (1)
Line range hint
11-145: Add documentation and improve error handlingConsider the following improvements:
- Add class-level documentation explaining the purpose and usage of
FunctionSelectorRepository- Document the singleton pattern usage
- Add error handling for network failures and invalid responses
- Consider adding logging instead of print statements for debugging
Example class documentation:
/// Repository for fetching and caching EVM function selectors from various sources. /// /// Uses a singleton pattern to maintain a global cache of function selectors. /// Supports fetching from OpenChain and 4byte.directory APIs. /// /// Example: /// ```dart /// final repo = FunctionSelectorRepository(); /// final function = await repo.fetchSelector("0x..."); /// ```lib/src/crypto/evm/entities/contract/contract_function.dart (7)
160-160: Remove debug print statementThe debug print statement should be removed as it serves no purpose in production code.
- print("a");
238-243: Add explanation for data paddingThe data padding logic modifies the input without explaining why. Consider adding a comment explaining that EVM requires data to be padded to 32-byte boundaries.
if (dataWithoutSelector.length % 32 != 0) { + // Pad data to 32-byte boundary as required by EVM dataWithoutSelector = dataWithoutSelector.sublist( 0, dataWithoutSelector.length - (dataWithoutSelector.length % 32), ); }
486-491: Consider using DateTime instead of int for timestampUsing a raw integer for timestamp makes the code less type-safe and harder to work with. Consider using DateTime directly.
- final int timeStamp; + final DateTime timeStamp; const ErrorContractFunctionMixin({ - required this.timeStamp, + DateTime? timeStamp, - }); + }) : timeStamp = timeStamp ?? DateTime.now();
674-684: Enhance String extension documentationThe
splitParamextension method lacks documentation explaining its purpose and return value format.extension on String { + /// Splits a parameter string into type and name components. + /// + /// For example: + /// - "uint256 amount" returns ("uint256", "amount") + /// - "address" returns ("address", null) (String type, String? name) splitParam() {
686-738: Consider simplifying parameter extraction logicThe
extractParamsfunction is quite complex with nested loops and multiple string operations. Consider using a regular expression to simplify the parsing logic.Example approach:
List<(String type, String? name)> extractParams(String text) { // Match either: // 1. Type with optional name: (uint256 amount) // 2. Nested tuples: (uint256,(address,bool)) final regex = RegExp(r'\((.*?)\)|\b(\w+(?:\s+\w+)?)\b'); return regex .allMatches(text) .map((m) => (m.group(1) ?? m.group(2)!).splitParam()) .toList(); }
Line range hint
10-270: Consider implementing toString for better debuggingThe ContractFunction class and its subclasses would benefit from custom toString implementations for better debugging and logging.
@override String toString() { return '$runtimeType(name: $name, parameters: $parameters)'; }
312-321: Add documentation for ContractFunctionWithValuesAndOutputsThis sealed class lacks documentation explaining its purpose and when it should be used versus ContractFunctionWithValues.
+/// Represents a contract function that has both input parameters and output values. +/// +/// This is typically used after a contract function has been executed and its outputs +/// have been decoded. It extends [ContractFunctionWithValues] to add output handling. sealed class ContractFunctionWithValuesAndOutputs extends ContractFunctionWithValues {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/src/crypto/evm/entities/contract/contract_function.dart(6 hunks)lib/src/crypto/evm/evm.dart(1 hunks)lib/src/crypto/evm/repositories/function_selector_repository.dart(6 hunks)
🔇 Additional comments (3)
lib/src/crypto/evm/evm.dart (1)
42-42: Verify the usage and documentation of the newly exposed functionality.
Since this export makes the function selector repository publicly available, we should verify its integration and documentation.
✅ Verification successful
Let me gather more information about the documentation and implementation details of the FunctionSelectorRepository.
Based on the gathered information, I can now provide a final response about the export's impact:
The export of function_selector_repository.dart is properly integrated and used
The repository is:
- Well-documented with clear implementation details
- Used appropriately in
contract_function.dartfor decoding EVM function selectors - Has test coverage in
fetch_function_signature_test.dart - Implements singleton pattern and caching for performance
- Used as an internal service to fetch and decode contract function selectors from external APIs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and documentation of FunctionSelectorRepository
# Check if the repository is properly documented
echo "Checking documentation..."
rg -l "/// " repositories/function_selector_repository.dart
# Check for existing usages in the codebase
echo "Checking usages..."
rg "FunctionSelectorRepository" --type dart
# Look for any test coverage
echo "Checking test coverage..."
fd -e dart -e test "function_selector.*test"
Length of output: 1459
Script:
#!/bin/bash
# Check the implementation and documentation of FunctionSelectorRepository
echo "Checking FunctionSelectorRepository implementation..."
cat lib/src/crypto/evm/repositories/function_selector_repository.dart
# Check the test implementation to understand usage patterns
echo -e "\nChecking test implementation..."
cat test/ci/parsing/fetch_function_signature_test.dart
# Check the contract function implementation to understand integration
echo -e "\nChecking contract function implementation..."
cat lib/src/crypto/evm/entities/contract/contract_function.dart
Length of output: 55744
lib/src/crypto/evm/repositories/function_selector_repository.dart (1)
79-83: Previous review comments are still applicable
The following issues from previous reviews are still present and should be addressed:
- Return type mismatch between
ExternalContractFunction?andContractFunction - Missing error handling in
fromTextSignature
Please refer to the previous review comments for the detailed fixes.
Also applies to: 122-122, 141-143
lib/src/crypto/evm/entities/contract/contract_function.dart (1)
47-54:
Re-enable type validation logic
The commented-out type validation code is crucial for ensuring type safety at runtime. Without it, the function may silently accept invalid parameter types.
This issue was previously identified and the suggested fix is still valid. Please refer to the previous review comment for the implementation details.
| print(functionCache.length); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statements and extract duplicated cache logic
- Debug print statements should not be in production code
- Cache access logic is duplicated across methods
Consider extracting the cache logic into a private method:
+ private ExternalContractFunction? _getCachedFunction(String selector) {
+ if (_functionCache.containsKey(selector)) {
+ return _functionCache[selector];
+ }
+ return null;
+ }Then use it in both methods:
- if (functionCache.containsKey(selector)) {
- print("Cache hit");
- return functionCache[selector];
- }
+ final cachedFunction = _getCachedFunction(selector);
+ if (cachedFunction != null) {
+ return cachedFunction;
+ }Also applies to: 49-51, 99-101
| final Map<String, ExternalContractFunction> functionCache = {}; | ||
|
|
||
| static final FunctionSelectorRepository _instance = | ||
| FunctionSelectorRepository._(); | ||
|
|
||
| FunctionSelectorRepository._(); | ||
|
|
||
| factory FunctionSelectorRepository() { | ||
| return _instance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the cache private
The cache has been changed from a private variable _functionCache to a public functionCache. This exposes the internal cache implementation and allows external modification, which could lead to inconsistencies.
Apply this change to maintain encapsulation:
- final Map<String, ExternalContractFunction> functionCache = {};
+ final Map<String, ExternalContractFunction> _functionCache = {};📝 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.
| final Map<String, ExternalContractFunction> functionCache = {}; | |
| static final FunctionSelectorRepository _instance = | |
| FunctionSelectorRepository._(); | |
| FunctionSelectorRepository._(); | |
| factory FunctionSelectorRepository() { | |
| return _instance; | |
| } | |
| final Map<String, ExternalContractFunction> _functionCache = {}; | |
| static final FunctionSelectorRepository _instance = | |
| FunctionSelectorRepository._(); | |
| FunctionSelectorRepository._(); | |
| factory FunctionSelectorRepository() { | |
| return _instance; | |
| } |
| static Future<ContractFunctionWithValues> decodeRawWithFetch({ | ||
| required Uint8List data, | ||
| ExternalContractFunction? function, | ||
| Map<String, String>? functionMap, | ||
| }) async { | ||
| try { | ||
| assert(data.length >= 4, "Invalid data length"); | ||
| final function_selector = data.sublist(0, 4).toHex; | ||
|
|
||
| if (function != null) { | ||
| return _decodeExternal(data: data, function: function); | ||
| } | ||
| print("a"); | ||
| if (data.length < 4) return UnknownContractFunction(data: data); | ||
|
|
||
| final localResult = decodeRaw(data: data); | ||
| if (functionMap != null) { | ||
| final localResult = decodeRaw(data: data, functionMap: functionMap); | ||
|
|
||
| if (localResult != null) { | ||
| if (localResult is! UnknownContractFunction && | ||
| localResult is! NotDecodableContractFunction) { | ||
| return localResult; | ||
| } | ||
| } | ||
|
|
||
| /// Fetch the function from 4byte.directory | ||
| final externalResult = | ||
| await FunctionSelectorRepository.fetchSelector(function_selector); | ||
| final function_selector = data.sublist(0, 4).toHex; | ||
|
|
||
| if (externalResult != null) { | ||
| return _decodeExternal(data: data, function: externalResult); | ||
| try { | ||
| final externalFunction = await FunctionSelectorRepository().fetchSelector( | ||
| function_selector, | ||
| ); | ||
| if (externalFunction == null) { | ||
| return UnknownContractFunction(data: data); | ||
| } | ||
|
|
||
| return UnknownExternalContractFunction(data); | ||
| return decode(data: data, function: externalFunction); | ||
| } catch (e) { | ||
| return NotDecodableExternalContractFunction(data); | ||
| return UnknownContractFunction(data: data); |
There was a problem hiding this comment.
Improve error handling in decodeRawWithFetch
The function silently catches all exceptions and returns an UnknownContractFunction. This could mask important errors.
} catch (e) {
- return UnknownContractFunction(data: data);
+ Logger.logError(
+ e,
+ hint: "Failed to decode contract function",
+ );
+ return NotDecodableContractFunction(
+ data: data,
+ error: e,
+ );
}📝 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.
| static Future<ContractFunctionWithValues> decodeRawWithFetch({ | |
| required Uint8List data, | |
| ExternalContractFunction? function, | |
| Map<String, String>? functionMap, | |
| }) async { | |
| try { | |
| assert(data.length >= 4, "Invalid data length"); | |
| final function_selector = data.sublist(0, 4).toHex; | |
| if (function != null) { | |
| return _decodeExternal(data: data, function: function); | |
| } | |
| print("a"); | |
| if (data.length < 4) return UnknownContractFunction(data: data); | |
| final localResult = decodeRaw(data: data); | |
| if (functionMap != null) { | |
| final localResult = decodeRaw(data: data, functionMap: functionMap); | |
| if (localResult != null) { | |
| if (localResult is! UnknownContractFunction && | |
| localResult is! NotDecodableContractFunction) { | |
| return localResult; | |
| } | |
| } | |
| /// Fetch the function from 4byte.directory | |
| final externalResult = | |
| await FunctionSelectorRepository.fetchSelector(function_selector); | |
| final function_selector = data.sublist(0, 4).toHex; | |
| if (externalResult != null) { | |
| return _decodeExternal(data: data, function: externalResult); | |
| try { | |
| final externalFunction = await FunctionSelectorRepository().fetchSelector( | |
| function_selector, | |
| ); | |
| if (externalFunction == null) { | |
| return UnknownContractFunction(data: data); | |
| } | |
| return UnknownExternalContractFunction(data); | |
| return decode(data: data, function: externalFunction); | |
| } catch (e) { | |
| return NotDecodableExternalContractFunction(data); | |
| return UnknownContractFunction(data: data); | |
| static Future<ContractFunctionWithValues> decodeRawWithFetch({ | |
| required Uint8List data, | |
| Map<String, String>? functionMap, | |
| }) async { | |
| print("a"); | |
| if (data.length < 4) return UnknownContractFunction(data: data); | |
| if (functionMap != null) { | |
| final localResult = decodeRaw(data: data, functionMap: functionMap); | |
| if (localResult is! UnknownContractFunction && | |
| localResult is! NotDecodableContractFunction) { | |
| return localResult; | |
| } | |
| } | |
| final function_selector = data.sublist(0, 4).toHex; | |
| try { | |
| final externalFunction = await FunctionSelectorRepository().fetchSelector( | |
| function_selector, | |
| ); | |
| if (externalFunction == null) { | |
| return UnknownContractFunction(data: data); | |
| } | |
| return decode(data: data, function: externalFunction); | |
| } catch (e) { | |
| Logger.logError( | |
| e, | |
| hint: "Failed to decode contract function", | |
| ); | |
| return NotDecodableContractFunction( | |
| data: data, | |
| error: e, | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
lib/src/crypto/evm/repositories/function_selector_repository.dart (2)
47-48: Use Uri.https for safer URL constructionString concatenation for URLs can be error-prone. Using Uri.https ensures proper encoding of query parameters.
- final endpoint = "$_4byteEndpoint/signatures/?hex_signature=$selector"; - final uri = Uri.parse(endpoint); + final uri = Uri.https( + '4byte.directory', + '/api/v1/signatures/', + {'hex_signature': selector}, + );
74-76: Use Uri.https for safer URL constructionSimilar to the 4byte endpoint, use Uri.https for safer URL construction.
- final endpoint = "$_openchainEndpoint/lookup?function=$selector&filter=true"; - final uri = Uri.parse(endpoint); + final uri = Uri.https( + 'api.openchain.xyz', + '/signature-database/v1/lookup', + {'function': selector, 'filter': 'true'}, + );lib/src/crypto/evm/entities/contract/contract_function.dart (2)
Line range hint
567-617: Add error details to UnknownContractFunctionThe
UnknownContractFunctionclass could provide more context about why the function couldn't be identified.Consider adding an error field:
class UnknownContractFunction extends ContractFunctionWithValues implements ErrorContractFunctionMixin { final Uint8List data; final int timeStamp; + final String? errorReason; UnknownContractFunction({ required this.data, int? timeStamp, + this.errorReason, }) : timeStamp = timeStamp ?? DateTime.now().millisecondsSinceEpoch, super(name: "Unknown"); @override Json toJson() { return { "name": name, "data": data.toHex, "timeStamp": timeStamp, + if (errorReason != null) "error": errorReason, }; }
10-24: Consider adding documentation for the sealed class hierarchyThe
ContractFunctionclass serves as the base for a complex hierarchy but lacks documentation explaining the relationships and use cases.Add class-level documentation:
/// Base class for all contract function representations. /// /// This sealed class hierarchy provides different implementations for: /// - [LocalContractFunction]: Functions from local ABI with complete information /// - [ExternalContractFunction]: Functions from external sources (e.g., 4byte.directory) /// - [UnknownContractFunction]: Functions that couldn't be identified /// - [NotDecodableContractFunction]: Functions that couldn't be decoded sealed class ContractFunction implements ExternalContractFunctionMixin {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/src/crypto/evm/entities/contract/contract_function.dart(6 hunks)lib/src/crypto/evm/repositories/function_selector_repository.dart(5 hunks)test/ci/parsing/fetch_function_signature_test.dart(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ci/parsing/fetch_function_signature_test.dart
🔇 Additional comments (2)
lib/src/crypto/evm/repositories/function_selector_repository.dart (2)
22-40: LGTM! Well-structured selector fetching logic
The implementation provides good flexibility with source selection and proper null handling. The assertion helps prevent misconfiguration.
122-122: Skip comment: Return type mismatch issue
This issue was already identified in a previous review comment.
| final Map<String, Completer<ExternalContractFunction?>> functionCache = {}; | ||
|
|
||
| static final FunctionSelectorRepository _instance = | ||
| FunctionSelectorRepository._(); | ||
|
|
||
| FunctionSelectorRepository._(); | ||
|
|
||
| factory FunctionSelectorRepository() { | ||
| return _instance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the cache private to maintain encapsulation
The cache should be private to prevent external modification that could lead to inconsistencies. The singleton pattern is well implemented, but exposing the cache breaks encapsulation.
Apply this change:
- final Map<String, Completer<ExternalContractFunction?>> functionCache = {};
+ final Map<String, Completer<ExternalContractFunction?>> _functionCache = {};📝 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.
| final Map<String, Completer<ExternalContractFunction?>> functionCache = {}; | |
| static final FunctionSelectorRepository _instance = | |
| FunctionSelectorRepository._(); | |
| FunctionSelectorRepository._(); | |
| factory FunctionSelectorRepository() { | |
| return _instance; | |
| } | |
| final Map<String, Completer<ExternalContractFunction?>> _functionCache = {}; | |
| static final FunctionSelectorRepository _instance = | |
| FunctionSelectorRepository._(); | |
| FunctionSelectorRepository._(); | |
| factory FunctionSelectorRepository() { | |
| return _instance; | |
| } |
| try { | ||
| final externalFunction = await FunctionSelectorRepository().fetchSelector( | ||
| function_selector, | ||
| openChain: openChain, | ||
| fourByte: fourByte, | ||
| ); | ||
| if (externalFunction == null) { | ||
| return UnknownContractFunction(data: data); | ||
| } | ||
|
|
||
| return UnknownExternalContractFunction(data); | ||
| return decode(data: data, function: externalFunction); | ||
| } catch (e) { | ||
| return NotDecodableExternalContractFunction(data); | ||
| return UnknownContractFunction(data: data); |
There was a problem hiding this comment.
Improve error handling in decodeRawWithFetch
The function silently catches all exceptions and returns UnknownContractFunction. This masks potentially important errors that could help diagnose issues.
Apply this diff to improve error handling:
try {
final externalFunction = await FunctionSelectorRepository().fetchSelector(
function_selector,
openChain: openChain,
fourByte: fourByte,
);
if (externalFunction == null) {
- return UnknownContractFunction(data: data);
+ return UnknownContractFunction(
+ data: data,
+ error: "Function selector not found in external sources",
+ );
}
return decode(data: data, function: externalFunction);
} catch (e) {
- return UnknownContractFunction(data: data);
+ return NotDecodableContractFunction(
+ data: data,
+ error: e,
+ );
}Committable suggestion skipped: line range outside the PR's diff.
| // if (param.type.internalType != value.runtimeType) { | ||
| // if (param.type.internalType == BigInt && | ||
| // value.runtimeType.toString() == "_BigIntImpl") { | ||
| // } else | ||
| // throw Exception( | ||
| // "Invalid type for param: ${param.name}. Expected: ${param.type.internalType} Got: ${value.runtimeType}", | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
Re-enable and improve type validation
The commented-out type validation code is crucial for ensuring type safety. Without it, invalid parameter types could lead to runtime errors.
Apply this diff to implement robust type validation:
- // if (param.type.internalType != value.runtimeType) {
- // if (param.type.internalType == BigInt &&
- // value.runtimeType.toString() == "_BigIntImpl") {
- // } else
- // throw Exception(
- // "Invalid type for param: ${param.name}. Expected: ${param.type.internalType} Got: ${value.runtimeType}",
- // );
- // }
+ if (!_isValidType(param.type.internalType, value)) {
+ throw ArgumentError(
+ 'Invalid type for param "${param.name}". '
+ 'Expected: ${param.type.internalType}, '
+ 'Got: ${value.runtimeType}',
+ );
+ }
// Add this helper method
bool _isValidType(Type expectedType, dynamic value) {
if (expectedType == BigInt && value.runtimeType.toString() == "_BigIntImpl") {
return true;
}
return value.runtimeType == expectedType;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/src/crypto/evm/entities/contract/contract_function.dart (1)
Line range hint
566-674: Reduce code duplication in error handling classesUnknownContractFunction and NotDecodableContractFunction share significant code. Consider extracting common functionality to a base class.
+abstract class BaseErrorContractFunction extends ContractFunctionWithValues + implements ErrorContractFunctionMixin { + final Uint8List data; + final int timeStamp; + + @override + StateMutability? get stateMutability => null; + + @override + List<FunctionParam>? get outputTypes => null; + + @override + List<FunctionParamWithValue> get parameters { + return [ + FunctionParamWithValue.fromParam<Uint8List>( + FunctionParam(name: "data", type: FunctionParamBytes()), + data, + ), + ]; + } + + BaseErrorContractFunction({ + required this.data, + required String name, + int? timeStamp, + }) : timeStamp = timeStamp ?? DateTime.now().millisecondsSinceEpoch, + super(name: name); + + String get UTF8 => utf8.decode(data); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
lib/src/crypto/evm/entities/contract/contract_function.dart(6 hunks)pubspec.yaml(1 hunks)test/ci/parsing/fetch_function_signature_test.dart(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pubspec.yaml
- test/ci/parsing/fetch_function_signature_test.dart
🔇 Additional comments (3)
lib/src/crypto/evm/entities/contract/contract_function.dart (3)
174-186:
Improve error handling and logging in decodeRawWithFetch
The function silently catches all exceptions and returns UnknownContractFunction without logging. This makes debugging difficult.
try {
final externalFunction = await FunctionSelectorRepository().fetchSelector(
function_selector,
openChain: openChain,
fourByte: fourByte,
);
if (externalFunction == null) {
- return UnknownContractFunction(data: data);
+ return UnknownContractFunction(
+ data: data,
+ error: "Function selector not found in external sources",
+ );
}
return decode(data: data, function: externalFunction);
} catch (e) {
+ Logger.logError(
+ e,
+ hint: "Failed to decode contract function with selector: $function_selector",
+ );
- return UnknownContractFunction(data: data);
+ return NotDecodableContractFunction(
+ data: data,
+ error: e,
+ );
}Likely invalid or redundant comment.
46-53:
Re-enable type validation with improved implementation
The commented-out type validation is crucial for ensuring type safety at runtime. Consider implementing a more robust type validation system.
- // if (param.type.internalType != value.runtimeType) {
- // if (param.type.internalType == BigInt &&
- // value.runtimeType.toString() == "_BigIntImpl") {
- // } else
- // throw Exception(
- // "Invalid type for param: ${param.name}. Expected: ${param.type.internalType} Got: ${value.runtimeType}",
- // );
- // }
+ if (!_isValidParamType(param.type.internalType, value)) {
+ throw ArgumentError(
+ 'Invalid type for parameter "${param.name}". '
+ 'Expected: ${param.type.internalType}, '
+ 'Got: ${value.runtimeType}',
+ );
+ }
+bool _isValidParamType(Type expectedType, dynamic value) {
+ if (expectedType == BigInt && value.runtimeType.toString() == "_BigIntImpl") {
+ return true;
+ }
+ return value.runtimeType == expectedType;
+}Likely invalid or redundant comment.
9-9: Add comprehensive documentation for ContractFunction hierarchy
The sealed class hierarchy is complex and would benefit from detailed documentation explaining the relationships between classes and their intended usage.
| List<(String type, String? name)> extractParams(String text) { | ||
| text = text.trim(); | ||
|
|
||
| var opening = text.indexOf("("); | ||
|
|
||
| final values = <(String type, String? name)>[]; | ||
| final start = opening == -1 | ||
| ? text | ||
| : opening == 0 | ||
| ? "" | ||
| : text.substring(0, opening - 1); | ||
|
|
||
| if (start.isNotEmpty) { | ||
| if (start.startsWith('(')) { | ||
| values.add(start.splitParam()); | ||
| } else if (start.contains(',')) { | ||
| values.addAll( | ||
| start.split(',').map((s) => s.splitParam()), | ||
| ); | ||
| } else { | ||
| values.add(start.splitParam()); | ||
| } | ||
| } | ||
| if (opening != -1) { | ||
| var closing = -1; | ||
| var nested = 0; | ||
| for (var i = opening; i < text.length; i++) { | ||
| final char = text[i]; | ||
| if (nested == 0 && char == ',') { | ||
| closing = i; | ||
| break; | ||
| } | ||
|
|
||
| if (char == "(") nested++; | ||
| if (char == ")") nested--; | ||
| } | ||
|
|
||
| if (closing == -1) closing = text.length; | ||
|
|
||
| var tuple = text.substring(opening, closing).splitParam(); | ||
| values.add(tuple); | ||
|
|
||
| if (closing < text.length) { | ||
| values.addAll( | ||
| extractParams( | ||
| text.substring(closing + 1), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return values; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance parameter extraction robustness
The parameter extraction logic needs better validation for malformed input and nested parentheses handling.
List<(String type, String? name)> extractParams(String text) {
+ if (text.isEmpty) {
+ return [];
+ }
text = text.trim();
var opening = text.indexOf("(");
+ if (opening == -1 && text.contains(")")) {
+ throw FormatException("Unmatched closing parenthesis in: $text");
+ }
// ... existing code ...
if (char == "(") nested++;
if (char == ")") nested--;
+ if (nested < 0) {
+ throw FormatException("Unmatched parentheses in: $text");
+ }
// ... rest of the code ...
}📝 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.
| List<(String type, String? name)> extractParams(String text) { | |
| text = text.trim(); | |
| var opening = text.indexOf("("); | |
| final values = <(String type, String? name)>[]; | |
| final start = opening == -1 | |
| ? text | |
| : opening == 0 | |
| ? "" | |
| : text.substring(0, opening - 1); | |
| if (start.isNotEmpty) { | |
| if (start.startsWith('(')) { | |
| values.add(start.splitParam()); | |
| } else if (start.contains(',')) { | |
| values.addAll( | |
| start.split(',').map((s) => s.splitParam()), | |
| ); | |
| } else { | |
| values.add(start.splitParam()); | |
| } | |
| } | |
| if (opening != -1) { | |
| var closing = -1; | |
| var nested = 0; | |
| for (var i = opening; i < text.length; i++) { | |
| final char = text[i]; | |
| if (nested == 0 && char == ',') { | |
| closing = i; | |
| break; | |
| } | |
| if (char == "(") nested++; | |
| if (char == ")") nested--; | |
| } | |
| if (closing == -1) closing = text.length; | |
| var tuple = text.substring(opening, closing).splitParam(); | |
| values.add(tuple); | |
| if (closing < text.length) { | |
| values.addAll( | |
| extractParams( | |
| text.substring(closing + 1), | |
| ), | |
| ); | |
| } | |
| } | |
| return values; | |
| } | |
| List<(String type, String? name)> extractParams(String text) { | |
| if (text.isEmpty) { | |
| return []; | |
| } | |
| text = text.trim(); | |
| var opening = text.indexOf("("); | |
| if (opening == -1 && text.contains(")")) { | |
| throw FormatException("Unmatched closing parenthesis in: $text"); | |
| } | |
| final values = <(String type, String? name)>[]; | |
| final start = opening == -1 | |
| ? text | |
| : opening == 0 | |
| ? "" | |
| : text.substring(0, opening - 1); | |
| if (start.isNotEmpty) { | |
| if (start.startsWith('(')) { | |
| values.add(start.splitParam()); | |
| } else if (start.contains(',')) { | |
| values.addAll( | |
| start.split(',').map((s) => s.splitParam()), | |
| ); | |
| } else { | |
| values.add(start.splitParam()); | |
| } | |
| } | |
| if (opening != -1) { | |
| var closing = -1; | |
| var nested = 0; | |
| for (var i = opening; i < text.length; i++) { | |
| final char = text[i]; | |
| if (nested == 0 && char == ',') { | |
| closing = i; | |
| break; | |
| } | |
| if (char == "(") nested++; | |
| if (char == ")") nested--; | |
| if (nested < 0) { | |
| throw FormatException("Unmatched parentheses in: $text"); | |
| } | |
| } | |
| if (closing == -1) closing = text.length; | |
| var tuple = text.substring(opening, closing).splitParam(); | |
| values.add(tuple); | |
| if (closing < text.length) { | |
| values.addAll( | |
| extractParams( | |
| text.substring(closing + 1), | |
| ), | |
| ); | |
| } | |
| } | |
| return values; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores