Wallet interface #134
Wallet interface #134
Conversation
WalkthroughRemoved obsolete example entry; added an in-repo BIP39/HDNode implementation and CLI framework; migrated bip32/bip39 usages to HDNode; added crypto hash/HMAC utilities; refactored UTXO script/tx structures, Dogecoin support, tests, CI, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as WalletCLI
participant Reg as CommandRegistry
participant Cmd as Command
User->>CLI: start (main)
CLI->>Reg: register Help/Exit commands
CLI->>User: show welcome & prompt
User->>CLI: input "help"
CLI->>Reg: lookup("help")
Reg-->>CLI: HelpCommand
CLI->>Cmd: execute(args)
Cmd-->>CLI: CommandResult
CLI->>User: print result and show prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
lib/src/crypto/utxo/utils/pubkey_to_address.dart (1)
Line range hint
3-4: Remove unused imports.The following imports are no longer used after moving the hash functions to
crypto.dart:
package:pointycastle/digests/ripemd160.dartpackage:pointycastle/digests/sha256.dart-import 'package:pointycastle/digests/ripemd160.dart'; -import 'package:pointycastle/digests/sha256.dart';Also applies to: 9-9
🧹 Nitpick comments (7)
lib/src/wallet/hd_node.dart (2)
245-251: Clarify seed length constraints infromSeedThe current checks enforce that the seed length is between 16 and 64 bytes (128 to 512 bits). While this aligns with BIP32 recommendations, providing more descriptive error messages can improve usability.
Apply this diff to enhance error messages:
- throw new ArgumentError("Seed should be at least 128 bits"); + throw new ArgumentError("Seed length is too short; it should be at least 16 bytes (128 bits)"); ... - throw new ArgumentError("Seed should be at most 512 bits"); + throw new ArgumentError("Seed length is too long; it should be at most 64 bytes (512 bits)");
221-223: Validate public key before creating HDNodeIn the
_fromPublicKeyfactory method, you check if the public key is on the curve usingecurve.isPoint(publicKey). However, the error message"Point is not on the curve"may be misleading if the public key is simply invalid. Consider refining the error handling to provide more accurate feedback.Apply this diff to improve error handling:
- if (ecurve.isPoint(publicKey) == false) { - throw ArgumentError("Point is not on the curve"); - } + if (!ecurve.isPoint(publicKey)) { + throw ArgumentError("Invalid public key: not a valid point on the elliptic curve"); + }lib/src/utils/crypto.dart (3)
7-11: Optimize hash function compositionIn the
ripmed160Sha256Hashfunction, you compose the RIPEMD160 hash over the SHA256 hash of the input. While this is standard, consider caching the intermediate SHA256 result if this function is called repeatedly with the same input to improve performance.Alternatively, if caching is not suitable due to input variability, document that this function intentionally does not cache results for correctness.
29-35: Correct the comment forhmacSHA512The comment above the
hmacSHA512function says/// HMAC Sha256, but the function actually implements HMAC with SHA512.Apply this diff to correct the comment:
- /// HMAC Sha256 + /// HMAC SHA512
33-35: Ensure key length compliance in HMACWhen initializing HMAC with SHA512, ensure that the key length is appropriate. While HMAC can handle keys of any length, using keys longer than the block size (128 bytes for SHA512) can lead to unnecessary overhead. Consider adding a check or note in the documentation.
test/ci/derivation/derivation_path_test.dart (1)
5-5: Remove unnecessary commented codeThe commented-out imports at the top of the file are no longer needed. Removing them can improve code cleanliness and maintainability.
Apply this diff to remove unused code:
-// import 'package:test/test.dart'; -// import 'package:walletkit_dart/src/crypto/derivation.dart'; -// import 'package:walletkit_dart/src/domain/entities/default_assets.dart';lib/src/crypto/utxo/utils/pubkey_to_address.dart (1)
Line range hint
134-157: Add documentation for BCH address constants.The
_polymodimplementation uses magic numbers in theGENERATORarray. Consider adding documentation explaining these constants and linking to the BCH address specification for better maintainability.List<BigInt> GENERATOR = [ + // Constants from the BCH address specification + // https://github.com/bitcoincashorg/bitcoincash.org/blob/master/spec/cashaddr.md BigInt.from(0x98f2bc8e61), BigInt.from(0x79b76d99e2), BigInt.from(0xf33e5fb3c4), BigInt.from(0xae2eabe2a8), BigInt.from(0x1e4f43e470), ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
example/web3_kit_example.dart(0 hunks)example/wkdart.dart(1 hunks)lib/src/crypto/tron/entities/tron_address.dart(1 hunks)lib/src/crypto/tron/entities/tron_transaction_utils.dart(1 hunks)lib/src/crypto/utxo/entities/payments/p2h.dart(1 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(1 hunks)lib/src/crypto/utxo/utils/eurocoin_signing.dart(1 hunks)lib/src/crypto/utxo/utils/proof_of_payment.dart(1 hunks)lib/src/crypto/utxo/utils/pubkey_to_address.dart(1 hunks)lib/src/utils/crypto.dart(1 hunks)lib/src/wallet/hd_node.dart(1 hunks)lib/src/wallet/wallet.dart(1 hunks)test/ci/derivation/derivation_path_test.dart(2 hunks)test/ci/fetching/assets/bch_fetch_test.dart(1 hunks)test/ci/tron/send_simulation_test.dart(1 hunks)
💤 Files with no reviewable changes (1)
- example/web3_kit_example.dart
✅ Files skipped from review due to trivial changes (9)
- lib/src/wallet/wallet.dart
- lib/src/crypto/utxo/utils/eurocoin_signing.dart
- test/ci/tron/send_simulation_test.dart
- test/ci/fetching/assets/bch_fetch_test.dart
- lib/src/crypto/utxo/utils/proof_of_payment.dart
- lib/src/crypto/tron/entities/tron_transaction_utils.dart
- lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart
- lib/src/crypto/utxo/entities/payments/p2h.dart
- lib/src/crypto/tron/entities/tron_address.dart
🧰 Additional context used
🪛 GitHub Actions: TestSuite
lib/src/crypto/utxo/utils/pubkey_to_address.dart
[warning] 3-3: Unused import: 'package:pointycastle/digests/ripemd160.dart'. Try removing the import directive.
[warning] 4-4: Unused import: 'package:pointycastle/digests/sha256.dart'. Try removing the import directive.
🔇 Additional comments (3)
example/wkdart.dart (1)
1-13: Verify wallet interface implementation and dependencies.Let's ensure that the necessary wallet interface implementation and dependencies are in place.
✅ Verification successful
Wallet implementation and dependencies are properly integrated.
The codebase demonstrates a comprehensive wallet implementation with:
- Full HD wallet support with multiple derivation paths (BIP44/49/84)
- Multi-chain compatibility (Bitcoin, Ethereum, Tron, Litecoin)
- Complete transaction lifecycle management
- All required dependencies properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for wallet interface implementation echo "Checking for wallet interface implementation..." rg -l "class.*Wallet.*{" --type dart # Check pubspec.yaml for required dependencies echo "Checking dependencies..." if [ -f "pubspec.yaml" ]; then cat pubspec.yaml fiLength of output: 1077
Script:
#!/bin/bash # Search for wallet-related classes and interfaces echo "Searching for wallet-related implementations..." rg "class.*Wallet|interface.*Wallet|abstract.*Wallet" --type dart # Search for common wallet method signatures echo -e "\nSearching for wallet-related methods..." rg "(sign|transfer|balance|address|transaction).*\(" --type dartLength of output: 51472
lib/src/crypto/utxo/utils/pubkey_to_address.dart (2)
Line range hint
43-63: Well-structured address type handling.The address generation logic is well-organized with clear separation between different address types (legacy, segwit, compatibility, cashaddr). The exhaustive pattern matching ensures proper handling of all address types.
Line range hint
22-42: Verify cryptographic implementation in the new location.The hash functions (
ripmed160Sha256Hash,sha256Sha256Hash) have been moved tocrypto.dart. While the function calls remain unchanged, it's crucial to verify that the new implementation maintains the same security properties.Let's verify the implementation of these critical cryptographic functions:
✅ Verification successful
Cryptographic implementation verified successfully
The moved hash functions maintain the same security properties and follow cryptographic best practices:
ripmed160Sha256Hash: Correctly implements RIPEMD160(SHA256(input))sha256Sha256Hash: Correctly implements SHA256(SHA256(input))The functions are used consistently across the codebase for standard cryptocurrency operations including address generation, transaction signing, and HD wallet operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of moved hash functions # Check the implementation of the moved hash functions rg -A 10 "ripmed160Sha256Hash|sha256Sha256Hash" "src/utils/crypto.dart" # Verify if any tests were moved along with the implementation fd -e dart -e test "crypto_test" -x cat {}Length of output: 178
Script:
#!/bin/bash # First, let's find crypto-related files fd -t f crypto # Then search for the specific hash functions in all files rg -A 10 "ripmed160Sha256Hash|sha256Sha256Hash" # Look for any crypto-related test files fd -t f -e dart "crypto.*test"Length of output: 14816
example/wkdart.dart
Outdated
| void main(List<String> args) { | ||
| print("Welcome to Walletkit-Dart"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance main function with wallet initialization and CLI argument handling.
The current implementation lacks essential wallet functionality initialization and doesn't utilize the command-line arguments.
Consider implementing:
- Command-line argument parsing for wallet operations
- Wallet initialization
- Usage instructions
- Error handling
Example structure:
void main(List<String> args) async {
try {
if (args.contains('--help')) {
printUsage();
return;
}
print("Welcome to Walletkit-Dart");
final wallet = await WalletKit.initialize();
// Interactive loop here
} catch (e) {
print("Error: $e");
exit(1);
}
}
example/wkdart.dart
Outdated
| while (true) { | ||
| final input = stdin.readLineSync(); | ||
|
|
||
| if (input == 'exit') { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper command processing and async I/O.
The current interactive loop implementation has several limitations:
- Uses blocking I/O
- Lacks command processing
- No error handling
- No user feedback
Consider implementing:
while (true) {
stdout.write('wallet> ');
String? input;
try {
input = await stdin.readLineSync();
if (input == null) continue;
if (input == 'exit') {
break;
}
final result = await processCommand(input);
print(result);
} catch (e) {
print('Error: $e');
}
}
Future<String> processCommand(String input) async {
final parts = input.split(' ');
switch (parts[0]) {
case 'balance':
return await wallet.getBalance();
case 'send':
if (parts.length != 3) return 'Usage: send <address> <amount>';
return await wallet.send(parts[1], double.parse(parts[2]));
case 'help':
return '''
Available commands:
balance - Show wallet balance
send <address> <amount> - Send funds
exit - Exit the application
''';
default:
return 'Unknown command. Type "help" for available commands.';
}
}| import 'dart:io'; | ||
|
|
There was a problem hiding this comment.
Add necessary wallet-related imports and error handling.
As this is a wallet interface implementation, the file should import the necessary wallet-related modules and implement proper error handling for I/O operations.
Consider adding:
import 'dart:io';
+import 'package:walletkit_dart/walletkit_dart.dart';
+import 'dart:async';📝 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.
| import 'dart:io'; | |
| import 'dart:io'; | |
| import 'package:walletkit_dart/walletkit_dart.dart'; | |
| import 'dart:async'; | |
| final IR = I.sublist(32); | ||
| if (ecurve.isPrivate(IL) == false) { | ||
| return derive(index + 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential infinite recursion in derive method
In the derive method, recursive calls are made via return derive(index + 1); when invalid keys are encountered. Without a termination condition, this could lead to infinite recursion and a stack overflow if consecutive invalid keys occur. Consider implementing a maximum retry limit or handling such cases iteratively to prevent potential runtime errors.
lib/src/wallet/hd_node.dart
Outdated
| String? extendedPrivateKey(int version) { | ||
| if (version.isUint(32) == false) { | ||
| throw ArgumentError("Expected UInt32 for version"); | ||
| } | ||
|
|
||
| if (isNeutered) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure extendedPrivateKey returns a non-null value
The extendedPrivateKey method returns null if the node is neutered (i.e., lacks a private key). This might lead to unexpected null references when callers expect a string. Consider throwing an exception or returning a meaningful default value to prevent potential Null errors downstream.
| test( | ||
| "HD Wallet Derivation", | ||
| () { | ||
| const mnemonic = | ||
| "walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart"; | ||
|
|
||
| final seed = mnemonicToSeed(mnemonic); | ||
|
|
||
| // final masterNode = deriveMasterNodeFromSeed(seed: seed, walletPath: ); | ||
|
|
||
| print(seed); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to the "HD Wallet Derivation" test
The test case currently prints the seed but lacks assertions to verify the correctness of the HD wallet derivation process. To make this test meaningful, add assertions that compare the derived keys or addresses against expected values.
Here's an example of how you might enhance the test:
test(
"HD Wallet Derivation",
() {
const mnemonic =
"walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart";
final seed = mnemonicToSeed(mnemonic);
final masterNode = HDNode.fromSeed(seed);
final derivedNode = masterNode.derivePath("m/44'/0'/0'/0/0");
final address = derivedNode.publicKey; // Or use an address generating function
// Expected public key or address (to be replaced with actual expected value)
final expectedPublicKey = ...;
expect(derivedNode.publicKey, expectedPublicKey);
},
);
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
lib/src/crypto/utxo/utils/derivation.dart (1)
Line range hint
122-138: Ensure non-null values for critical properties in extensionIn the extension on
HDNode, properties likenetwork,privateKey, andpublicKeyare force unwrapped with!. This can lead to runtime exceptions if any of these properties arenull. Add necessary null checks or handle cases where these properties might benullto prevent potential crashes.Apply this diff to add null safety checks:
String toBase58wkCompatibility(int parentFingerprint, int depth) { - final version = - (!isNeutered) ? network!.bip32.private : network!.bip32.public; + if (network == null) { + throw StateError('Network cannot be null'); + } + final version = + (!isNeutered) ? network!.bip32.private : network!.bip32.public; Uint8List buffer = Uint8List(78); ByteData bytes = buffer.buffer.asByteData(); bytes.setUint32(0, version); // ... - if (!isNeutered) { + if (!isNeutered && privateKey != null) { bytes.setUint8(45, 0); buffer.setRange(46, 78, privateKey!); } else if (publicKey != null) { buffer.setRange(45, 78, publicKey); + } else { + throw StateError('Either privateKey or publicKey must be non-null'); }lib/src/crypto/utxo/utxo_analyzer.dart (2)
Line range hint
322-327: Ensure null safety forpurposeparameterThe
purposeparameter insearchTransactionsForWalletTypeis nullable (HDWalletPurpose?). Ensure that any usage ofpurposewithin the function handles the null case to prevent potential runtime null reference errors.Consider applying this diff to handle null values:
required HDNode masterNode, -required HDWalletPurpose? purpose, +required HDWalletPurpose purpose,Or add null checks within the function:
if (purpose != null) { // Use purpose as needed } else { // Handle the null case appropriately }
Line range hint
361-369: ValidatewalletPurposeinsearchForTransactionsThe
walletPurposeparameter is nullable. Ensure that thederiveChildNodefunction can handle a nullwalletPurposeor include necessary null checks to prevent unexpected behavior.Apply this diff to enforce non-nullability or handle null cases:
required int chainIndex, -required HDWalletPurpose? walletPurpose, +required HDWalletPurpose walletPurpose,Or add null checks:
if (walletPurpose != null) { // Proceed with walletPurpose } else { // Handle null walletPurpose appropriately }
♻️ Duplicate comments (2)
test/ci/derivation/derivation_path_test.dart (1)
56-68:⚠️ Potential issueComplete the "HD Wallet Derivation" test implementation
The test currently only generates a seed from a mnemonic but lacks assertions to verify the HD wallet derivation process.
Add assertions to verify the derived keys match expected values. For example:
test( "HD Wallet Derivation", () { const mnemonic = "walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart"; final seed = mnemonicToSeed(mnemonic); + final masterNode = HDNode.fromSeed(seed); + final derivedNode = masterNode.derivePath("m/44'/0'/0'/0/0"); + + // Add assertions to verify the derived node + expect(derivedNode.publicKey, equals(expectedPublicKey)); + expect(derivedNode.extendedPublicKey(), equals(expectedXpub)); }, );lib/src/wallet/hd_node.dart (1)
165-168:⚠️ Potential issueFix potential infinite recursion in derive method
The recursive call to
derive(index + 1)when encountering invalid keys could lead to infinite recursion and stack overflow.Add a maximum retry limit:
+ static const int MAX_DERIVE_RETRIES = 100; + HDNode derive(int index) { + return _deriveWithRetry(index, 0); + } + + HDNode _deriveWithRetry(int index, int retryCount) { + if (retryCount >= MAX_DERIVE_RETRIES) { + throw ArgumentError("Exceeded maximum retries for key derivation"); + } // ... existing code ... - return derive(index + 1); + return _deriveWithRetry(index + 1, retryCount + 1); }
🧹 Nitpick comments (9)
lib/src/crypto/utxo/utils/derivation.dart (1)
144-145: Specify the network when deriving a nodeIn the
deriveNodefunction,HDNode.fromSeed(seed)is called without specifying thenetwork. This may lead to unexpected behavior if the default network is not the intended one. It's advisable to explicitly specify the network for clarity and correctness.Apply this diff to specify the network:
HDNode deriveNode(Uint8List seed, String path) { - final node = HDNode.fromSeed(seed); + final node = HDNode.fromSeed(seed, network: desiredNetworkType.networkBIP); return node.derivePath(path); }Replace
desiredNetworkTypewith the appropriateUTXONetworkTypevariable.lib/src/crypto/network_type.dart (1)
101-112: Optimize prefix lookup infindPrefixesFromVersionThe
findPrefixesFromVersionmethod iterates over a list ofNetworkBIPinstances to find matching prefixes. If performance becomes an issue, especially with a larger number of networks, consider using aMap<int, BIP32Prefixes>for constant-time lookups.test/ci/sending/signature_test.dart (1)
63-63: VerifyparentFingerprintusageCurrently,
parentFingerprintis set to0, which is acceptable for root nodes. Confirm that this is intentional and aligns with the hierarchical structure used in your HD wallet implementation.test/ci/bip32/bip32-dart_test.dart (3)
1-12: Add more comprehensive documentation for test setup.While the file origin is noted, additional documentation would be helpful to understand:
- The purpose and structure of the test fixtures
- The relationship between this implementation and the original bip32-dart
- The test coverage goals
Add a detailed comment block explaining:
/// Originally from: https://github.com/dart-bitcoin/bip32-dart/blob/master/test/bip32_test.dart +/// This test suite validates the HDNode implementation against the BIP32 test vectors. +/// The test fixtures contain valid and invalid test cases for: +/// - HD wallet creation from seeds +/// - Private/public key derivation +/// - Child key derivation (normal and hardened) +/// - Edge cases like leading zeros in private keys
15-179: Consider reorganizing test cases for better readability.The test structure is comprehensive but could be more organized. Consider grouping related test cases into separate describe blocks for:
- Key Generation
- Derivation
- Error Handling
Example structure:
void main() { group('Key Generation', () { // Tests for fromSeed, fromExtendedKey etc. }); group('Key Derivation', () { // Tests for derive, deriveHardened etc. }); group('Error Handling', () { // Tests for invalid inputs }); }
229-273: Improve type safety in verify function.The verify function parameters could benefit from stronger typing:
prvparameter should be boolfparameter should have a defined type for the fixture data-void verify(HDNode hd, prv, f, NetworkBIP network) { +void verify(HDNode hd, bool prv, Map<String, dynamic> f, NetworkBIP network) {test/ci/derivation/derive_addresses_test.dart (1)
21-23: Consider adding edge cases to address derivation tests.While the migration to HDNode is correct, the test coverage could be enhanced by adding test cases for:
- Invalid extended keys
- Edge cases in derivation paths
- Network-specific derivation rules
Also applies to: 57-59, 102-104, 472-474
lib/src/crypto/utxo/utils/send.dart (1)
284-284: Enhance error handling for neutered node check.The condition could be more explicit about the failure case.
-if (bip32Node == null || bip32Node.isNeutered) { +if (bip32Node == null) { + throw SendFailure("Missing node for output: $output"); +} +if (bip32Node.isNeutered) {test/ci/derivation/derivation_path_test.dart (1)
Line range hint
12-31: Consider uncommenting and updating the derivation path testsThe commented-out tests for various coin derivation paths would provide valuable test coverage for the wallet's derivation functionality.
Consider updating and re-enabling these tests to ensure proper derivation path handling for different coins (BTC, LTC, BCH).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
lib/src/crypto/network_type.dart(2 hunks)lib/src/crypto/tron/entities/tron_address.dart(1 hunks)lib/src/crypto/utxo/utils/derivation.dart(6 hunks)lib/src/crypto/utxo/utils/eurocoin_signing.dart(3 hunks)lib/src/crypto/utxo/utils/send.dart(5 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(5 hunks)lib/src/crypto/wallet_utils.dart(2 hunks)lib/src/domain/constants.dart(2 hunks)lib/src/domain/entities/node.dart(2 hunks)lib/src/utils/base58.dart(1 hunks)lib/src/wallet/hd_node.dart(1 hunks)pubspec.yaml(0 hunks)test/ci/bip32/bip32-dart_test.dart(1 hunks)test/ci/bip32/fixtures.json(1 hunks)test/ci/derivation/derivation_path_test.dart(2 hunks)test/ci/derivation/derive_addresses_test.dart(4 hunks)test/ci/derivation/derive_extended_pubkey_test.dart(1 hunks)test/ci/sending/signature_test.dart(2 hunks)
💤 Files with no reviewable changes (1)
- pubspec.yaml
🧰 Additional context used
🪛 Gitleaks (8.21.2)
test/ci/bip32/fixtures.json
8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
79-79: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
80-80: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
96-96: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
97-97: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
109-109: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
110-110: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
124-124: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
125-125: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
138-138: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
153-153: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
154-154: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
167-167: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
168-168: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
185-185: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
186-186: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
198-198: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
199-199: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
228-228: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
229-229: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
245-245: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
246-246: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
259-259: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
260-260: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (20)
lib/src/crypto/utxo/utils/derivation.dart (2)
10-10:⚠️ Potential issueConsider the impact of making
typea required parameterThe
typeparameter in thederiveExtendedPubKeyfunction has been changed from optional to required. This change may cause breaking issues in parts of the codebase wherederiveExtendedPubKeyis called without specifying thetype. Ensure that all calls to this function provide the necessarytypeargument.
43-43:⚠️ Potential issueVerify that all calls to
deriveMasterNodeFromSeedincludenetworkTypeThe
networkTypeparameter inderiveMasterNodeFromSeedis now required. This could lead to runtime errors if any existing calls to this function do not pass thenetworkType. Please review all usages of this function to confirm thatnetworkTypeis provided.lib/src/crypto/utxo/utils/eurocoin_signing.dart (4)
4-8: Confirm new imports and remove unused dependenciesThe imports have been updated to include
ecurve.dart,crypto.dart, andhd_node.dart, and thebip32import has been removed. Ensure that all functions and classes used from these imports are correctly referenced. Also, verify thatbip32is no longer needed elsewhere in the codebase.
110-110: Ensure correct use ofpointFromScalarfunctionWith the change to
pointFromScalar(childNode.privateKey!, compressed), verify that the function behaves as expected and that it correctly replaces the previous usage ofecc.pointFromScalar. Ensure that any differences in implementation between the two functions do not affect the output.
120-120: UpdatederivationPathto reflect actual path usedSetting
derivationPathtoderivationPathStringprovides accurate information about the node's derivation path. This improves debuggability and traceability of wallet derivations.
106-107:⚠️ Potential issueHandle potential null
privateKeyinchildNodeThe
childNode.privateKey!is force unwrapped. IfprivateKeyisnull, this will cause a runtime exception. Ensure thatchildNode.privateKeyis always non-null at this point, or add a null check to handle the scenario where it might benull.Apply this diff to add null safety:
- final publicKey = pointFromScalar(childNode.privateKey!, compressed)!; + if (childNode.privateKey == null) { + throw StateError('childNode.privateKey cannot be null'); + } + final publicKey = pointFromScalar(childNode.privateKey!, compressed)!;Likely invalid or redundant comment.
lib/src/crypto/network_type.dart (3)
83-88: RefactorNetworkBIPclass for better structureThe
NetworkBIPclass now usesBIP32Prefixesfor different BIP standards, which enhances code clarity by grouping related prefixes. This refactor improves maintainability and aligns with modern Dart practices.
79-80:⚠️ Potential issueEnsure Dart SDK version supports records and type aliases
The use of type aliases with named records, like
typedef BIP32Prefixes = ({int private, int public});, requires Dart SDK version 3.0.0 or higher. Verify that the project's SDK constraints inpubspec.yamlallow for this feature. If not, consider alternative implementations compatible with the current SDK version.
123-140:⚠️ Potential issueValidate version matching logic in
fromVersionThe
fromVersionmethod checks if the provided version matches any of the predefined prefixes. Ensure that all possible versions are accounted for and that there are no overlaps or conflicts among the prefixes that could cause incorrect matches.test/ci/derivation/derive_extended_pubkey_test.dart (1)
40-45: Ensure correct network detection for LitecoinVerify that
deriveMasterNodeFromExtendedKeycorrectly identifies the Litecoin network from the extended public key and thathdNode.networkis properly initialized.test/ci/sending/signature_test.dart (1)
57-64: Confirm parameters forHDNode.fromIEnsure that the parameters passed to
HDNode.fromIcorrectly represent the intended key material:
IL: Should be the private key (master secret key).IR: Should be the chain code.network: Confirm thatBITCOIN_NETWORK_BIPcorresponds to the desired Bitcoin network parameters.lib/src/crypto/wallet_utils.dart (2)
108-113: Verify compatibility with BIP32 standards.The transition from
bip32.BIP32toHDNodefor private key derivation needs validation to ensure it maintains compatibility with BIP32 standards.✅ Verification successful
HDNode implementation verified as BIP32 compliant
The codebase includes comprehensive test coverage derived from the established bip32-dart library, validating all critical BIP32 functionalities including proper key derivation, extended key formats, and seed handling. The implementation maintains compatibility with BIP32 standards and is suitable for Ethereum key derivation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify HDNode implementation matches BIP32 standards # Look for test vectors and compatibility tests rg -A 5 "HDNode.fromSeed" "test/"Length of output: 1339
Script:
#!/bin/bash # Get full content of BIP32 test file and look for test vectors cat test/ci/bip32/bip32-dart_test.dart # Also search for Ethereum-specific BIP32 tests rg -l "ethereum" "test/ci/bip32/"Length of output: 9352
117-122: Ensure consistent public key derivation.The public key derivation logic has been updated to use HDNode. Verify that the uncompressed public key format matches the Ethereum requirements.
✅ Verification successful
Public key derivation implementation verified as correct
The implementation correctly follows Ethereum's requirements:
- Uses proper BIP44 derivation path (m/44'/60'/0'/0/0)
- Generates uncompressed public keys as required by Ethereum
- Implements secure key derivation using standard HD wallet specifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the public key format is consistent with Ethereum requirements ast-grep --pattern 'publicKeyUncompressed'Length of output: 42
Script:
#!/bin/bash # Search for HDNode class definition and usage ast-grep --pattern 'class HDNode' # Search for wallet test files fd -t f -e dart test wallet # Search for Ethereum-specific key handling rg -l "ethereum.*key" --type dartLength of output: 195
Script:
#!/bin/bash # List files in crypto directory fd . lib/src/crypto # Search for test files fd test -e dart # Search for key derivation and BIP44 related code rg -l "bip44|HDNode|publicKey" --type dart # Search for Ethereum path constants rg "ethereumBip44" --type dart -A 2Length of output: 13075
Script:
#!/bin/bash # Check HDNode implementation cat lib/src/wallet/hd_node.dart # Check EthereumBip44HDPath implementation cat lib/src/domain/entities/hd_wallet_type.dart # Look for public key tests rg "publicKeyUncompressed.*test" -A 3Length of output: 15136
lib/src/domain/entities/node.dart (2)
8-8: Review the neutered state implementation.The change in accessing the neutered state (
isNeuteredproperty instead of method) suggests a different API in HDNode. Ensure this maintains the same security guarantees for private key handling.Also applies to: 15-15
✅ Verification successful
Implementation of neutered state is correct and secure
The HDNode's
isNeuteredproperty correctly determines the state based on private key presence, and the delegation in NodeWithAddress maintains the same security guarantees with proper null safety handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify neutered state handling in HDNode ast-grep --pattern 'class HDNode { $$$ bool get isNeutered { $$$ } $$$ }'Length of output: 87
Script:
#!/bin/bash # Find HDNode class definition rg "class HDNode" --type dart -A 5 # Also search for any references to isNeutered rg "isNeutered" --type dart -A 2Length of output: 2218
48-48: Validate HDNode parameter type change.The factory method now accepts HDNode instead of BIP32. Ensure all callers have been updated to provide the correct type.
✅ Verification successful
Parameter type change from BIP32 to HDNode is consistent across the codebase.
The codebase has been fully migrated to use HDNode type, with no remaining BIP32 type parameters found. All callers have been properly updated to use the correct type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining BIP32 usage rg "BIP32" --type dartLength of output: 1092
Script:
#!/bin/bash # Check for HDNode usage echo "=== HDNode Usage ===" rg "HDNode" --type dart -A 2 echo -e "\n=== Looking for BIP32 type parameters ===" ast-grep --pattern 'BIP32 $_'Length of output: 12124
lib/src/domain/constants.dart (2)
43-51: Verify Bitcoin network parameters.The BIP32, BIP49, and BIP84 parameters for Bitcoin need verification against the official specifications.
Consider adding references to the BIP specifications in comments for future maintainability.
54-61: Validate altcoin network parameters.The Litecoin and Dogecoin network parameters need verification:
- Litecoin uses different BIP32 prefixes (Ltub/Ltpv)
- Dogecoin appears to use the same prefixes for all BIP paths
Consider adding validation tests for these constants to prevent accidental modifications.
Also applies to: 64-70
lib/src/crypto/tron/entities/tron_address.dart (1)
1-5: LGTM! Clean import reorganization.The import changes correctly align with the new crypto utilities structure.
lib/src/wallet/hd_node.dart (1)
72-86: 🛠️ Refactor suggestionEnhance security for extended private key handling
The
extendedPrivateKeymethod returns null for neutered nodes, which could lead to null pointer exceptions.Consider throwing an explicit exception instead of returning null:
String? extendedPrivateKey({ int? version, }) { version ??= network?.bip32.private; if (version == null) { throw ArgumentError("Missing version"); } if (version.isUint(32) == false) { throw ArgumentError("Expected UInt32 for version"); } if (isNeutered) { - return null; + throw ArgumentError("Cannot export extended private key from neutered node"); }Likely invalid or redundant comment.
test/ci/bip32/fixtures.json (1)
1-359: LGTM! Comprehensive test vectors for BIP32 implementationThe test fixtures provide excellent coverage for both valid and invalid cases, including:
- Master key generation from seeds
- Child key derivation
- Network-specific cases (Bitcoin, Litecoin)
- Edge cases with leading zeros
- Invalid inputs for proper error handling
🧰 Tools
🪛 Gitleaks (8.21.2)
8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
79-79: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
80-80: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
96-96: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
97-97: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
109-109: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
110-110: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
124-124: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
125-125: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
138-138: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
153-153: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
154-154: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
167-167: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
168-168: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
185-185: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
186-186: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
198-198: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
199-199: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
215-215: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
228-228: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
229-229: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
245-245: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
246-246: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
259-259: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
260-260: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| return HDNode.fromExtendedKey( | ||
| ePubKey, | ||
| switch ((networkType, purpose)) { | ||
| (UTXONetworkType network, HDWalletPurpose purpose) => | ||
| network.networkBIP.getForWalletType(purpose), | ||
| _ => bip32.NetworkType( | ||
| wif: 0x80, | ||
| bip32: bip32.Bip32Type( | ||
| private: 0x0488ADE4, | ||
| public: 0x0488B21E, | ||
| ), | ||
| ), | ||
| }, | ||
| network: networkType?.networkBIP, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential null values for networkType
In deriveMasterNodeFromExtendedKey, the optional networkType parameter is used to access networkType?.networkBIP. If networkType is null, this could lead to unexpected behavior in HDNode.fromExtendedKey. Consider providing a default network type or adding checks to handle the null case appropriately.
Apply this diff to handle null networkType:
- return HDNode.fromExtendedKey(
- ePubKey,
- network: networkType?.networkBIP,
- );
+ final network = networkType?.networkBIP ?? defaultNetworkBIP;
+ return HDNode.fromExtendedKey(
+ ePubKey,
+ network: network,
+ );Replace defaultNetworkBIP with the appropriate default value.
Committable suggestion skipped: line range outside the PR's diff.
| return deriveMasterNodeFromSeed( | ||
| seed: arg.$1, | ||
| walletPath: arg.$2, | ||
| networkType: networkType, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle exceptions when deriving master node from seed
The deriveMasterNodeFromSeed function may throw exceptions if the seed or wallet path is invalid. It's advisable to wrap this call in a try-catch block to manage potential errors gracefully.
Apply this diff to include error handling:
try {
return deriveMasterNodeFromSeed(
seed: arg.$1,
walletPath: arg.$2,
networkType: networkType,
);
} catch (e) {
+ // Handle the error appropriately
+ Logger.logError('Failed to derive master node from seed: \$e');
+ // You could rethrow the exception or handle it as needed
+ throw Exception('Invalid seed or wallet path.');
}Committable suggestion skipped: line range outside the PR's diff.
| return deriveMasterNodeFromExtendedKey( | ||
| arg.$1, | ||
| networkType: arg.$2, | ||
| purpose: arg.$3, | ||
| ); | ||
| }, | ||
| argument: (ePubKey, networkType, purpose), | ||
| argument: (ePubKey, networkType), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for deriveMasterNodeFromExtendedKey
When calling deriveMasterNodeFromExtendedKey, consider adding error handling to handle cases where the extended key might be invalid or incompatible with the provided networkType. This will enhance the robustness of the code by preventing unhandled exceptions.
Apply this diff to include error handling:
try {
return deriveMasterNodeFromExtendedKey(
arg.$1,
networkType: arg.$2,
);
} catch (e) {
+ // Handle the error appropriately
+ Logger.logError('Failed to derive master node: \$e');
+ // You could rethrow the exception or handle it as needed
+ throw Exception('Invalid extended key or network type.');
}Committable suggestion skipped: line range outside the PR's diff.
| var hdNode = deriveMasterNodeFromExtendedKey(btcPubKey); | ||
|
|
||
| expect(version, BitcoinNetwork.networkBIP.bip32XpubPrefix); | ||
| expect( | ||
| hdNode.network?.bip32.public, | ||
| BitcoinNetwork.networkBIP.bip32.public, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert hdNode.network is not null before property access
In the test case, you access hdNode.network?.bip32.public. To prevent potential null dereference, add an assertion to ensure hdNode.network is not null before accessing its properties.
Apply this diff to include the assertion:
+ expect(hdNode.network, isNotNull, reason: 'Network should not be null');
expect(
hdNode.network?.bip32.public,
BitcoinNetwork.networkBIP.bip32.public,
);📝 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.
| var hdNode = deriveMasterNodeFromExtendedKey(btcPubKey); | |
| expect(version, BitcoinNetwork.networkBIP.bip32XpubPrefix); | |
| expect( | |
| hdNode.network?.bip32.public, | |
| BitcoinNetwork.networkBIP.bip32.public, | |
| ); | |
| var hdNode = deriveMasterNodeFromExtendedKey(btcPubKey); | |
| expect(hdNode.network, isNotNull, reason: 'Network should not be null'); | |
| expect( | |
| hdNode.network?.bip32.public, | |
| BitcoinNetwork.networkBIP.bip32.public, | |
| ); |
| String base58CheckEncode2(Uint8List payload) { | ||
| return Base58CheckCodec.bitcoin().encode( | ||
| Base58CheckPayload(payload.first, payload.sublist(1)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review payload handling safety.
The new base58CheckEncode2 method assumes the first byte is the version. This could be unsafe if the payload is empty or if the caller is unaware of this requirement.
Consider adding validation:
String base58CheckEncode2(Uint8List payload) {
+ if (payload.isEmpty) {
+ throw ArgumentError('Payload cannot be empty');
+ }
return Base58CheckCodec.bitcoin().encode(
Base58CheckPayload(payload.first, payload.sublist(1)),
);
}📝 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.
| String base58CheckEncode2(Uint8List payload) { | |
| return Base58CheckCodec.bitcoin().encode( | |
| Base58CheckPayload(payload.first, payload.sublist(1)), | |
| ); | |
| } | |
| String base58CheckEncode2(Uint8List payload) { | |
| if (payload.isEmpty) { | |
| throw ArgumentError('Payload cannot be empty'); | |
| } | |
| return Base58CheckCodec.bitcoin().encode( | |
| Base58CheckPayload(payload.first, payload.sublist(1)), | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/crypto/utxo/utils/pubkey_to_address.dart (1)
Line range hint
143-190: Enhance documentation for BCH address polynomial calculations.The BCH address implementation uses complex polynomial math for checksum calculation. Consider adding:
- Documentation explaining the polynomial modulo algorithm
- References to the BCH address specification
- Examples of input/output values
Example documentation:
+ /// Calculates the BCH address checksum using polynomial modulation as specified in the + /// BCH address format (CashAddr). + /// + /// The algorithm uses a set of generators to create a unique checksum that helps detect + /// errors in addresses. For details, see: + /// https://github.com/bitcoincashorg/bitcoincash.org/blob/master/spec/cashaddr.md + /// + /// Example: + /// ```dart + /// final checksum = _polymod([/* example data */]); + /// print(checksum); // Expected output: ... + /// ``` BigInt _polymod(List<int> data) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/crypto/tron/entities/tron_transaction_utils.dart(1 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(1 hunks)lib/src/crypto/utxo/utils/proof_of_payment.dart(2 hunks)lib/src/crypto/utxo/utils/pubkey_to_address.dart(1 hunks)test/ci/tron/send_simulation_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ci/tron/send_simulation_test.dart
- lib/src/crypto/tron/entities/tron_transaction_utils.dart
- lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart
- lib/src/crypto/utxo/utils/proof_of_payment.dart
🔇 Additional comments (2)
lib/src/crypto/utxo/utils/pubkey_to_address.dart (2)
7-7: LGTM! Good refactoring of crypto utilities.Moving hash functions to a dedicated crypto utility file improves code organization and maintainability.
Line range hint
20-41: Verify hash function compatibility after refactoring.The functions still reference
ripmed160Sha256Hashandsha256Sha256Hashwhich were moved to crypto.dart. Let's verify that the moved implementations maintain the same interface and behavior.Also applies to: 45-64, 89-102
✅ Verification successful
Hash function implementations and usage verified successfully
The hash functions have been properly moved to
lib/src/utils/crypto.dartand maintain their correct implementations. All usages across the codebase are properly resolved and consistent with the expected cryptographic operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hash function implementations in crypto.dart # Check the function signatures and implementations ast-grep --pattern 'ripmed160Sha256Hash($buffer) { $$$ }' ast-grep --pattern 'sha256Sha256Hash($buffer) { $$$ }' # Look for any test files that verify these functions fd -e dart -e test | xargs rg "test.*ripmed160Sha256Hash|test.*sha256Sha256Hash"Length of output: 203
Script:
#!/bin/bash # First, find files containing these hash functions echo "=== Files containing hash functions ===" rg -l "ripmed160Sha256Hash|sha256Sha256Hash" --type dart echo -e "\n=== Implementation details ===" # Show the actual implementations with context rg -B2 -A5 "ripmed160Sha256Hash|sha256Sha256Hash" --type dart echo -e "\n=== Test files ===" # Search specifically in test files fd -e dart -g "*test*" | xargs rg "ripmed160Sha256Hash|sha256Sha256Hash"Length of output: 11750
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/src/wallet/hd_node.dart (2)
85-86:⚠️ Potential issueEnsure
extendedPrivateKeydoes not return nullThe
extendedPrivateKeymethod returnsnullwhen the node is neutered, which may lead to unexpectednullreferences when callers expect aString. Consider throwing an exception or providing an alternative mechanism to handle neutered nodes to prevent potentialNullerrors downstream.
167-168:⚠️ Potential issuePotential infinite recursion in
derivemethodThe recursive calls to
derive(index + 1)when invalid keys are encountered lack a termination condition. This could lead to infinite recursion and a stack overflow if invalid keys persist. Implement a maximum retry limit or handle such cases iteratively to prevent potential runtime errors.Also applies to: 173-174, 185-187
test/ci/derivation/derivation_path_test.dart (1)
53-65:⚠️ Potential issueComplete the HD Wallet Derivation test implementation.
The test case has several issues:
- Missing assertions to verify the derivation results
- Contains a commented-out line that needs to be implemented
- Only prints the seed without verifying its correctness
Apply this diff to complete the test implementation:
test( "HD Wallet Derivation", () { const mnemonic = "walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart walletkit dart"; final seed = mnemonicToSeed(mnemonic); - - // final masterNode = deriveMasterNodeFromSeed(seed: seed, walletPath: ); - - print(seed); + final masterNode = deriveMasterNodeFromSeed( + seed: seed, + networkType: BitcoinNetwork, + walletPath: bitcoinBip44HDPath, + ); + + final derivedNode = masterNode.derivePath("m/44'/0'/0'/0/0"); + + // Add assertions to verify the derived node + expect(derivedNode.depth, equals(5)); + expect(derivedNode.index, equals(0)); + expect(derivedNode.isNeutered, isFalse); }, );
🧹 Nitpick comments (7)
lib/src/wallet/hd_node.dart (3)
139-141: Enhance error message for invalid index inderiveHardenedIn the
deriveHardenedmethod, the error message can be more informative by including the invalid index value. This aids in debugging by showing the exact value that caused the error.Apply this diff to improve the error message:
- throw ArgumentError("Expected UInt31"); + throw ArgumentError("Expected UInt31, got $index");
147-148: Enhance error message for invalid index inderiveIn the
derivemethod, including the actual index value in the error message helps with troubleshooting invalid inputs.Apply this diff:
- throw ArgumentError("Expected UInt32"); + throw ArgumentError("Expected UInt32, got $index");
88-100: Replace magic numbers with named constants for clarityIn the
extendedPrivateKeyandextendedPublicKeymethods, you're using magic numbers for buffer offsets, which can reduce code readability and maintainability. Defining these offsets as named constants improves clarity.Example:
+const int VERSION_OFFSET = 0; +const int DEPTH_OFFSET = 4; +const int PARENT_FINGERPRINT_OFFSET = 5; +const int INDEX_OFFSET = 9; +const int CHAIN_CODE_OFFSET = 13; +const int CHAIN_CODE_END_OFFSET = 45; +const int KEY_PREFIX_OFFSET = 45; +const int PRIVATE_KEY_OFFSET = 46; +const int BUFFER_SIZE = 78; - final buffer = Uint8List(78); + final buffer = Uint8List(BUFFER_SIZE); final bytes = buffer.buffer.asByteData(); - bytes.setUint32(0, version); - bytes.setUint8(4, depth); - bytes.setUint32(5, parentFingerprint); - bytes.setUint32(9, index); - buffer.setRange(13, 45, chainCode); - bytes.setUint8(45, 0); - buffer.setRange(46, 78, privateKey!); + bytes.setUint32(VERSION_OFFSET, version); + bytes.setUint8(DEPTH_OFFSET, depth); + bytes.setUint32(PARENT_FINGERPRINT_OFFSET, parentFingerprint); + bytes.setUint32(INDEX_OFFSET, index); + buffer.setRange(CHAIN_CODE_OFFSET, CHAIN_CODE_END_OFFSET, chainCode); + bytes.setUint8(KEY_PREFIX_OFFSET, 0); + buffer.setRange(PRIVATE_KEY_OFFSET, BUFFER_SIZE, privateKey!);Also applies to: 111-122
lib/src/crypto/network_type.dart (1)
111-125: Optimize thefindPrefixesFromVersionmethodThe
findPrefixesFromVersionmethod iterates over all default networks to find matching prefixes. For efficiency, consider using aMapto index networks by their versions, reducing the lookup time from O(n) to O(1).Example:
// Build a version-to-network map (this could be done statically) static final _versionToNetworkMap = { for (var network in defaults) ...{ network.bip32.private: network, network.bip32.public: network, network.bip49.private: network, network.bip49.public: network, network.bip84.private: network, network.bip84.public: network, network.bip49MultiSig.private: network, network.bip49MultiSig.public: network, network.bip84MultiSig.private: network, network.bip84MultiSig.public: network, } }; static NetworkNodeInfo? findPrefixesFromVersion(int version) { final network = _versionToNetworkMap[version]; if (network != null) { final prefixes = network.fromVersion(version); if (prefixes != null) { return NetworkNodeInfo( wif: network.wif, keyPrefixes: prefixes, ); } } return null; }test/ci/fetching/epubkey_test.dart (2)
19-21: Remove redundant variable for consistency in Bitcoin testsIn the Bitcoin EPubKey derivation tests, you're directly using the wallet purpose constants without the
walletTypevariable. However, the variable declaration comments remain. Removing these comments enhances code clarity.Apply this diff to clean up the code:
final pubKey = deriveExtendedPubKey( seed: helloSeed, type: BitcoinNetwork, - // walletType: bitcoinBip44HDPath, walletPurpose: bitcoinBip44HDPath, ); final pubKey2 = deriveExtendedPubKey( seed: helloSeed, type: BitcoinNetwork, - // walletType: bitcoinBip49HDPath, walletPurpose: bitcoinBip49HDPath, ); final pubKey3 = deriveExtendedPubKey( seed: helloSeed, type: BitcoinNetwork, - // walletType: bitcoinBip84HDPath, walletPurpose: bitcoinBip84HDPath, ); final pubKey4 = deriveExtendedPubKey( seed: helloSeed, type: BitcoinNetwork, - // walletType: bitcoinNSHDPath, walletPurpose: bitcoinNSHDPath, );Also applies to: 27-29, 35-37, 43-45
Line range hint
52-76: Align Litecoin tests with Bitcoin tests for consistencyIn the Litecoin EPubKey derivation tests, you're using the
walletTypevariable, whereas in Bitcoin tests, you directly use the wallet purpose constants. For consistency and code simplicity, consider removing thewalletTypevariable and using direct references.Apply this diff to update the Litecoin tests:
- HDWalletPath walletType = bitcoinBip44HDPath; final pubKey = deriveExtendedPubKey( seed: helloSeed, type: LitecoinNetwork, - walletPurpose: walletType, + walletPurpose: litecoinBip44HDPath, ); expect(pubKey, "xpub69BsBUCquKCiiip64fmSF5RPt4XXudBeqAGGdBrMCCUanxaAc9myKLEMHUV5pUNDCXyYpjyhT7RocMXXocxzZMYgLWD3LFLYiWWoUA37i3h"); - walletType = bitcoinBip49HDPath; final pubKey2 = deriveExtendedPubKey( seed: helloSeed, type: LitecoinNetwork, - walletPurpose: walletType, + walletPurpose: litecoinBip49HDPath, ); expect(pubKey2, "xpub69BsBUCquKCixpE9wVXQmxiooc57zXbvujFuAreBtvbDjt66EXtGs8WXPjtAR2aCBwP4imDmQJN4v5kNcQD2sSueg12RxjTmTzxAzwjB4rT"); - walletType = bitcoinBip84HDPath; final pubKey3 = deriveExtendedPubKey( seed: helloSeed, type: LitecoinNetwork, - walletPurpose: walletType, + walletPurpose: litecoinBip84HDPath, ); expect(pubKey3, "xpub69BsBUCquKCkUvJczrqfBKDeHTRzNRsdBm61D72VjRPmtiTpzqz98k2wYnQppBb1TLfQp16iMMapaBJ2MSbtsCKRkDti9goSWpUbaAeSKQx"); - walletType = bitcoinNSHDPath; final pubKey4 = deriveExtendedPubKey( seed: helloSeed, type: LitecoinNetwork, - walletPurpose: walletType, + walletPurpose: bitcoinNSHDPath, ); expect(pubKey4, "xpub69BsBUCquKCgn1u6cum7syZewx5J2fyJE479qzDRJtB6hSHkhLsPZZW2SBfPfJaidDtE9g6wyH7ZFv8bWTkwuLeRJx3H2Kray9AsvY8EJAP"); - walletType = litecoinBip44HDPath; final pubKey5 = deriveExtendedPubKey( seed: helloSeed, type: LitecoinNetwork, - walletPurpose: walletType, + walletPurpose: litecoinBip44HDPath, ); expect(pubKey5, "xpub69BsBUCquKCgooMeNw2a3hW6NBZ9Kwn4kYUxYSjygK6YTa8PRqqAEyAKEwBsNH4GiPpb4BCXZ1QiHDeYCtPJtVJoBWNeF8QFRik7MyUstiZ");lib/src/crypto/utxo/utxo_analyzer.dart (1)
Line range hint
642-699: Add detailed documentation for transaction direction logic.The
determineSendDirectionfunction contains complex logic for determining transaction types. Consider adding detailed documentation explaining:
- The significance of each case in the pattern matching
- The implications of having both inputs and outputs belonging to us
- The difference between receive and change addresses in the context of direction determination
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/dart.yml(1 hunks)lib/src/crypto/network_type.dart(2 hunks)lib/src/crypto/utxo/utils/derivation.dart(5 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(5 hunks)lib/src/wallet/hd_node.dart(1 hunks)test/ci/bip32/bip32-dart_test.dart(1 hunks)test/ci/derivation/derivation_path_test.dart(2 hunks)test/ci/derivation/derive_addresses_test.dart(4 hunks)test/ci/derivation/derive_extended_pubkey_test.dart(1 hunks)test/ci/fetching/epubkey_test.dart(1 hunks)test/ci/sending/signature_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ci/derivation/derive_extended_pubkey_test.dart
- test/ci/bip32/bip32-dart_test.dart
🔇 Additional comments (14)
lib/src/crypto/network_type.dart (1)
79-79: Verify Dart SDK version compatibility for record typesThe use of record types in the type alias:
typedef BIP32Prefixes = ({int private, int public});requires Dart SDK version 3.0 or higher. Ensure that the project is using Dart 3.0+ to support this syntax. If the project aims to maintain compatibility with earlier Dart versions, consider using a class or a traditional
typedefinstead.Please confirm the Dart SDK version specified in your project's configuration.
✅ Verification successful
Record types are compatible with project's SDK requirements ✅
The project's
pubspec.yamlspecifiessdk: ^3.0.0, which ensures compatibility with record types introduced in Dart 3.0.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check pubspec.yaml for SDK constraints echo "=== Checking pubspec.yaml for SDK constraints ===" cat pubspec.yaml | grep -A 5 "environment:" # Look for other record type usages echo -e "\n=== Checking for other record type patterns ===" rg "typedef.*=.*\({.*}\)" --type dartLength of output: 517
test/ci/sending/signature_test.dart (2)
5-6: LGTM! Required imports added for HDNode implementation.The imports correctly include the necessary dependencies for the HDNode functionality.
Also applies to: 8-8
58-65: LGTM! Proper HDNode instantiation with all required parameters.The transition from BIP32 to HDNode is implemented correctly with appropriate parameters:
- Private key and chain code are properly passed as IL and IR
- Network configuration is correctly specified
- Depth, index, and parent fingerprint are properly initialized
test/ci/derivation/derivation_path_test.dart (2)
5-5: LGTM! Added BIP39 import for mnemonic handling.The import is correctly added for mnemonic functionality.
46-48: LGTM! Updated node derivation method.The transition to the new HDNode derivation method is implemented correctly.
lib/src/crypto/utxo/utils/derivation.dart (4)
10-10: LGTM! Made network type parameter required.Good improvement to ensure type safety by making the network type parameter required.
40-48: LGTM! Updated master node derivation implementation.The implementation correctly:
- Uses HDNode.fromSeed with proper parameters
- Handles network type and wallet path appropriately
60-67: Verify error handling in deriveMasterNodeFromExtendedKey.The new implementation removes previous error handling for buffer length and version mismatches.
Run this script to verify error handling:
#!/bin/bash # Description: Check if HDNode.fromExtendedKey handles invalid inputs appropriately # Test: Search for error handling in HDNode implementation ast-grep --pattern 'class HDNode { $$$ static HDNode fromExtendedKey($_, {$_}) { $$$ throw $_($$$); $$$ } $$$ }'
Line range hint
126-146: LGTM! Updated extension implementation for HDNode.The extension method correctly:
- Handles both private and public key scenarios
- Maintains compatibility with the wallet kit
- Properly manages buffer operations
.github/workflows/dart.yml (1)
67-67: LGTM! Added BIP32 test path to CI workflow.The addition of "test/ci/bip32" to the test matrix ensures proper coverage of the new HDNode implementation.
test/ci/derivation/derive_addresses_test.dart (1)
21-22: LGTM! Consistently updated derivation method calls.All instances of
deriveMasterNodeFromExtendedKeyhave been properly updated to:
- Use the new method signature
- Remove the purpose parameter
- Convert ePubKey to a positional argument
Also applies to: 56-57, 100-101, 469-470
lib/src/crypto/utxo/utxo_analyzer.dart (3)
9-9: LGTM! Transition to custom HD wallet implementation.The import change aligns with the architectural shift from the bip32 package to a custom HD wallet implementation.
283-289: 🛠️ Refactor suggestionAdd error handling for seed-based master node derivation.
The seed-based master node derivation could fail due to invalid seeds or wallet paths. Consider adding proper error handling.
final masterNode = await isolateManager.executeTask( IsolateTask( task: (arg) { + try { return deriveMasterNodeFromSeed( seed: arg.$1, walletPath: arg.$2, networkType: arg.$3, ); + } catch (e) { + Logger.logError('Failed to derive master node from seed: $e'); + throw Exception('Invalid seed or wallet path.'); + } }, argument: (seed, walletType, networkType), ), );Likely invalid or redundant comment.
324-324: Verify all callers for the updated parameter type.The parameter type change from
BipNodetoHDNodeneeds to be verified across all callers to ensure compatibility.✅ Verification successful
Parameter type change is consistent with codebase
The migration from
BipNodetoHDNodehas been completely implemented across the codebase.HDNodeis consistently used in all related functionality including wallet operations, derivations, and transaction handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to BipNode rg "BipNode" --type dart # Search for all calls to searchTransactionsForWalletType ast-grep --pattern 'searchTransactionsForWalletType($$$)'Length of output: 84
Script:
#!/bin/bash # Find the containing class and method name rg -B 5 -A 5 "required HDNode masterNode" --type dart # Search for HDNode usage rg "HDNode" --type dart # Find the file name to get more context fd "utxo_analyzer.dart$"Length of output: 6661
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
example/commands.dart (1)
5-24: Add null check for CommandRegistry.The
HelpCommandconstructor should validate that the registry is not null.final CommandRegistry registry; - HelpCommand(this.registry); + HelpCommand(this.registry) { + ArgumentError.checkNotNull(registry, 'registry'); + }example/command.dart (1)
9-29: Enhance CommandResult with copyWith method.Add a
copyWithmethod to support immutable updates toCommandResultinstances.+ CommandResult<T> copyWith({ + bool? success, + T? data, + String? error, + }) { + return CommandResult<T>( + success: success ?? this.success, + data: data ?? this.data, + error: error ?? this.error, + ); + }example/wkdart.dart (1)
93-95: Implement wallet synchronization.The TODO comment should be implemented to handle wallet synchronization.
Would you like me to help implement the wallet synchronization logic? I can provide a solution that includes:
- Wallet initialization
- Periodic sync with configurable interval
- Error handling and retry logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example/command.dart(1 hunks)example/commands.dart(1 hunks)example/wkdart.dart(1 hunks)
🔇 Additional comments (1)
example/wkdart.dart (1)
1-6: Add necessary wallet-related imports.As mentioned in the previous review, add the required wallet-related imports:
import 'dart:convert'; import 'dart:io'; +import 'package:walletkit_dart/walletkit_dart.dart'; +import 'dart:async'; import 'command.dart'; import 'commands.dart';
| class ExitCommand extends Command<void> { | ||
| @override | ||
| String get name => 'exit'; | ||
|
|
||
| @override | ||
| String get description => 'Exit the application'; | ||
|
|
||
| @override | ||
| Future<void> execute(List<String> args) async { | ||
| print('Goodbye!'); | ||
| exit(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement graceful shutdown.
The ExitCommand uses exit(0) which abruptly terminates the application. Consider implementing a graceful shutdown that allows cleanup of resources.
@override
Future<void> execute(List<String> args) async {
print('Goodbye!');
- exit(0);
+ // Signal the application to start cleanup
+ return Future.value();
}📝 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.
| class ExitCommand extends Command<void> { | |
| @override | |
| String get name => 'exit'; | |
| @override | |
| String get description => 'Exit the application'; | |
| @override | |
| Future<void> execute(List<String> args) async { | |
| print('Goodbye!'); | |
| exit(0); | |
| } | |
| } | |
| class ExitCommand extends Command<void> { | |
| @override | |
| String get name => 'exit'; | |
| @override | |
| String get description => 'Exit the application'; | |
| @override | |
| Future<void> execute(List<String> args) async { | |
| print('Goodbye!'); | |
| // Signal the application to start cleanup | |
| return Future.value(); | |
| } | |
| } |
| void register(Command command) { | ||
| _commands[command.name.toLowerCase()] = command; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add duplicate command name validation.
The register method should check for existing commands with the same name (case-insensitive) to prevent overwriting.
void register(Command command) {
+ ArgumentError.checkNotNull(command, 'command');
+ final normalizedName = command.name.toLowerCase();
+ if (_commands.containsKey(normalizedName)) {
+ throw ArgumentError('Command "${command.name}" is already registered');
+ }
- _commands[command.name.toLowerCase()] = command;
+ _commands[normalizedName] = command;
}📝 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.
| void register(Command command) { | |
| _commands[command.name.toLowerCase()] = command; | |
| } | |
| void register(Command command) { | |
| ArgumentError.checkNotNull(command, 'command'); | |
| final normalizedName = command.name.toLowerCase(); | |
| if (_commands.containsKey(normalizedName)) { | |
| throw ArgumentError('Command "${command.name}" is already registered'); | |
| } | |
| _commands[normalizedName] = command; | |
| } |
| final inputStream = stdin | ||
| .transform(utf8.decoder) | ||
| .transform(const LineSplitter()) | ||
| .asBroadcastStream(); | ||
|
|
There was a problem hiding this comment.
Implement stream subscription cleanup.
The broadcast stream subscription should be stored and cleaned up when the CLI is terminated.
+ StreamSubscription<String>? _inputSubscription;
+
Future<void> run() async {
// ... existing code ...
- inputStream.listen(
+ _inputSubscription = inputStream.listen(
// ... existing code ...
);
}
+ Future<void> cleanup() async {
+ await _inputSubscription?.cancel();
+ _inputSubscription = null;
+ }Committable suggestion skipped: line range outside the PR's diff.
| void main() async { | ||
| final app = WalletCLI(); | ||
| await app.run(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add signal handling for graceful shutdown.
Implement signal handling to ensure proper cleanup when the application is terminated.
void main() async {
final app = WalletCLI();
+ ProcessSignal.sigint.watch().listen((_) async {
+ print('\nReceived SIGINT, shutting down...');
+ await app.cleanup();
+ exit(0);
+ });
await app.run();
}Committable suggestion skipped: line range outside the PR's diff.
* feat: add ERC1155 support with balance fetching and transfer function… (#133) * feat: add ERC1155 support with balance fetching and transfer functionality * feat: add ERC1155 export to EVM module * refactor: rename 'account' parameter to 'address' for consistency in ERC1155 balance functions * feat: add isErc1155 function to check ERC1155 token support and include optional token ID in TokenInfo * feat: add ERC1155 transaction fetching and balance handling in EVM module * fix: update tokenValue parsing to default to zero instead of -1 in EtherscanTransaction * Add Hex Normalization to prevent leading Zeros in RLP (#135) * Add Hex Normalization to prevent leading Zeros in RLP * Start of Reimplementing RLP & Testing * Implement all Test Cases and Decoding * Finish RawEvmTx Encoding and Decode and fix lint * Refactor Buffer to int/BigInt and vice versa * Refactor BigInt Conversion * Refactor RLP encoding to use sublist for length extraction * Remove unused import * Fix Access List Encoding & add isHex Parameter to RLPString to indicate hex * Add RLP test directory to GitHub Actions workflow * Add decodeRLPCheck function for input validation and refactor length extraction (check for leading 0) --------- Co-authored-by: dev2 <[email protected]> Co-authored-by: thomas.fercher <[email protected]> * FunctionParamWithValue JsonEncoding (#136) * initial * feat: implement JSON serialization and deserialization for function parameter types * Fix Lint --------- Co-authored-by: dev2 <[email protected]> * fix: update toJson method for EvmType2GasPrice to use proper serialization (#138) * Initial * Update PipeTests * Fix Tests * Fix Lint * Update GitHub Actions to use upload-artifact@v4 * Update Tests * Fix Lints --------- Co-authored-by: The Nomo App's core developer team <[email protected]> Co-authored-by: thomas.fercher <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (26)
lib/src/domain/entities/fee.dart (1)
160-161: LGTM! Consider reordering properties for consistency.The update to use
toJson()for serialization is correct and aligns with the pattern used throughout the codebase. This ensures proper JSON serialization of gas price properties.Consider reordering the properties to match the constructor parameter order:
Json toJson() { return { - 'maxPriorityFeePerGas': maxPriorityFeePerGas.toJson(), - 'maxFeePerGas': maxFeePerGas.toJson(), + 'maxFeePerGas': maxFeePerGas.toJson(), + 'maxPriorityFeePerGas': maxPriorityFeePerGas.toJson(), }; }lib/src/wallet/bip39/bip39.dart (2)
86-93: Secure random approach is good but evaluate additional entropy.
Random.secure()is a strong PRNG. The fallback to_getRandomBytesMixusing timestamps and PIDs might introduce minor security risk if implemented incorrectly. Evaluate whether you want the additional complexity or to rely solely onRandom.secure().
128-134: Strength assertion is good, but handle invalid user-supplied values gracefully.
Currently, assertions are used to enforce validstrength. Assertions are removed in production builds. For user-facing code, consider throwing an exception or returning an error ifstrengthis invalid.lib/src/crypto/evm/utils/rlp.dart (3)
16-45: Consider using a factory or external function for item creation.
RLPItem.fromValueis convenient but large. Splitting out logic for each type might improve testability and help future expansions (e.g., new types).
98-101: Keep raw byte items distinct.
RLPBytesis straightforward, but ensure you only store raw bytes (not strings or other data). Consider clarifying docstrings.
306-315: No leading zero check is standard.
Rejecting leading zeros is correct to avoid malignant data. Consider whether you want an explicit error message referencing RLP spec.lib/src/wallet/bip39/pbkdf2.dart (1)
24-31: Consider adding input validation for the mnemonic parameter.While the implementation is correct, it would be beneficial to validate the mnemonic string before processing.
Consider adding validation:
Uint8List process(String mnemonic, {passphrase = ""}) { + if (mnemonic.isEmpty) { + throw ArgumentError('Mnemonic cannot be empty'); + } final Uint8List mnemonicBytes = Uint8List.fromList(utf8.encode(mnemonic)); final salt = Uint8List.fromList(utf8.encode(saltPrefix + passphrase)); _derivator.reset(); _derivator.init(new Pbkdf2Parameters(salt, iterationCount, desiredKeyLength)); return _derivator.process(mnemonicBytes); }lib/src/crypto/wallet_utils.dart (1)
49-66: Consider improving error handling in isErc1155.The implementation could benefit from more specific error handling to distinguish between contract-related errors and RPC failures.
Consider this improvement:
Future<bool> isErc1155({ required String contractAddress, required EvmRpcInterface rpc, required String address, }) async { - bool isErc1155 = false; try { await rpc.fetchERC1155BalanceOfToken( address: address, tokenID: BigInt.from(0), contractAddress: contractAddress, ); - isErc1155 = true; + return true; } catch (e) { - isErc1155 = false; + if (e.toString().contains('execution reverted')) { + // Contract exists but is not ERC1155 + return false; + } + // Re-throw RPC or network errors + rethrow; } - return isErc1155; }lib/src/domain/entities/coin_entity.dart (1)
108-122: Consider adding fallback or validation for tokenId parsing.If
tokenIdis not strictly guaranteed to be parseable asBigInt, a try/catch or additional validation may help avoid runtime errors.lib/src/domain/extensions.dart (4)
22-24: Consider renaming for clarity.While the implementation is correct, consider renaming
toBytestotoBytesSignedto explicitly indicate that it handles signed values, maintaining consistency withtoBytesUnsigned.- Uint8List get toBytes { + Uint8List get toBytesSigned { return p_utils.encodeBigInt(this); }
49-60: Add size validation and improve documentation.While the implementation correctly handles signed integers using two's complement, it needs the following improvements:
- Add buffer size validation to prevent overflow
- Add documentation explaining the two's complement conversion for negative numbers
/// Decode a big-endian integer from the buffer. /// Can be positive or negative. + /// Uses two's complement for negative numbers where the most significant bit (bit 7) of the first byte indicates sign. int get toInt { + if (length > 8) { + throw ArgumentError('Buffer too large: max 8 bytes for int'); + } bool negative = length > 0 && (this[0] & 0x80) == 0x80; int result = 0; for (var i = 0; i < length; i++) { result = result * 256 + this[i]; } return negative ? -(~result + 1) : result; }
66-71: Consider using pointycastle for better performance.While the implementation is correct, consider using pointycastle's utilities for potentially better performance and consistency with other BigInt operations in the codebase.
BigInt get toUBigInt { - BigInt result = BigInt.zero; - for (var i = 0; i < length; i++) { - result = result * BigInt.from(256) + BigInt.from(this[i]); - } - return result; + if (isEmpty) return BigInt.zero; + // Handle sign bit to ensure unsigned interpretation + final signByte = this[0]; + if ((signByte & 0x80) == 0x80) { + final unsignedBytes = Uint8List(length + 1); + unsignedBytes[0] = 0; + unsignedBytes.setRange(1, length + 1, this); + return p_utils.decodeBigInt(unsignedBytes); + } + return p_utils.decodeBigInt(this); }
76-76: Add documentation about malformed UTF-8 handling.The implementation correctly handles malformed UTF-8, but this behavior should be documented.
+ /// Decodes the buffer as UTF-8 string. + /// Allows malformed UTF-8 sequences by replacing them with the Unicode Replacement Character (U+FFFD). String get bytesToUTF8 => utf8.decode(this, allowMalformed: true);lib/src/crypto/evm/entities/abi/erc/erc1155.dart (4)
322-329: Consider adding documentation for the constructor.
Providing doc comments (triple-slash///) for class constructors helps other developers understand the purpose of each parameter and usage in the broader codebase.
330-339: Avoid referencing contract functions by array index.
Usingabi.functions[0]relies on the ABI's function order. If the ABI changes in the future, the wrong function could be called. If the library supports it, prefer identifying the function by name.- final function = abi.functions[0]; + final function = abi.functions.firstWhere((f) => f.name == 'balanceOf');
355-363: Prefer referencing contract functions by name.
Similar tobalanceOf, referencingabi.functions[7]can be fragile if the ABI changes.- final function = abi.functions[7]; + final function = abi.functions.firstWhere((f) => f.name == 'uri');
365-384: Ensure function index alignment remains valid.
Usingabi.functions[4]forsafeTransferFromworks only as long assafeTransferFromremains in that position in the ABI. Consider referencing by function name to future-proof your code.lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart (1)
19-23: Consider reducing code duplication in JSON serialization.The same JSON serialization logic is duplicated across three classes. Consider moving the implementation to the base class since they all handle
Amounttype identically.abstract class FunctionParamFixedBase extends BaseFunctionParamType<Amount> { // ... existing code ... + @override + valueToJson(Amount value) => value.toJson(); + + @override + Amount valueFromJson(dynamic json) => Amount.fromJson(json); } final class FunctionParamFixed extends BaseFunctionParamType<Amount> { // ... existing code ... - @override - valueToJson(Amount value) => value.toJson(); - - @override - Amount valueFromJson(dynamic json) => Amount.fromJson(json); } final class FunctionParamUFixed extends BaseFunctionParamType<Amount> { // ... existing code ... - @override - valueToJson(Amount value) => value.toJson(); - - @override - Amount valueFromJson(dynamic json) => Amount.fromJson(json); }Also applies to: 40-44, 61-65
lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart (3)
239-254: Consider handling null values in valueFromJson.The tuple implementation looks good, but it might be worth adding null handling for more robustness.
List<dynamic> valueFromJson(dynamic json) { return switch (json) { List list => list.mapIndexed((index, element) { final type = types[index]; - return type.valueFromJson(element); + return element != null ? type.valueFromJson(element) : null; }).toList(), + null => [], _ => throw Exception('Invalid value'), }; }
374-383: Consider validating address format in valueFromJson.While the implementation works, it would be safer to validate the address format.
String valueFromJson(json) { return switch (json) { - String str => str, + String str when str.startsWith('0x') && str.length == 42 => str, _ => throw Exception('Invalid value'), }; }
399-409: Fix typo in variable name.There's a typo in the variable name 'strinbool'.
bool valueFromJson(json) { return switch (json) { - String strinbool => bool.parse(strinbool), + String stringBool => bool.parse(stringBool), bool b => b, _ => throw Exception('Invalid value'), }; }lib/src/crypto/evm/entities/abi/gameItemsContract.dart (2)
1-490: Consider adding documentation for the contract interface.The contract implementation looks good, but adding documentation would help developers understand:
- The purpose and usage of each game item (GOLD, SHIELD, etc.)
- The meaning of error conditions
- The expected behavior of events
+/// GameItems is an ERC1155 contract that manages various in-game items +/// Each item is represented by a unique token ID and can be owned in multiple quantities final gameItemsContract = ContractABI.fromAbi('''
230-294: Consider using an enum for token IDs.The contract defines multiple token types (GOLD, SHIELD, etc.) that could benefit from being defined as an enum for type safety.
/// Defines the available game items and their corresponding token IDs enum GameItem { GOLD, SHIELD, SILVER, SWORD, THORS_HAMMER; BigInt get tokenId => BigInt.from(index); }lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (1)
174-308: Consider extracting common JSON field validation.The implementation handles two similar JSON structures with a lot of duplicate field validation. Consider extracting the common validation logic.
bool _hasRequiredFields(Json json, List<String> fields) { return fields.every((field) => json.containsKey(field)); } final requiredFields = [ 'blockNumber', 'timeStamp', 'hash', 'from', 'to', 'gas', 'gasUsed', 'gasPrice', 'contractAddress', 'tokenID', ];lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (1)
149-163: Consider caching URI results.The URI for a token ID is typically static. Consider caching the results to reduce RPC calls.
+ final _uriCache = <String, Map<BigInt, String>>{}; + Future<String> fetchERC1155UriOfToken({ required BigInt tokenID, required String contractAddress, }) async { + if (_uriCache[contractAddress]?[tokenID] != null) { + return _uriCache[contractAddress]![tokenID]!; + } final erc1155Contract = ERC1155Contract( contractAddress: contractAddress, rpc: this, ); final uri = await erc1155Contract.getUri( tokenID: tokenID, ); + _uriCache[contractAddress] ??= {}; + _uriCache[contractAddress]![tokenID] = uri; return uri; }lib/src/crypto/evm/entities/raw_transaction/raw_evm_transaction.dart (1)
670-686: Refactor duplicated access list handling code.The access list handling logic is duplicated across multiple methods. Consider extracting it into a shared utility method to improve maintainability and reduce code duplication.
Create a new utility method:
List<AccessListItem> _parseAccessList(RLPValue value) { return switch (value) { RLPList list => list.value .whereType<RLPList>() .map((item) { final subList = item[1]; if (subList is RLPList) return ( address: "0x" + item[0].buffer.toHex, storageKeys: subList.value.map((key) => key.buffer.toHex).toList(), ); return null; }) .nonNulls .toList(), _ => [], }; }Then use it in all places:
- accessList: switch (rlpDecoded[8]) { - RLPList list => list.value - .whereType<RLPList>() - .map((item) { - final subList = item[1]; - if (subList is RLPList) - return ( - address: "0x" + item[0].buffer.toHex, - storageKeys: - subList.value.map((key) => key.buffer.toHex).toList(), - ); - return null; - }) - .nonNulls - .toList(), - _ => [], - }, + accessList: _parseAccessList(rlpDecoded[8]),Also applies to: 714-730, 784-796, 829-841
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/dart.yml(4 hunks).gitmodules(0 hunks)lib/src/crypto/evm/entities/abi/erc/erc1155.dart(1 hunks)lib/src/crypto/evm/entities/abi/gameItemsContract.dart(1 hunks)lib/src/crypto/evm/entities/contract/contract_function_param.dart(1 hunks)lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart(2 hunks)lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart(3 hunks)lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart(8 hunks)lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart(1 hunks)lib/src/crypto/evm/entities/raw_transaction/raw_evm_transaction.dart(15 hunks)lib/src/crypto/evm/entities/raw_transaction/signature.dart(1 hunks)lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart(1 hunks)lib/src/crypto/evm/evm.dart(1 hunks)lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart(3 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(2 hunks)lib/src/crypto/evm/utils/rlp.dart(1 hunks)lib/src/crypto/tron/entities/tron_parameter_type.dart(1 hunks)lib/src/crypto/tron/entities/tron_transaction_utils.dart(3 hunks)lib/src/crypto/wallet_utils.dart(4 hunks)lib/src/domain/entities/coin_entity.dart(6 hunks)lib/src/domain/entities/fee.dart(1 hunks)lib/src/domain/entities/token_info.dart(1 hunks)lib/src/domain/extensions.dart(1 hunks)lib/src/utils/bigint_utils.dart(0 hunks)lib/src/utils/var_uint.dart(1 hunks)lib/src/wallet/bip39/bip39.dart(1 hunks)lib/src/wallet/bip39/pbkdf2.dart(1 hunks)lib/src/wallet/bip39/wordlist/combined.dart(1 hunks)lib/src/wallet/bip39/wordlist/en.dart(1 hunks)lib/src/wallet/bip39/wordlist/es.dart(1 hunks)lib/src/wallet/bip39/wordlist/fr.dart(1 hunks)lib/src/wallet/bip39/wordlist/it.dart(1 hunks)lib/src/wallet/bip39/wordlist/ja.dart(1 hunks)lib/src/wallet/bip39/wordlist/ko.dart(1 hunks)lib/src/wallet/bip39/wordlist/zhHans.dart(1 hunks)
💤 Files with no reviewable changes (2)
- .gitmodules
- lib/src/utils/bigint_utils.dart
✅ Files skipped from review due to trivial changes (5)
- lib/src/wallet/bip39/wordlist/en.dart
- lib/src/wallet/bip39/wordlist/fr.dart
- lib/src/wallet/bip39/wordlist/zhHans.dart
- lib/src/wallet/bip39/wordlist/ja.dart
- lib/src/wallet/bip39/wordlist/ko.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/dart.yml
- lib/src/crypto/tron/entities/tron_transaction_utils.dart
🔇 Additional comments (51)
lib/src/wallet/bip39/bip39.dart (5)
4-4: Question the usage ofdart:iofor randomness.
Usingio.pidin the random seed mix might have limited entropy on some platforms. Consider whether relying solely on high-quality cryptographic randomness fromRandom.secure()is more robust.
17-20: Consider rename or clarification of_SIZE_BYTE.
_SIZE_BYTE = 255suggests maximum byte value is 254. Typically, for random bytes, we want0xFF(255) to be valid. Verify if subtracting 1 from the range is intended or a potential off-by-one.
95-116: Potential overhead with continuous_stopWatchusage.
You start_stopWatchonce and reuse the microseconds, which is helpful for ephemeral entropy. Confirm whether the overhead is acceptable compared to a more direct cryptographic approach.
162-165: Ensurepassphraseis properly handled and salted.
The PBKDF2 usage is standard for generating seeds. Just confirm that the passphrase logic matches BIP39 spec, especially for the salt ("mnemonic" + passphrase).
208-251: Confirm correctness of the checksum logic.
BIP39 logic requires correct splitting of bits for ENT and CS. The code looks aligned with the spec, but a subtle off-by-one can occur. Ensure thorough testing of edge cases (e.g., 128 and 256 bit entropy).lib/src/crypto/evm/utils/rlp.dart (6)
6-14: Excellent use of a custom exception class.
RLPExceptionprovides contextual error messages for RLP issues. This improves debugging and error handling.
70-74: Prefer consistent hex handling.
You conditionally treat strings starting with0x. Confirm that the absence ofisHexmight yield unexpected conversions if the string is not hex.
122-134: Validate or handle empty RLPLists.
RLPList.fromBufferaccumulates items until the parsed length is reached. Confirm that empty lists or length mismatches are separately tested to avoid partial decodes.
138-176: Encoding logic is clear but watch for corner cases.
The code handles short vs. long data well, but be mindful of integer overflow or negative length in arrayify. Additional tests for data sizes near 56 bytes are recommended.
[approve]
179-184: Perform truncated input checks.
decodeRLPCheckensures input is fully consumed. This strengthens correctness by forbidding trailing garbage. Great approach!
187-290: Nested switch arms are comprehensive.
Each RLP prefix range is handled properly. Confirm that 0xc0, 0xf8, etc. match official RLP specs. Minor risk of off-by-one if spec changes or if extended.lib/src/wallet/bip39/wordlist/combined.dart (1)
10-19: Centralized multi-language word list is excellent.
Combining all language wordlists into one structure improves flexibility. Just ensure capacity for possible new languages or subsets.lib/src/wallet/bip39/pbkdf2.dart (1)
9-23: LGTM! Well-structured class with proper initialization.The class is well-designed with:
- Appropriate default values following BIP39 specification
- Proper initialization of PBKDF2KeyDerivator with HMAC-SHA512
lib/src/wallet/bip39/wordlist/es.dart (1)
1-2050: LGTM! Comprehensive Spanish BIP39 wordlist.The wordlist is correctly implemented with:
- Exactly 2048 words as required by BIP39
- Proper handling of Spanish characters and accents
- Unique and well-formatted entries
lib/src/wallet/bip39/wordlist/it.dart (1)
1-2050: LGTM! Comprehensive French BIP39 wordlist.The wordlist is correctly implemented with:
- Exactly 2048 words as required by BIP39
- Proper handling of French characters and accents
- Unique and well-formatted entries
lib/src/crypto/wallet_utils.dart (2)
114-114: LGTM! Proper migration to new mnemonic handling.The change correctly uses the new
mnemonicToSeedimplementation.
126-131: LGTM! Clean migration from bip32 to HDNode.The changes properly migrate from the external bip32 library to the custom HDNode implementation while maintaining the same functionality.
Also applies to: 135-140
lib/src/crypto/tron/entities/tron_parameter_type.dart (2)
51-51: Verify endianness handling in toUBigInt extension.While the change to
toUBigIntmaintains consistency with unsigned integer handling, we need to verify that its default endianness matches the previous explicit big-endian setting (littleEndian: false). Incorrect endianness can lead to value misinterpretation in blockchain interactions.✅ Verification successful
The endianness handling in toUBigInt is verified as correct.
The implementation processes bytes from left to right in base-256, which is mathematically equivalent to the previous big-endian specification. This maintains consistent byte order interpretation across all blockchain-related operations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the toUBigInt extension implementation to verify endianness rg -A 5 'extension.*on.*Uint8List.*{.*toUBigInt' || rg -A 5 'toUBigInt' # Search for other usages to ensure consistent endianness handling rg -B 2 -A 2 'toUBigInt'Length of output: 19072
48-48: LGTM! Verify consistent usage of toBytesUnsigned across the codebase.The change to
toBytesUnsignedis appropriate for TRON parameter encoding and aligns with blockchain conventions for unsigned integers.Let's verify consistent usage across the codebase:
✅ Verification successful
Implementation verified and consistent across codebase
The change to
toBytesUnsignedis correctly implemented using PointyCastle'sencodeBigIntAsUnsignedand is consistently used across all blockchain-related code. The remainingbigIntToBytesreferences are only in comments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of bigIntToBytes to ensure consistent conversion method usage rg 'bigIntToBytes' # Search for toBytesUnsigned usage to verify the pattern rg 'toBytesUnsigned'Length of output: 914
Script:
#!/bin/bash # Check the implementation of toBytesUnsigned in extensions cat lib/src/domain/extensions.dartLength of output: 4911
lib/src/crypto/evm/entities/raw_transaction/signature.dart (1)
27-28: Verify cryptographic compatibility of the new byte conversion method.The change from
toBigInt(littleEndian: false)totoUBigIntaffects how the signature components (r,s) are parsed. While this change simplifies the code, we need to ensure it maintains compatibility with EVM specifications for ECDSA signatures.Let's verify the compatibility:
Additionally, please ensure:
- The new implementation maintains compatibility with externally generated signatures
- Add test cases specifically for parsing signatures with high-bit set in the first byte
- Add test vectors from Ethereum test suites if available
✅ Verification successful
The signature byte conversion change is cryptographically sound and properly tested.
The
toUBigIntimplementation correctly maintains big-endian byte order, which is the standard for EVM signatures. This is verified by:
- Consistent usage across all EVM numeric parsing (nonce, gas, signatures)
- Successful test cases with real transaction data
- Proper signature serialization/deserialization verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases that verify signature parsing rg -l "test.*signature.*fromBytes|test.*ECDSA" --type dart # Search for any existing implementations to compare against ast-grep --pattern 'factory $_(Signature|ECDSA) { $$$ fromBytes($_) { $$$ } }'Length of output: 151
Script:
#!/bin/bash # Search for signature-related files echo "=== Signature-related files ===" fd -e dart signature # Search for toUBigInt extension definition echo -e "\n=== toUBigInt extension definition ===" rg "extension.*on.*List<int>" --type dart -A 5 # Search for signature tests with broader pattern echo -e "\n=== Signature test files ===" fd -e dart test | rg -l "signature" # Look for actual usage of fromBytes echo -e "\n=== Usage of fromBytes ===" rg "fromBytes" --type dart -A 2Length of output: 2194
Script:
#!/bin/bash # Find toUBigInt implementation echo "=== toUBigInt implementation ===" rg "toUBigInt" --type dart -B 2 -A 2 # Check signature test content echo -e "\n=== Signature test content ===" cat test/ci/evm/signing/signature_test.dart # Look for signature validation echo -e "\n=== Signature validation code ===" rg "verify|isValid|validate.*signature" --type dart -A 3Length of output: 15107
lib/src/domain/entities/coin_entity.dart (7)
29-30: Expanding coin entity to handle ERC-1155 tokens.This is consistent with the approach used for
asEthBasedandisERC20. Implementation looks good.Also applies to: 34-34
69-76: Ensure tokenId persists as a string in JSON.Utilizing
tokenId.toString()is consistent with howBigIntis usually serialized. Great job ensuring alignment with the correspondingfromJsonmethod.
152-152: Optional allowDeletion property for EvmCoinEntity.If this property is intended to be always defined, consider making it non-nullable. Otherwise, it looks consistent with existing code usage.
167-167: Clarify reason behind excluding ERC1155Entity in EvmCoinEntity equality check.Ensure this is the intended behavior, so that ERC1155Entity is handled separately from base EvmCoinEntity types.
179-193: copyWith method is well-structured.Implementation is consistent with existing code patterns. Nice work.
196-257: Implementation of ERC1155Entity is consistent with existing patterns.Using
BigIntfortokenIdis appropriate, and implementing equality/hashCode parallels other ERC entity classes. Nicely done.
269-269: AllowDeletion field properly forwarded in ERC20Entity constructor.Forwarding the
allowDeletionparameter maintains consistency throughout entity classes.lib/src/domain/entities/token_info.dart (1)
10-10: New optional 'id' field for TokenInfo.Providing an optional identifier is a helpful enhancement. If relevant, consider adding serialization logic to maintain consistency in JSON representation.
Also applies to: 18-18
lib/src/domain/extensions.dart (1)
17-20: LGTM! Clear and safe implementation.The getter correctly handles unsigned BigInt to bytes conversion with appropriate validation.
lib/src/crypto/evm/entities/abi/erc/erc1155.dart (3)
1-4: Imports look good.
Thedart:typed_dataandwalletkit_dartimports appear valid and minimal.
341-353: Length check for arrays is explicitly handled – good!
TheArgumentErroris clear and prevents mismatched input arrays for batch queries. The logic to handle multiple accounts and token IDs looks correct.
5-319: Consider externalizing large ABI definitions.
Keeping the entire ABI inline is convenient, but it can reduce readability and maintainability. Placing the ABI in a separate JSON file or a dedicated resource might make version updates, automated validations, and code reviews more straightforward.Feel free to run a command to confirm usage references or changes to this ABI. For instance:
lib/src/crypto/evm/evm.dart (1)
31-35: Exports are organized and consistent.
Exportingerc1155.dartin line with other ERC contract classes is a coherent addition. No particular issues here.lib/src/crypto/evm/entities/contract/contract_function_param.dart (1)
80-90: Type-based serialization looks correct.
Delegating serialization and deserialization totype.valueToJsonandtype.valueFromJsonensures well-defined, type-safe transformations. Review any edge cases (e.g., extremely large BigInts, empty strings) to confirm correct handling.To see if any boundary conditions are untested, try searching the test suite for coverage:
✅ Verification successful
Type-based serialization implementation is robust and safe
The implementation uses a sealed class hierarchy with comprehensive type-safe handling for all parameter types including edge cases (BigInt, bytes, nested types). Each type has proper validation and conversion logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -A 5 "FunctionParamWithValue.*fromJson" --type dartLength of output: 1267
Script:
#!/bin/bash # Search for test files and implementations echo "=== Searching for test files ===" rg -l "FunctionParamWithValue" --type dart "*_test.dart" echo -e "\n=== Examining FunctionParamType implementation ===" ast-grep --pattern 'class FunctionParamType { $$$ valueToJson($_) { $$$ } $$$ valueFromJson($_) { $$$ } $$$ }'Length of output: 435
Script:
#!/bin/bash # Find test files correctly echo "=== Finding test files ===" fd -e dart test echo -e "\n=== Searching test files for FunctionParamWithValue ===" fd -e dart test -x rg -l "FunctionParamWithValue" echo -e "\n=== Looking for FunctionParamType implementation ===" rg -A 10 "class FunctionParamType" --type dart echo -e "\n=== Finding valueToJson/valueFromJson usage ===" rg "(valueToJson|valueFromJson)" --type dartLength of output: 8433
lib/src/crypto/evm/entities/contract/parameter_type/bytes_parameter_type.dart (2)
24-31: LGTM! Clean implementation of JSON serialization.The implementation correctly handles hex conversion and includes proper error handling for invalid JSON types.
52-59: LGTM! Consistent implementation with FunctionParamBytes.The JSON serialization logic matches the implementation in
FunctionParamBytes, maintaining consistency across the codebase.lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart (2)
33-40: LGTM! Clean implementation of JSON serialization.The implementation correctly handles BigInt conversion and includes proper error handling for invalid JSON types.
31-31: Verify the change in decoding method.The switch from
toBigInt(littleEndian: false)totoUBigIntmight affect the handling of signed integers.Let's verify the behavior:
✅ Verification successful
The change to
toUBigIntis correct and aligns with EVM's integer handlingThe implementation is safe because:
- The encode method already enforces non-negative values and proper size limits
- EVM uses big-endian encoding for all integers
- The type information (signed vs unsigned) is handled by the parameter type name, not the byte representation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any tests or usages that might be affected by this change rg -l "toBigInt.*littleEndian.*false" . ast-grep --pattern 'decode($data) { $$$toBigInt$$$}'Length of output: 96
Script:
#!/bin/bash # Get the content of the file echo "=== Int Parameter Type Implementation ===" cat lib/src/crypto/evm/entities/contract/parameter_type/int_parameter_type.dart echo -e "\n=== Related Test Files ===" fd "int.*test" --type f echo -e "\n=== Similar Parameter Types ===" fd ".*parameter_type.dart" --type f echo -e "\n=== Usage of IntParameterType ===" rg "IntParameterType" -A 2 -B 2Length of output: 9317
lib/src/crypto/evm/entities/contract/parameter_type/fixed_parameter_type.dart (1)
Line range hint
11-17: Consider implementing encode/decode methods.The base class has unimplemented
encodeanddecodemethods. Consider providing a default implementation or documenting why these methods are left unimplemented.Let's check if these methods are implemented in derived classes:
lib/src/crypto/evm/repositories/etherscan/etherscan_explorer.dart (3)
74-89: LGTM! Consistent with other endpoint builders.The implementation follows the established pattern of other transaction endpoint builders.
121-147: Verify ERC1155 transaction parsing.The implementation looks good but relies on a new
fromJsonErc1155parser.Let's verify the parser implementation:
✅ Verification successful
ERC1155 transaction parsing implementation verified successfully.
The parser correctly handles both ERC1155 transaction formats, properly manages token metadata, and includes all necessary transaction details with appropriate error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the ERC1155 transaction parser implementation ast-grep --pattern 'fromJsonErc1155($json, {$$$}) { $$$ }'Length of output: 62
Script:
#!/bin/bash # Search for EtherscanTransaction class and fromJsonErc1155 constructor echo "=== Searching for EtherscanTransaction class ===" rg "class EtherscanTransaction" -A 5 echo -e "\n=== Searching for fromJsonErc1155 constructor ===" rg "fromJsonErc1155" -A 10 echo -e "\n=== Searching for related test files ===" fd "etherscan.*test\.dart$"Length of output: 3056
Script:
#!/bin/bash # Get the complete implementation of EtherscanTransaction class echo "=== Complete EtherscanTransaction implementation ===" cat lib/src/crypto/evm/entities/transactions/etherscan_transaction.dartLength of output: 13481
358-374: Verify ZeniqScan's ERC1155 implementation.The ZeniqScan implementation uses a different endpoint (
tokentxinstead oftoken1155tx) compared to the base class. This might affect ERC1155 transaction fetching.Let's verify if this is intentional:
✅ Verification successful
ZeniqScan's ERC1155 implementation is working as intended
The test suite confirms that ZeniqScan's ERC1155 transaction fetching is working correctly with the current 'tokentx' endpoint. This is validated by the passing test case 'test erc1155 fetching zsc' which successfully retrieves non-empty ERC1155 transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation or tests related to ZeniqScan's ERC1155 support rg -l "ZeniqScan.*ERC1155" . rg -l "token1155tx.*ZeniqScan" .Length of output: 65
Script:
#!/bin/bash # Find base class implementation and ERC1155 related code ast-grep --pattern 'abstract class $_ { $$$ buildERC1155TransactionEndpoint($$$) { $$$ } $$$ }' # Search for ERC1155 related tests and implementations rg -l "ERC1155.*transaction" . # Search for tokentx endpoint usage rg "tokentx.*address" .Length of output: 701
Script:
#!/bin/bash # Check test file content cat ./test/ci/evm/evm_explorer_test.dart # Look for different token transaction endpoint implementations rg -A 5 "buildERC.*Transaction" .Length of output: 16132
lib/src/crypto/evm/entities/contract/parameter_type/function_parameter_type.dart (3)
166-168: LGTM! Good addition of JSON serialization methods.The abstract methods provide a clear contract for JSON serialization/deserialization that all parameter types must implement.
320-330: LGTM! Array type JSON handling is well implemented.The implementation correctly handles array serialization and deserialization with proper error handling.
427-436: LGTM! String parameter JSON handling is well implemented.The implementation correctly handles string serialization and deserialization.
lib/src/crypto/evm/entities/transactions/etherscan_transaction.dart (1)
179-239: LGTM! Good use of helper function for transaction creation.The
_createTransactionhelper function effectively reduces code duplication and encapsulates the transaction creation logic.lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (1)
108-123: LGTM! Well-implemented balance fetching for ERC1155 tokens.The implementation correctly handles balance fetching with proper parameter validation.
lib/src/crypto/evm/entities/raw_transaction/raw_evm_transaction.dart (4)
71-71: LGTM! Method name correction.The method name change from
fromUnsignedHexingtofromUnsignedHeximproves clarity by fixing the typo.
141-155: LGTM! Improved RLP encoding with type safety.The serialization logic now uses strongly typed RLP classes (
RLPList,RLPBigInt, etc.) which improves type safety and maintainability.
212-228: LGTM! Enhanced RLP decoding with better error handling.The RLP decoding improvements include:
- Proper type checking with
is! RLPList- More descriptive error messages
- Better field validation
Also applies to: 243-262
Line range hint
1-853: Verify transaction compatibility with different clients.The RLP encoding changes are significant. Please ensure compatibility with different Ethereum clients by:
- Testing transaction creation and signing with different client implementations
- Verifying that signed transactions can be decoded correctly
- Testing with different chain IDs
Run the following script to find test files and verify coverage:
| String mnemonicToEntropy(mnemonic) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, ENWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, ESWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, FRWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, ITWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, JAWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, KOWORDS); | ||
| } catch (e) { | ||
| try { | ||
| return _mnemonicToEntropy(mnemonic, ZHHANSWORDS); | ||
| } catch (e) { | ||
| return _mnemonicToEntropy(mnemonic, ZHHANTWORDS); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple nested try-catch blocks can hamper maintainability.
Looping through language wordlists is correct, but deeply nested try-catches can hide precise error contexts. Consider refactoring to a single loop over your wordlists to simplify the logic.
lib/src/utils/var_uint.dart
Outdated
| int readVarIntFromLength( | ||
| int offset, | ||
| int length, | ||
| Endian endian, | ||
| ) { | ||
| if (length < 0xfd) { | ||
| return getUint8(offset); | ||
| } | ||
| if (length == 0xfd) { | ||
| return getUint16(offset, endian); | ||
| } | ||
| if (length == 0xfe) { | ||
| return getUint32(offset, endian); | ||
| } | ||
| return getUint64(offset, endian); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align method signature with existing patterns.
The new method has several inconsistencies with the established patterns in this file:
- Return type: Other read methods return a tuple of (value, bytesRead)
- Endianness: Other methods consistently use
Endian.little - Missing documentation
Consider this implementation:
- int readVarIntFromLength(
+ /// Reads a variable-length integer from the specified offset based on the length parameter.
+ /// Returns a tuple of (value, bytesRead) to match the pattern of other read methods.
+ (int, int) readVarIntFromLength(
int offset,
int length,
- Endian endian,
) {
+ if (offset < 0) {
+ throw ArgumentError('Offset must be non-negative');
+ }
+
if (length < 0xfd) {
- return getUint8(offset);
+ return (getUint8(offset), 1);
}
if (length == 0xfd) {
- return getUint16(offset, endian);
+ return (getUint16(offset, Endian.little), 2);
}
if (length == 0xfe) {
- return getUint32(offset, endian);
+ return (getUint32(offset, Endian.little), 4);
}
- return getUint64(offset, endian);
+ return (getUint64(offset, Endian.little), 8);
}📝 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.
| int readVarIntFromLength( | |
| int offset, | |
| int length, | |
| Endian endian, | |
| ) { | |
| if (length < 0xfd) { | |
| return getUint8(offset); | |
| } | |
| if (length == 0xfd) { | |
| return getUint16(offset, endian); | |
| } | |
| if (length == 0xfe) { | |
| return getUint32(offset, endian); | |
| } | |
| return getUint64(offset, endian); | |
| } | |
| /// Reads a variable-length integer from the specified offset based on the length parameter. | |
| /// Returns a tuple of (value, bytesRead) to match the pattern of other read methods. | |
| (int, int) readVarIntFromLength( | |
| int offset, | |
| int length, | |
| ) { | |
| if (offset < 0) { | |
| throw ArgumentError('Offset must be non-negative'); | |
| } | |
| if (length < 0xfd) { | |
| return (getUint8(offset), 1); | |
| } | |
| if (length == 0xfd) { | |
| return (getUint16(offset, Endian.little), 2); | |
| } | |
| if (length == 0xfe) { | |
| return (getUint32(offset, Endian.little), 4); | |
| } | |
| return (getUint64(offset, Endian.little), 8); | |
| } |
💡 Codebase verification
Method implementation is inconsistent with codebase patterns
The readVarIntFromLength method has several inconsistencies with established patterns:
- Other similar methods return
(int, int)tuples including bytes read - All existing integer reading operations use
Endian.little - The method is currently unused in the codebase
The method should be revised to:
- Return a tuple of
(int, int)like other methods - Use
Endian.littleconsistently instead of a parameter - Include documentation explaining its purpose
🔗 Analysis chain
Verify the usage of this method in the codebase.
Let's verify how this method is being used to understand if the endianness parameter is required for compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of readVarIntFromLength
rg "readVarIntFromLength" --type dart
Length of output: 93
Script:
#!/bin/bash
# Search for similar methods and patterns
echo "=== Similar methods ==="
rg "read.*Int|get.*Int" --type dart -A 2 lib/src/utils/var_uint.dart
echo -e "\n=== Check file history ==="
git log -p lib/src/utils/var_uint.dart | head -n 50
Length of output: 581
| required super.symbol, | ||
| required super.decimals, | ||
| required this.chainID, | ||
| this.allowDeletion = false, |
There was a problem hiding this comment.
Inconsistent default for allowDeletion.
The constructor defaults allowDeletion to false, but JSON parsing defaults it to true. This discrepancy can lead to unexpected behavior.
Apply this patch or ensure the defaults are consistent:
- this.allowDeletion = false,
+ this.allowDeletion = true,📝 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.
| this.allowDeletion = false, | |
| this.allowDeletion = true, |
|
|
||
| extension IntListUtil on List<int> { | ||
| BigInt get toBigInt => BigInt.parse(hex.encode(this), radix: 16); | ||
| // BigInt get toBigInt => BigInt.parse(hex.encode(this), radix: 16); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
Instead of keeping commented code, it should be removed as it can be retrieved from version control if needed. Additionally, the implementation being removed was inefficient as it used unnecessary hex encoding.
- // BigInt get toBigInt => BigInt.parse(hex.encode(this), radix: 16);| /// Decode a unsigned big-endian integer from the buffer. | ||
| int get toUInt { | ||
| int result = 0; | ||
| for (var i = 0; i < length; i++) { | ||
| result = result * 256 + this[i]; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add buffer size validation to prevent potential overflow
The codebase shows a consistent pattern of defensive programming with size validations. While current usage of toUInt in EVM transaction processing is safe (dealing with small integers like chain IDs), adding a size check would prevent potential misuse in other contexts:
int get toUInt {
+ if (length > 8) {
+ throw ArgumentError('Buffer too large: max 8 bytes for int');
+ }
int result = 0;
for (var i = 0; i < length; i++) {
result = result * 256 + this[i];
}
return result;
}🔗 Analysis chain
Add buffer size validation to prevent overflow.
The implementation correctly decodes unsigned big-endian integers, but it could overflow for buffers larger than 8 bytes (maximum size for Dart's int).
Consider adding a size check:
int get toUInt {
+ if (length > 8) {
+ throw ArgumentError('Buffer too large: max 8 bytes for int');
+ }
int result = 0;
for (var i = 0; i < length; i++) {
result = result * 256 + this[i];
}
return result;
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for existing size validations
echo "=== Searching for size validations ==="
rg "length\s*>[^=]" -A 2 -B 2
echo -e "\n=== Looking for buffer-related tests ==="
fd "test.*\.dart$" --exec rg -l "toUInt"
echo -e "\n=== Finding usages of toUInt ==="
rg "toUInt" -A 2 -B 2
echo -e "\n=== Looking for related extensions or utilities ==="
ast-grep --pattern 'extension $_ on List<int> {
$$$
}'
Length of output: 9364
| @override | ||
| valueToJson(ABIFunction value) { | ||
| throw UnimplementedError(); | ||
| } | ||
|
|
||
| @override | ||
| ABIFunction valueFromJson(json) { | ||
| throw UnimplementedError(); | ||
| } |
There was a problem hiding this comment.
Implement the unimplemented methods for FunctionParamFunction.
The methods are currently throwing UnimplementedError. Consider implementing them to handle function parameter serialization.
Would you like me to help implement these methods for handling function parameter serialization?
| Future<String> sendERC1155Token({ | ||
| required TransferIntent<EvmFeeInformation> intent, | ||
| required String contractAddress, | ||
| required BigInt tokenID, | ||
| required String from, | ||
| required Uint8List seed, | ||
| }) async { | ||
| final erc1155Contract = ERC1155Contract( | ||
| contractAddress: contractAddress, | ||
| rpc: this, | ||
| ); | ||
|
|
||
| return erc1155Contract.safeTransferFrom( | ||
| sender: from, | ||
| to: intent.recipient, | ||
| tokenID: tokenID, | ||
| amount: intent.amount.value, | ||
| seed: seed, | ||
| feeInfo: intent.feeInfo, | ||
| accessList: intent.accessList, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for token ownership before transfer.
Consider checking the sender's balance before attempting the transfer to provide a better error message.
Future<String> sendERC1155Token({
required TransferIntent<EvmFeeInformation> intent,
required String contractAddress,
required BigInt tokenID,
required String from,
required Uint8List seed,
}) async {
final erc1155Contract = ERC1155Contract(
contractAddress: contractAddress,
rpc: this,
);
+ final balance = await fetchERC1155BalanceOfToken(
+ address: from,
+ tokenID: tokenID,
+ contractAddress: contractAddress,
+ );
+ if (balance.value < intent.amount.value) {
+ throw InsufficientBalanceException(
+ 'Insufficient balance for token $tokenID',
+ );
+ }
return erc1155Contract.safeTransferFrom(
sender: from,
to: intent.recipient,
tokenID: tokenID,
amount: intent.amount.value,
seed: seed,
feeInfo: intent.feeInfo,
accessList: intent.accessList,
);
}Committable suggestion skipped: line range outside the PR's diff.
| Future<List<BigInt>> fetchERC1155BatchBalanceOfTokens({ | ||
| required List<String> accounts, | ||
| required List<BigInt> tokenIDs, | ||
| required String contractAddress, | ||
| }) async { | ||
| final erc1155Contract = ERC1155Contract( | ||
| contractAddress: contractAddress, | ||
| rpc: this, | ||
| ); | ||
|
|
||
| final balances = await erc1155Contract.balanceOfBatch( | ||
| accounts: accounts, | ||
| tokenIDs: tokenIDs, | ||
| ); | ||
|
|
||
| return balances; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for array lengths.
The batch balance fetching should validate that the accounts and tokenIDs arrays have the same length.
Future<List<BigInt>> fetchERC1155BatchBalanceOfTokens({
required List<String> accounts,
required List<BigInt> tokenIDs,
required String contractAddress,
}) async {
+ if (accounts.length != tokenIDs.length) {
+ throw ArgumentError('accounts and tokenIDs must have the same length');
+ }
final erc1155Contract = ERC1155Contract(
contractAddress: contractAddress,
rpc: this,
);📝 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.
| Future<List<BigInt>> fetchERC1155BatchBalanceOfTokens({ | |
| required List<String> accounts, | |
| required List<BigInt> tokenIDs, | |
| required String contractAddress, | |
| }) async { | |
| final erc1155Contract = ERC1155Contract( | |
| contractAddress: contractAddress, | |
| rpc: this, | |
| ); | |
| final balances = await erc1155Contract.balanceOfBatch( | |
| accounts: accounts, | |
| tokenIDs: tokenIDs, | |
| ); | |
| return balances; | |
| } | |
| Future<List<BigInt>> fetchERC1155BatchBalanceOfTokens({ | |
| required List<String> accounts, | |
| required List<BigInt> tokenIDs, | |
| required String contractAddress, | |
| }) async { | |
| if (accounts.length != tokenIDs.length) { | |
| throw ArgumentError('accounts and tokenIDs must have the same length'); | |
| } | |
| final erc1155Contract = ERC1155Contract( | |
| contractAddress: contractAddress, | |
| rpc: this, | |
| ); | |
| final balances = await erc1155Contract.balanceOfBatch( | |
| accounts: accounts, | |
| tokenIDs: tokenIDs, | |
| ); | |
| return balances; | |
| } |
| if (rawTxHex.startsWith("01") == false) { | ||
| throw Exception("Invalid Type 1 Transaction"); |
There was a problem hiding this comment.
Fix incorrect error message in Type 1 transaction validation.
The error message states "Invalid Type 1 Transaction" but is used in the Type 2 transaction class. This could be confusing for debugging.
Apply this diff to fix the error message:
- if (rawTxHex.startsWith("01") == false) {
- throw Exception("Invalid Type 1 Transaction");
+ if (rawTxHex.startsWith("01") == false) {
+ throw Exception("Invalid Type 2 Transaction");📝 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.
| if (rawTxHex.startsWith("01") == false) { | |
| throw Exception("Invalid Type 1 Transaction"); | |
| if (rawTxHex.startsWith("01") == false) { | |
| throw Exception("Invalid Type 2 Transaction"); |
…o wallet_interface
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
test/ci/bip39/bip39_test.dart (5)
10-10: Consider implementing the mentioned future test cases or removing the comment.This TODO comment suggests additional test cases for multiple languages but doesn't provide a concrete action item or timeline. Either implement these additional tests now or create a specific ticket/issue to track this enhancement.
23-40: Remove commented-out test code.This commented-out test appears to be a statistical test for finding a mnemonic ending with "arm". Since this is a new file, it's better to either implement the test properly or remove it entirely rather than leaving commented-out code that might confuse other developers.
- // test( - // "Generate Mnemonic with word arm", - // () { - // int tries = 0; - // bool found = false; - // for (; tries < 20000; tries++) { - // final m = generateMnemonic(); - // if (m.endsWith(" arm")) { - // print( - // "After $tries tries: Found random mnemonic that ends with arm: $m", - // ); - // found = true; - // break; - // } - // } - // expect(found, true); - // }, - // );
90-96: Document the replaceUnicode() extension method.The test relies on a
replaceUnicode()extension method that's not defined in this file. Consider adding a brief comment explaining what this method does and where it's defined for better code readability.
146-169: Simplify exception testing using Dart's idiomatic approach.Instead of using try/catch to test exceptions, consider using Dart's built-in
expect(..., throwsA(...))pattern which is more idiomatic and concise.test('throws for empty entropy', () { - try { - expect(entropyToMnemonic(''), throwsArgumentError); - } catch (err) { - expect((err as ArgumentError).message, "Invalid entropy"); - } + expect(() => entropyToMnemonic(''), + throwsA(isA<ArgumentError>().having((e) => e.message, 'message', 'Invalid entropy'))); }); test('throws for entropy that\'s not a multitude of 4 bytes', () { - try { - expect(entropyToMnemonic('000000'), throwsArgumentError); - } catch (err) { - expect((err as ArgumentError).message, "Invalid entropy"); - } + expect(() => entropyToMnemonic('000000'), + throwsA(isA<ArgumentError>().having((e) => e.message, 'message', 'Invalid entropy'))); }); test('throws for entropy that is larger than 1024', () { - try { - expect(entropyToMnemonic(Uint8List(1028 + 1).join('00')), - throwsArgumentError); - } catch (err) { - expect((err as ArgumentError).message, "Invalid entropy"); - } + expect(() => entropyToMnemonic(Uint8List(1028 + 1).join('00')), + throwsA(isA<ArgumentError>().having((e) => e.message, 'message', 'Invalid entropy'))); });
231-234: Fix typo in test description.There's a typo in the test description: "mnemoic" should be "mnemonic".
- test('mnemoic to entropy', () { + test('mnemonic to entropy', () { final String entropy = mnemonicToEntropy(vmnemonic); expect(entropy, equals(ventropy)); });lib/src/crypto/utxo/entities/raw_transaction/input.dart (2)
33-34: Consider clarifying the sentinel return value.Returning
-1.toBIwhen_scriptSigor_prevScriptPubKeyis null may confuse callers about the semantic meaning of a negative weight. Consider returningBigInt.zeroor throwing an exception if a valid weight cannot be calculated.
102-103: Rename_wittnessScriptto_witnessScriptfor consistency.“Wittness” appears to be a typographical error. Consider renaming it consistently throughout the codebase for clarity.
lib/src/wallet/hd_node.dart (1)
17-42: Private and public key fields with robust getters.Storing private and public keys in
_pand_qrespectively is straightforward. Elevated usage of read-only getters (e.g.,publicKey,identifier) correctly encapsulates data.Consider encrypting or obfuscating private keys in memory for additional security if this code will handle sensitive funds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/dart.yml(2 hunks)lib/src/crypto/utxo/entities/raw_transaction/input.dart(3 hunks)lib/src/crypto/wallet_utils.dart(3 hunks)lib/src/wallet/hd_node.dart(1 hunks)pubspec.yaml(1 hunks)test/ci/bip39/bip39_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/dart.yml
- lib/src/crypto/wallet_utils.dart
🔇 Additional comments (18)
lib/src/crypto/utxo/entities/raw_transaction/input.dart (2)
39-39: Null-safe conversion logic looks good.Using a null check before converting
_scriptSigto hex prevents potentialNullPointerExceptionscenarios.
88-89: Safer null and emptiness check.Verifying
_scriptSigis non-null and non-empty before instantiating aScriptobject avoids runtime errors and improves code reliability.lib/src/wallet/hd_node.dart (14)
1-9: Imports are organized properly.All required imports for cryptographic operations, base58 encoding, and typed data are included, which helps keep this file self-contained.
10-13: Constants are well-defined.Defining
HIGHEST_BIT,UINT31_MAX, andUINT32_MAXas named constants aids clarity in boundary checks.
43-70:derivePathapproach is logically sound.The regex-based path validation is practical and the stepwise derivation logic aligns with standard BIP32 flows.
72-101:extendedPrivateKeymethod accurately encodes data.The method checks version constraints, ensures the node is non-neutered, and uses
base58CheckEncode2. This approach is consistent with BIP32 extended key encoding.
102-122:extendedPublicKeymethod extends the correct data.Similar to
extendedPrivateKey, the public key variant ensures a valid version and encodes the chain code plus compressed pubkey data.
124-136:toWIFimplementation is complete.Good job incorporating version fallback to
network?.wifand ensuring the compression flag is set totrue.
138-197: Hardened and normal derivation with fallback to next index.The logic for appending the highest bit for hardened indexes is correct. However, repeatedly calling
return derive(index + 1);upon an invalid private key may lead to infinite recursion if many consecutive indexes are invalid.
199-209:neuteredmethod is consistent with BIP32.Stripping out the private key while preserving the public key and chain code effectively creates a watch-only node.
211-238:fromIconstructor ensures valid scalar.Throwing an error if
ILis not a valid private key helps maintain key integrity. Generating the corresponding public key viapointFromScalaris aligned with standard ecdsa usage.
239-249:verifyandsigneffectively delegate toecurve.Exposing these convenience methods on the HDNode itself is handy for local signing/verification.
251-283:fromPublicKeyfactory supports watch-only nodes.Enforcing curve validation is vital to ensure the public key is valid.
285-309:fromSeedmethod uses HMAC-SHA512 to generate key material.Enforcing min 128-bit (16 bytes) and max 512-bit (64 bytes) seed input is consistent with BIP32 conventions.
311-382:fromExtendedKeycovers both xpub and xprv decoding.Completes essential checks on depth, parent fingerprint, index, and chain code. This robust approach aligns with standard extended key parsing flows.
384-445:WIFclass thoroughly handles variable buffer lengths and compression.Properly implements version checks, compression flags, and conversion to/from Base58Check. This is a clean, straightforward approach.
pubspec.yaml (2)
9-9: Updated Dart SDK version.Raising the SDK requirement to
^3.7.2grants access to newer language features. Ensure the rest of the codebase is compatible with these changes.
13-13:httpdependency updated to 1.3.0.Confirm that no breaking changes in
http1.3.0 affect existing implementations (e.g., method signature changes or new constraints).
…o wallet_interface
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/crypto/network_type.dart (1)
176-201: Remove commented-out code.The commented-out
getForWalletTypemethod appears to be obsolete code that's been replaced by the newer implementation. It should be removed to reduce noise and improve maintainability.- // bip32.NetworkType getForWalletType(HDWalletPurpose purpose) => - // switch (purpose) { - // HDWalletPurpose.NO_STRUCTURE || - // HDWalletPurpose.BIP44 => - // bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip32PrivPrefix, - // public: bip32PubPrefix, - // ), - // ), - // HDWalletPurpose.BIP84 => bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip84PrivPrefix, - // public: bip84PubPrefix, - // ), - // ), - // HDWalletPurpose.BIP49 => bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip49PrivPrefix, - // public: bip49PubPrefix, - // ), - // ), - // };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/dart.yml(1 hunks)lib/src/crypto/network_type.dart(2 hunks)lib/src/crypto/utxo/entities/payments/p2h.dart(1 hunks)lib/src/crypto/utxo/utils/eurocoin_signing.dart(3 hunks)lib/src/crypto/utxo/utils/send.dart(5 hunks)pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/dart.yml
- lib/src/crypto/utxo/entities/payments/p2h.dart
- pubspec.yaml
- lib/src/crypto/utxo/utils/send.dart
- lib/src/crypto/utxo/utils/eurocoin_signing.dart
🔇 Additional comments (9)
lib/src/crypto/network_type.dart (9)
3-3: Good addition of the collection package import.This import is necessary for using
nonNullsandfirstOrNullextension methods in thefindPrefixesFromVersionmethod.
79-79: Well-structured type alias for improved code organization.The
BIP32Prefixesrecord type provides a clean way to encapsulate related prefix values, improving code readability and maintainability compared to individual prefix integers.
81-89: Good encapsulation with the new NetworkNodeInfo class.This class properly encapsulates network node information, including WIF and key prefixes, which aligns with good object-oriented design principles.
94-98: Well-structured BIP prefixes in NetworkBIP class.Replacing individual prefix integers with structured
BIP32Prefixesobjects improves code organization and type safety, making the code more maintainable.
111-125: Well-implemented finder method with functional programming principles.The
findPrefixesFromVersionmethod elegantly uses functional programming constructs likemap,nonNulls, andfirstOrNullto create a clean and efficient lookup mechanism.
127-146: Good use of switch statement for purpose-based node info retrieval.The
getForPurposemethod clearly maps each wallet purpose to its corresponding network configuration using a concise switch statement.
148-155: Constructor updated to match the new class structure.The constructor properly requires the new
BIP32Prefixesfields, ensuring type safety and consistency.
157-174: Clear implementation of version-based prefix lookup.The
fromVersionmethod provides a straightforward way to determine the appropriate prefixes based on a version number, with proper null-handling.
105-109:✅ Verification successful
Verify that default network BIP constants are properly updated.
The
defaultslist references constants likeBITCOIN_NETWORK_BIPwhich must be updated to use the newBIP32Prefixesstructure to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Verify that the NetworkBIP constants referenced in defaults list are properly updated # Look for the declarations of these constants to ensure they match the new structure echo "Searching for BITCOIN_NETWORK_BIP declaration..." rg -A 5 "BITCOIN_NETWORK_BIP\s*=" --type dart echo "Searching for LTC_NETWORK_BIP declaration..." rg -A 5 "LTC_NETWORK_BIP\s*=" --type dart echo "Searching for DOGE_NETWORK_BIP declaration..." rg -A 5 "DOGE_NETWORK_BIP\s*=" --type dartLength of output: 2134
NetworkBIP constants now use the updated BIP32Prefixes structure.
The constants (BITCOIN_NETWORK_BIP, LTC_NETWORK_BIP, and DOGE_NETWORK_BIP) in
lib/src/domain/constants.dartare defined with record literals (e.g.,bip32: (private: 0x0488ADE4, public: 0x0488B21E)) that conform to the new structure. Thedefaultslist inlib/src/crypto/network_type.dartcorrectly references these updated constants.
* fix: prevent rebroadcasting when no clients are available * feat: add smart fee estimation method to ElectrumXClient * Start with Weight Calculation * refactor: improve weight calculation and script size handling in BTCInput and BTCRawTransaction * Initial Refactor for UTXOScript and TransactionStructure * refactor: update BTCInput to handle script types and enhance transaction simulation tests * Add more derivation Tests * Fix Lint
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
lib/src/crypto/utxo/entities/raw_transaction/input.dart (1)
197-214: 🛠️ Refactor suggestion
EC8Inputweight is left unimplemented.Invoking
_addInputsor building an EC8 transaction that relies on weight might throwUnimplementedError. If this method is critical, consider at least returning a default or partial calculation.
♻️ Duplicate comments (1)
lib/src/crypto/utxo/utils/derivation.dart (1)
61-66:⚠️ Potential issueHandle potential null values for network information.
The
deriveMasterNodeFromExtendedKeyfunction does not handle the case whennodeNetworkInfois null, which could lead to unexpected behavior.Apply this diff to handle null
nodeNetworkInfo:HDNode deriveMasterNodeFromExtendedKey( String ePubKey, { NetworkNodeInfo? nodeNetworkInfo, }) { - return HDNode.fromExtendedKey(ePubKey, network: nodeNetworkInfo); + return HDNode.fromExtendedKey( + ePubKey, + network: nodeNetworkInfo, + ); }Consider adding a default network or additional error handling if
nodeNetworkInfois null.
🧹 Nitpick comments (22)
lib/src/utils/crypto.dart (1)
1-45: Comprehensive crypto utility functions, with minor inconsistencies.The cryptographic utility functions are well-implemented and provide important hash functionality for blockchain operations. The code is clean and follows good practices overall.
There are a few minor naming inconsistencies that could be improved:
- Function comment on line 39 says "HMAC Sha256" but the implementation and function name use SHA512
- The function and variable naming uses different casing patterns: "ripmed160" (lowercase) vs "SHA512" (uppercase)
Consider standardizing on consistent naming conventions:
- /// HMAC Sha256 + /// HMAC SHA512 ///lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart (1)
218-228: Add documentation and consider extracting shared logicThis method lacks documentation explaining how it differs from
estimateFee. Additionally, there's significant code duplication between the two fee estimation methods.Add documentation and consider refactoring to reduce duplication:
+/// Estimates the smart fee for the specified number of blocks. +/// Uses the blockchain.estimatesmartfee method which may use a different +/// algorithm than the regular estimateFee method. Future<double> estimateSmartFee({required int blocks}) async { final response = await _client.sendRequest( { "method": "blockchain.estimatesmartfee", "params": [blocks] }, ); final fee = response as double?; - if (fee == null || fee == 0) throw Exception("Fee estimation failed"); + if (fee == null || fee == 0) throw Exception("Smart fee estimation failed"); return fee; }Consider extracting the common logic to a private method like
_estimateFee(String method, int blocks)to eliminate duplicate code.lib/src/crypto/utxo/entities/payments/p2h.dart (1)
274-276: Add documentation for the new utility functionThe new
getSegwitScriptfunction lacks documentation explaining its purpose, parameters, and return value.Add documentation to clarify the function's purpose and usage:
+/// Creates a SegWit script (P2WPKH) from a public key hash. +/// +/// The script follows the structure: OP_0 <length> <pubKeyHash> +/// +/// @param pubKeyHash The hash of the public key (usually 20 bytes) +/// @return The complete SegWit script as a Uint8List Uint8List getSegwitScript(Uint8List pubKeyHash) { return [OPCODE.OP_0.hex, pubKeyHash.length, ...pubKeyHash].toUint8List; }lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart (1)
542-549: Add documentation for the new extensionThe new
OutputConverterextension lacks documentation explaining its purpose and usage context.Add documentation to clarify the extension's purpose:
+/// Extension to convert ElectrumOutput instances to the BTCOutput format. +/// This facilitates interoperability between Electrum data structures and +/// the wallet's internal transaction building components. extension OutputConverter on ElectrumOutput { + /// Converts this ElectrumOutput to a BTCOutput instance. + /// Uses the value and lockingScript from this output. Output get toOutput { return BTCOutput( value: value, script: scriptPubKey.lockingScript, ); } }lib/src/crypto/utxo/entities/raw_transaction/output.dart (1)
98-110: Avoid code duplication inbytesForTxId.
bytesForTxIdandbytesshare similar logic for offset calculation and script serialization. Consider extracting common logic into a helper to reduce duplication and potential maintenance overhead.- final buffer = Uint8List( - value_length + weight_length + script.size + 1, - ); - // Repeated offset & writeVarSlice logic + // Potential idea: unify in a shared function that both methods can calllib/src/crypto/utxo/entities/raw_transaction/input.dart (2)
29-50: Large block of commented-out code.These commented methods appear outdated. If no longer needed, consider removing them to reduce clutter. If planning to restore them, add a clear
TODOnote with context.- // BigInt get witnessSize {...} - // ... - // BigInt get scriptSize {...} + // TODO: Remove or revive with relevant new logic
74-79:publicKeyFromSigswitch expression is clean but watch for expansions.If future script types need special handling, add them carefully; otherwise, you risk
Unknown Scriptexceptions.lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (2)
40-40:feePerBytemight cause double truncation.
fee.toInt() / sizeyields an integer division for the numerator. For more precision, consider convertingfeeto double first.- double get feePerByte => fee.toInt() / size; + double get feePerByte => fee.toDouble() / size;
535-535: Similar to BTC, watch buffer size math.Large numbers of inputs/outputs might cause big final size. Possibly handle an overflow scenario or provide a max safe limit upstream.
lib/src/crypto/utxo/entities/raw_transaction/tx_structure.dart (13)
28-46: Check for input validation
InBTCLockingScript, consider additional validations (e.g., non-empty byte arrays) or error handling for abnormal inputs.
47-106: Enhance address parsing coverage
fromAddresssupports multiple address formats (P2PKH, P2WPKH, etc.). Add test coverage for malformed addresses and unrecognized prefixes to ensure robust error handling.
219-239: ScriptWitness and error handling
Verify thatscriptSigLengthis validated. Otherwise, a short buffer might cause sublist range errors.
281-298: PayToPublicKeyHashScript length checks
EnsurepublicKeyHashis strictly 20 bytes in all cases to uphold standard P2PKH script requirements.
300-315: Validate script hash length
ConfirmscriptHash.length == 20to avoid generating invalid P2SH scripts.
317-327: Check witness program length
Standard P2WPKH demands a 20-byte witness program. Validate length before constructing the script.
329-343: Check witness script hash length
For P2WSH, ensure thewitnessScriptHashis 32 bytes. Add validation to prevent malformed scripts.
345-350: Nested SegWit not fully implemented
This class might need expansion if supporting more advanced redeeming scenarios for nested SegWit.
370-385: Support for P2WSH-nested
Double-check thatripmed160Hash([00, 20, ...sha256Hash(witnessSript)])matches the intended nested SegWit design.
397-419: Commented-out multi-sig script
Remove or implement this code. Keeping large commented sections can lead to confusion.
431-445: Potential coverage for special scripts
“AnyoneCanSpend” and empty scripts can be risky if misused. Provide ample testing or usage restrictions.
487-510: SegWit buffer building
SegwitFormat.buildBufferis likewise unimplemented. Offer a fallback or provide a thorough implementation for building SegWit transactions.
512-525: Taproot Format
Future expansions may handle script path spending or advanced taproot features. Ask if you need assistance implementing these.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(0 hunks)lib/src/crypto/evm/utils/signing.dart(0 hunks)lib/src/crypto/utxo/entities/op_codes.dart(2 hunks)lib/src/crypto/utxo/entities/payments/input_selection.dart(0 hunks)lib/src/crypto/utxo/entities/payments/p2h.dart(7 hunks)lib/src/crypto/utxo/entities/payments/pk_script_converter.dart(0 hunks)lib/src/crypto/utxo/entities/raw_transaction/input.dart(8 hunks)lib/src/crypto/utxo/entities/raw_transaction/output.dart(6 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(28 hunks)lib/src/crypto/utxo/entities/raw_transaction/tx_structure.dart(1 hunks)lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart(2 hunks)lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart(1 hunks)lib/src/crypto/utxo/utils/derivation.dart(5 hunks)lib/src/crypto/utxo/utils/eurocoin_signing.dart(3 hunks)lib/src/crypto/utxo/utils/proof_of_payment.dart(6 hunks)lib/src/crypto/utxo/utils/pubkey_to_address.dart(5 hunks)lib/src/crypto/utxo/utils/send.dart(15 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(7 hunks)lib/src/domain/entities/generic_transaction.dart(1 hunks)lib/src/utils/crypto.dart(1 hunks)lib/src/utils/var_uint.dart(1 hunks)
💤 Files with no reviewable changes (4)
- lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart
- lib/src/crypto/utxo/entities/payments/pk_script_converter.dart
- lib/src/crypto/utxo/entities/payments/input_selection.dart
- lib/src/crypto/evm/utils/signing.dart
✅ Files skipped from review due to trivial changes (1)
- lib/src/domain/entities/generic_transaction.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/crypto/utxo/utils/eurocoin_signing.dart
- lib/src/crypto/utxo/utils/proof_of_payment.dart
- lib/src/crypto/utxo/utils/send.dart
🔇 Additional comments (63)
lib/src/crypto/utxo/utils/pubkey_to_address.dart (4)
1-8: Good migration to centralized crypto utilities.The file now imports the centralized crypto utilities rather than implementing these functions separately, which improves code organization and maintainability.
25-40: Successful refactoring to use centralized crypto functions.The address generation functions now use the imported crypto utilities which reduces code duplication and centralizes cryptographic implementations.
109-111: Clean implementation of P2SH address encoding.The P2SH address function now uses
base58CheckEncodedirectly, making the code more concise.
153-162: Improved code formatting for checksum calculation.The checksum data setup and calculation is now better formatted with clear array construction.
lib/src/crypto/utxo/utils/derivation.dart (6)
3-4: Successful migration from BIP32 to HDNode.The code now uses the custom HDNode implementation instead of the BIP32 library, which appears to be part of a larger architectural change.
7-11: Required network type improves type safety.Making UTXONetworkType required instead of optional improves type safety and reduces potential runtime errors.
37-38: More direct approach to extended public key generation.Using
extendedPublicKey()method makes the code more readable than previous implementation.
68-94: Well-structured new utility method for deriving nodes with addresses.The new
deriveNodeFromSeedfunction provides a comprehensive solution that includes both node derivation and address generation, which will simplify client code.
152-174: Updated extension for HDNode with backward compatibility.The extension method has been properly updated to work with HDNode instead of BipNode, while maintaining backward compatibility with WalletKit.
176-179: Simplified node derivation function.The updated
deriveNodefunction is now cleaner and uses the new HDNode implementation correctly.lib/src/crypto/utxo/entities/op_codes.dart (2)
5-6: Added dependency for collection utilities.The import for the collection package is appropriate for accessing the
singleWhereOrNullmethod used in the new functionality.
153-155: Useful utility method for opcode lookup by hex value.The new
fromHexstatic method provides a convenient way to look up OPCODE instances by their hexadecimal value, which will likely be useful when parsing or processing transaction scripts.lib/src/crypto/utxo/entities/payments/p2h.dart (2)
8-8: LGTM! Added import for crypto utilitiesThe new import is appropriate for supporting the new functionality in this file.
93-95: LGTM! Formatting improvementsThe code restructuring improves readability without changing functionality.
lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart (1)
509-511:❓ Verification inconclusive
Verify backward compatibility with return type change
The return type of the
lockingScriptgetter has changed fromUint8ListtoBTCLockingScript, which could break existing code that expects a byte array.Please check all usages of this getter to ensure they can handle the new return type. If some callers still expect
Uint8List, consider adding a separate getter method instead of changing the existing one.
🏁 Script executed:
#!/bin/bash # Find all usages of lockingScript to verify compatibility with new return type rg "\.lockingScript" --type dart -A 2 -B 2Length of output: 1790
Action Required: Verify Compatibility of
lockingScriptGetter Return TypeThe new return type is now a
BTCLockingScript(constructed viaBTCLockingScript.fromBuffer(...)), whereas it previously returned aUint8List. The shell search output shows that the getter is used in several locations:
test/no_ci/input_simulation_test.dart
- Usage: Passed to
BTCOutputasscript: out.scriptPubKey.lockingScriptlib/src/crypto/utxo/utils/send.dart
- Usage: Assigned to
prevScriptPubKeyviaprevScriptPubKey = output.scriptPubKey.lockingScriptlib/src/crypto/utxo/entities/transactions/utxo_transaction.dart
- Usage: Passed directly to constructor calls within the transaction assembly
Please double-check that each caller correctly handles a
BTCLockingScriptinstance. If any consumers rely on receiving a raw byte array (aUint8List), then they will need to extract the underlying bytes from theBTCLockingScriptor, alternatively, a separate getter returning aUint8Listshould be added to preserve backward compatibility.lib/src/crypto/utxo/entities/raw_transaction/output.dart (4)
5-5: No concerns with additional import.Importing
tx_structure.dartseems necessary to useBTCLockingScript.
13-36: Good refactoring to a dedicatedBTCLockingScript.
- Replacing the old raw script representation with
BTCLockingScriptimproves type safety and readability.- Accessors like
scriptHexandweightdelegating directly toscriptseem concise; verify that there's no need for additional validations (e.g., empty or invalid script).
74-91: Consistent usage ofscriptinEC8Output.The approach matches the BTC counterpart with
BTCLockingScript, but remember to confirm behavior when the script is empty or especially large, to avoid out-of-bounds issues.
126-132: Consider handling malformed script data.When invoking
BTCLockingScript.fromBuffer(script), ensure it won’t throw unhandled exceptions for invalid or truncated inputs. If necessary, add safety checks or error handling upstream.lib/src/crypto/utxo/entities/raw_transaction/input.dart (10)
4-5: Import statements look fine.No issues with these imports referencing
output.dartandtx_structure.dart.
52-62: Abstract and derived getters align with the refactoring.
- Making
weightan abstract getter is logical since each transaction type has different weight calculations.intValue,txIdString,bytes,size, andtoHexcollectively unify the interface.- Verify any external usage expecting old weight calculation logic.
64-71: Clear scripts referencing.
previousScriptdelegates toprevOutput?.script. This is neat but watch for cases whereprevOutputis null.- The
isSegwitcheck (previousScript is PayToWitness...orhasWitness) is a simpler design. This is beneficial for type checking.
82-82: RefinedaddScriptsignature.Simplifying to
addScript(BTCScript script)clarifies usage. Make sure all code paths that used the older optional parameters are updated.
125-125: BTCInput:fromBuffersetsprevOutputto null.
- Ensure the rest of the system properly assigns
prevOutputif needed downstream.- If
prevOutputis mandatory in some contexts, confirm it's populated before transaction signing.
134-134: Adding script to input is straightforward.No immediate issues. The new function consistently returns a fresh
BTCInputwith updatedscript.
145-191:BTCInputweight calculation &byteslogic.
assert(script != null, "Script is required");is good for safety.- The approach to calculate input weight by factoring in script type is prudent. For further script types, update the
switch._script.size + getVarIntSize(_script.size)usage is correct for a VarSlice, but watch for potential large script sizes.
242-249:EC8Input.addScriptmirrors Bitcoin’s approach.Consistent design. Ensure advanced scripts (like witness or redeem scripts) don’t cause confusion with
EC8logic if they are not used.
252-279: Ensure buffer capacity in EC8’sbytesmethod.
- Like with BTC, watch
script.size + getVarIntSize(_script.size).assert(script != null)is good, but consider an explicit error message if parsing fails.
307-344:bytesForSigningenhancements.
- The assertion
assert(withWeight || withScript, "At least one...")is helpful.- Ensure the single input with a nonempty script is indeed the correct one for signing. Let’s confirm multi-input signing flows.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (17)
4-5: Confirmed new imports fortx_structureandcryptoutilities.No obvious issues; presumably needed for hashing and script classes.
24-25: Makingweightabstract is logical.Different transaction variants define weight differently. This clarifies design and enforces each subclass to provide an implementation.
28-29:vSizecalculation is trivial but correct.
vSize=weight / 4. This matches standard definitions. Verify no rounding edge cases are needed.
53-53: Sums output values safely.
outputs.fold(...)is straightforward; no concerns.
66-71: Assertion thathashType == networkType.sighash.all.Clarifies only
SIGHASH_ALLis supported. Might catch dev errors early.
77-82: Nullifying scriptSigs before substituting the correct one is standard.This step ensures only the target input has a real script for signature hashing. Looks good.
153-153: Signing transaction requiresinputMap.The assertion is correct. Failing early if inputs are missing is helpful.
185-185: Constructor chaining remains consistent.
super(inputs: inputs, outputs: outputs);ensures base fields stay in sync.
187-212:BTCRawTransactionweight logic.
- Factor of 4 for the base fields plus script weight is correct for segwit vs. non-segwit.
- The
isSegwitproperty short-circuits if any input is segwit. This is consistent with standard logic.
263-279: Parsing input length withreadVarInt.Check for edge cases: e.g., malformed or too-large input count. If an attacker crafts malicious data, be sure it’s handled gracefully.
290-311: Witness script reading logic.
- Collecting
ScriptWitnessfrom each segwit input is standard, then reassigning it to the correct input.- Confirm that throwing away the 0x00 placeholder for non-segwit inputs is intended.
339-356: ComputetxByteLengthfor segwit carefully.
- The separate additions for segwit marker, flag, scripts, and empty scripts are correct.
- Double-check the loop logic for large witness data.
389-389: Appending segwit script bytes.
offset += buffer.writeSlice(offset, input.script!.bytes);is direct. Just ensurescriptis truly a witness script in that context.
404-485:bip143sigHashis properly referencing BIP specs.
PayToPublicKeyHashScriptfor the previous script is correct for typical P2PKH flows.- Confirm no advanced script types (like multi-signature or P2SH) are needed for BIP143.
505-505: Consistent super constructor forEC8RawTransaction.Matches the approach in
BTCRawTransaction.
507-511:EC8RawTransactionweight method.Summing input + output weight is straightforward. If the chain’s future rules differ, ensure we adapt.
597-597: Same caution about potential overflow withtxByteLength.As with the other code, watch edge cases.
lib/src/crypto/utxo/utxo_analyzer.dart (9)
9-9: Check for usage ofHDNodeimport
It appears that you've introduced an import forpackage:walletkit_dart/src/wallet/hd_node.dart. Ensure that references toHDNodeare correct and that no references to the old bip32 remain in the codebase.
207-217: Add error handling forderiveMasterNodeFromExtendedKey
A previous review suggested wrapping this call in a try-catch block to handle invalid extended keys or network configuration issues. Revisit that recommendation to enhance robustness.
283-289: Add exception handling forderiveMasterNodeFromSeed
Consider wrapping this in a try-catch block to manage potential invalid seeds or wallet paths, similar to prior feedback.
323-324: Adoption ofHDNode
Switching fromBipNodetoHDNodeis consistent with the new HD-based approach.
363-364: Function signature updated
RequiringHDNode masterNodehelps unify the code with the new hierarchical deterministic wallet design.
502-502: Optional flag for smart fee
The new parameteruseSmartFeeconveniently toggles advanced fee estimation logic.
505-507: Conditional fee estimation
Switching betweenestimateSmartFeeandestimateFeebased onuseSmartFeeis a clear approach.
517-517: Check fee conversion
Multiplying by0.001is effectively the same as dividing by1000. Confirm that the ceiling operation matches the intended rounding for per-byte fees.
525-525: Introduce smart fee usage
AddinguseSmartFeehere aligns with the fee estimation logic introduced above.lib/src/crypto/utxo/entities/raw_transaction/tx_structure.dart (8)
1-13: Verify external dependencies
All new imports, includingbs58checkanddart_bech32, look correct. Verify that versions are compatible and secure.
14-26: Use ofsealed classfor scripts
DefiningBTCScriptas sealed helps enforce a controlled hierarchy. This is a good design choice for representing script variants.
107-156: Broad script type inference infromBuffer
The opcode-based logic is well-structured. Confirm that out-of-range opcodes or invalid sequences are either handled or reported properly.
241-267: Time-locked script design
The approach to building time-locked scripts is neatly organized. Good job integrating OP_CHECKLOCKTIMEVERIFY or OP_CHECKSEQUENCEVERIFY logic.
269-279: Use ofOP_CHECKSIG
Pay-to-Public-Key scripts look correct. Recommend writing specific tests to confirm valid and invalid P2PK usage.
352-368: Potential usage in multi-sig
Nested SegWit structures can grow complex. Validate thoroughly for advanced features like multi-sig if needed.
421-429: OP_RETURN usage
Enforcing an 80-byte data limit is network-dependent. Confirm that matches expected chain policies or user requirements.
447-467: Transaction structure design
Sealing the class and enumerating accepted input types is clean. Supports future extension for different transaction versions.
| int getVarIntSize(int value) { | ||
| if (value < 0xfd) { | ||
| return 1; | ||
| } | ||
| if (value <= 0xffff) { | ||
| return 3; | ||
| } | ||
| if (value <= 0xffffffff) { | ||
| return 5; | ||
| } | ||
| return 9; | ||
| } |
There was a problem hiding this comment.
Duplicate code detected - consolidate with encodingLength
The new getVarIntSize function duplicates the logic already present in the encodingLength function (lines 131-140). Both functions calculate the size of a variable integer using the same conditions and return values.
Consider removing this function and using encodingLength instead, or if there's a specific reason for having both, add documentation explaining the difference:
-int getVarIntSize(int value) {
- if (value < 0xfd) {
- return 1;
- }
- if (value <= 0xffff) {
- return 3;
- }
- if (value <= 0xffffffff) {
- return 5;
- }
- return 9;
-}If this function must be preserved because it has a different purpose than encodingLength, add a documentation comment explaining its purpose and the reason for having both.
📝 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.
| int getVarIntSize(int value) { | |
| if (value < 0xfd) { | |
| return 1; | |
| } | |
| if (value <= 0xffff) { | |
| return 3; | |
| } | |
| if (value <= 0xffffffff) { | |
| return 5; | |
| } | |
| return 9; | |
| } |
| final (script, off2) = buffer.readVarSlice(offset); | ||
| offset += off2; | ||
|
|
||
| return BTCOutput( | ||
| value: BigInt.from(value), | ||
| scriptPubKey: scriptPubKey, | ||
| script: BTCLockingScript.fromBuffer(script), | ||
| ); | ||
| } | ||
|
|
||
| Uint8List get bytes { | ||
| final buffer = Uint8List(value_length + scriptPubKey.length + 1); | ||
| final buffer = Uint8List(value_length + script.size + 1); | ||
|
|
||
| var offset = 0; | ||
|
|
||
| // Write Value | ||
| offset += buffer.bytes.writeUint64(offset, intValue); | ||
|
|
||
| // Write ScriptPubKey | ||
| offset += buffer.writeVarSlice(offset, scriptPubKey); | ||
| offset += buffer.writeVarSlice(offset, script.bytes); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate buffer allocation for script data.
The buffer is sized as Uint8List(value_length + script.size + 1), and then you call writeVarSlice. Ensure that the +1 is sufficient to accommodate variable-length encoding for all possible script sizes. Consider checking edge cases with large scripts to avoid potential overflow.
🏁 Script executed:
#!/bin/bash
# Description: Check all buffer allocations in the outputs to confirm they handle large scripts.
rg "Uint8List\(value_length \+ script\.size \+ 1\)" -A 5Length of output: 561
Attention: Validate Buffer Allocation for Variable-Length Script Data
The current allocation (Uint8List(value_length + script.size + 1)) assumes that the variable-length encoding for the script’s length will always require exactly one extra byte. However, Bitcoin’s CompactSize encoding uses more than one additional byte for script lengths ≥253 bytes. Please ensure that:
- The allocation is either restricted to inputs with script sizes below 253 bytes (if that invariant is maintained elsewhere), or
- The buffer allocation logic is adjusted to dynamically account for the extra bytes needed for larger scripts.
This verification is crucial to avoid potential overflow issues when handling large scripts.
| final Output? prevOutput; | ||
|
|
||
| final BTCScript? script; | ||
|
|
||
| BigInt? get value => prevOutput?.value; | ||
|
|
||
| const Input({ | ||
| required this.txid, | ||
| required this.vout, | ||
| this.value, | ||
| Uint8List? prevScriptPubKey, | ||
| Uint8List? scriptSig, | ||
| Uint8List? wittnessScript, | ||
| }) : _scriptSig = scriptSig, | ||
| _prevScriptPubKey = prevScriptPubKey, | ||
| _wittnessScript = wittnessScript; | ||
| required this.prevOutput, | ||
| required this.script, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Streamlined constructor with prevOutput and script.
- Consolidating old nullable fields into
prevOutputandscriptclarifies the input’s previous transaction details. - Ensure that existing code referencing
_scriptSig,_prevScriptPubKey, etc., is updated accordingly.
🏁 Script executed:
#!/bin/bash
# Description: Check for any residual references to removed properties.
rg "_scriptSig|_prevScriptPubKey"Length of output: 548
Enhanced Input Constructor – Consider Removing Legacy Comments
The refactor now uses prevOutput and script, which effectively streamlines the Input constructor. However, residual commented-out references to _scriptSig and _prevScriptPubKey remain in the file. Please review if these legacy comments are still needed; if not, removing them would help avoid potential confusion.
- File:
lib/src/crypto/utxo/entities/raw_transaction/input.dart- Legacy references remain in comments (e.g., around lines with
// if (_scriptSig ...).
- Legacy references remain in comments (e.g., around lines with
| /// Input to be signed | ||
| /// | ||
| /// Input to be signed has a scriptSig all other inputs have empty scriptSigs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Single input detection logic.
input.script!.size != 0 is used to identify which input is “the one” to sign. This might break in multi-input scenarios if multiple inputs need signing. Possibly refine logic if multi-sign is required.
- final input = inputs.singleWhereOrNull((input) => input.script!.size != 0);
+ // Consider indexing by "input to sign" or passing an index paramCommittable suggestion skipped: line range outside the PR's diff.
| sealed class BTCUnlockingScript extends BTCScript { | ||
| const BTCUnlockingScript(super.bytes); | ||
|
|
||
| factory BTCUnlockingScript.fromBuffer(Uint8List buffer) { | ||
| final first = buffer[0]; | ||
|
|
||
| return switch (first) { | ||
| 0x00 => RedeemScript.fromScript(buffer), | ||
| 0x02 => ScriptWitness.fromScript(buffer), | ||
| _ => ScriptSignature.fromScript(buffer), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uniform approach to unlocking scripts
BTCUnlockingScript is well-defined; consider range checks on buffer lengths to handle malformed scripts gracefully.
| /// | ||
| /// RedeemScript is used to unlock a P2SH output | ||
| /// | ||
| final class RedeemScript extends BTCUnlockingScript { | ||
| late final Uint8List signature; | ||
| late final Uint8List redeemScript; | ||
|
|
||
| RedeemScript.fromScript(super.bytes) { | ||
| final signatureLength = bytes[1]; | ||
| signature = bytes.sublist(2, 2 + signatureLength).toUint8List; | ||
| redeemScript = bytes.sublist(3 + signatureLength).toUint8List; | ||
| } | ||
|
|
||
| RedeemScript(this.signature, this.redeemScript) | ||
| : super( | ||
| [ | ||
| 0x00, | ||
| signature.length, | ||
| ...signature, | ||
| redeemScript.length, | ||
| ...redeemScript, | ||
| ].toUint8List, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible null-safety improvements
Accessing bytes[1] could fail if the buffer is too short. Add checks to prevent out-of-range sublist operations for RedeemScript.
| final class ScriptSignature extends BTCUnlockingScript { | ||
| late final Uint8List signature; | ||
| late final Uint8List publicKey; | ||
|
|
||
| ScriptSignature.fromScript(super.bytes) { | ||
| final signatureLength = bytes[1]; | ||
| signature = bytes.sublist(2, 2 + signatureLength).toUint8List; | ||
| publicKey = bytes.sublist(3 + signatureLength).toUint8List; | ||
| } | ||
|
|
||
| ScriptSignature(this.signature, this.publicKey) | ||
| : super( | ||
| [ | ||
| signature.length, | ||
| ...signature, | ||
| publicKey.length, | ||
| ...publicKey, | ||
| ].toUint8List, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ScriptSignature structure
Similarly, ensure sublist extraction won't overflow if the provided buffer is malformed or truncated.
| final class PayToTaprootScript extends BTCLockingScript { | ||
| final Uint8List pubkey; | ||
|
|
||
| PayToTaprootScript.fromScript(super.bytes) | ||
| : pubkey = bytes.sublist(2, bytes.length).toUint8List; | ||
|
|
||
| PayToTaprootScript(this.pubkey) | ||
| : super([OPCODE.OP_1.hex, pubkey.length, ...pubkey].toUint8List); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Taproot script handling
Currently limited to basic pubkey usage. For advanced Taproot scripts, consider supporting script path or key path logic.
| final class LegacyFormat extends BTCTransactionStructure { | ||
| @override | ||
| Set<InputType> get acceptedInputTypes => { | ||
| InputType.P2PKH, | ||
| InputType.P2SH, | ||
| InputType.P2PK, | ||
| }; | ||
|
|
||
| Uint8List buildBuffer({ | ||
| required int version, | ||
| required int lockTime, | ||
| required List<Input> inputs, | ||
| required List<Output> outputs, | ||
| }) { | ||
| return Uint8List(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unimplemented buffer builder (LegacyFormat)
buildBuffer currently returns an empty Uint8List(0). Implement or remove to prevent confusion.
Co-authored-by: dev2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/src/crypto/network_type.dart (4)
105-109: Consider a more extensible approach for defaults listThe static list of default networks might require manual updates when new networks are added.
Consider implementing a registration mechanism or factory pattern that would make it easier to add new networks without modifying this list directly.
176-201: Remove commented out codeThe commented out getForWalletType method should be removed since it appears to be replaced by the new implementation.
- // bip32.NetworkType getForWalletType(HDWalletPurpose purpose) => - // switch (purpose) { - // HDWalletPurpose.NO_STRUCTURE || - // HDWalletPurpose.BIP44 => - // bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip32PrivPrefix, - // public: bip32PubPrefix, - // ), - // ), - // HDWalletPurpose.BIP84 => bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip84PrivPrefix, - // public: bip84PubPrefix, - // ), - // ), - // HDWalletPurpose.BIP49 => bip32.NetworkType( - // wif: wif, - // bip32: bip32.Bip32Type( - // private: bip49PrivPrefix, - // public: bip49PubPrefix, - // ), - // ), - // };
243-246: Inconsistent spelling in constant nameThe spelling "TRESHOLD" is used instead of the correct "THRESHOLD".
-const DOGE_DUSTTRESHOLD = ( +const DOGE_DUST_THRESHOLD = ( legacy: 100000000, segwit: 100000000, );Note: This inconsistency appears throughout the codebase with other constants like BTC_DUSTTRESHOLD and NO_DUSTTRESHOLD.
445-448: Clarify commented out endpointsThere are commented out endpoints in the Dogecoin network configuration. If these are intentionally disabled, consider adding a comment explaining why, or remove them if they're no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/crypto/network_type.dart(4 hunks)lib/src/crypto/utxo/entities/payments/p2h.dart(8 hunks)lib/src/crypto/utxo/utils/send.dart(14 hunks)lib/src/domain/constants.dart(2 hunks)lib/src/domain/predefined_assets.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/crypto/utxo/utils/send.dart
🧰 Additional context used
🪛 GitHub Actions: TestSuite
lib/src/domain/constants.dart
[error] 76-76: The named parameter 'bip32' is required, but there's no corresponding argument. Try adding the required argument.
[error] 76-76: The named parameter 'bip49' is required, but there's no corresponding argument. Try adding the required argument.
[error] 76-76: The named parameter 'bip49MultiSig' is required, but there's no corresponding argument. Try adding the required argument.
[error] 76-76: The named parameter 'bip84' is required, but there's no corresponding argument. Try adding the required argument.
[error] 76-76: The named parameter 'bip84MultiSig' is required, but there's no corresponding argument. Try adding the required argument.
[error] 77-77: The named parameter 'bip32XpubPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip32XpubPrefix'.
[error] 78-78: The named parameter 'bip32XprivPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip32XprivPrefix'.
[error] 79-79: The named parameter 'bip49XpubPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip49XpubPrefix'.
[error] 81-81: The named parameter 'bip49XprivPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip49XprivPrefix'.
[error] 82-82: The named parameter 'bip84XpubPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip84XpubPrefix'.
[error] 83-83: The named parameter 'bip84XprivPrefix' isn't defined. Try correcting the name to an existing named parameter's name, or defining a named parameter with the name 'bip84XprivPrefix'.
🔇 Additional comments (18)
lib/src/crypto/network_type.dart (7)
79-79: Type alias improves code clarityGood use of Dart's record syntax to create a type alias for BIP32 prefixes. This makes the code more readable and type-safe.
81-89: Well-structured NetworkNodeInfo classThe new NetworkNodeInfo class is well-designed with clear responsibilities and proper immutability using final fields.
94-98: Good refactoring of NetworkBIP fieldsThe replacement of individual prefix fields with structured BIP32Prefixes objects improves type safety and maintainability.
111-125: Effective implementation of findPrefixesFromVersionGood use of functional programming with map/filter pattern and null safety with nonNulls and firstOrNull.
127-146: Well-structured getForPurpose methodThe switch statement is exhaustive for all enum values and properly handles each HDWallet purpose type.
157-174: Clean implementation of fromVersion methodThe method efficiently checks version numbers against all prefix types and returns the appropriate match.
431-459: Good implementation of Dogecoin networkThe Dogecoin network implementation follows the established pattern for other networks and includes appropriate network parameters.
lib/src/domain/predefined_assets.dart (1)
31-35: Addition of Dogecoin asset looks good.The Dogecoin cryptocurrency has been properly added as a predefined asset, with the correct number of decimal places (8) and appropriate name and symbol.
lib/src/crypto/utxo/entities/payments/p2h.dart (6)
8-8: Import for crypto utilities added correctly.The import for the crypto.dart utility package is necessary for the updated functionality.
22-23: Dogecoin P2PKH support added properly.The additional check for the Dogecoin legacy address prefix is correctly implemented.
28-29: Dogecoin P2SH support added properly.The additional check for the Dogecoin script hash address prefix is correctly implemented.
34-35: Dogecoin SegWit support added properly.The additional check for the Dogecoin SegWit address prefix is correctly implemented.
153-167: Code formatting improved.The improved formatting with more line breaks makes the switch statement clearer and more readable.
277-279: New SegWit script utility method looks good.The
getSegwitScriptutility method is properly implemented to generate a SegWit script from a public key hash. This will be useful for Dogecoin SegWit transaction support.lib/src/domain/constants.dart (4)
37-39: Dogecoin address prefixes added correctly.The address prefixes for Dogecoin's legacy (P2PKH), compatibility (P2SH), and native SegWit (P2WPKH) formats are correctly defined.
47-64: Network BIP parameters restructured properly for Bitcoin and Litecoin.The BIP parameters for Bitcoin and Litecoin have been restructured into a tuple format for better organization. The implementation includes proper support for multi-signature formats.
67-74: Dogecoin BIP parameters added correctly.The Dogecoin network BIP parameters are properly defined with the correct prefix values for different derivation paths.
108-114: Litecoin compatibility parameters updated correctly.The Litecoin compatibility parameters have been properly updated to match the new tuple-based format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/src/crypto/network_type.dart (4)
102-107: Consider making defaults list constantThe
defaultslist holds constant values but isn't defined asconst. Consider making it astatic constlist for better optimization.-static List<NetworkBIP> defaults = [ +static const List<NetworkBIP> defaults = [ BITCOIN_NETWORK_BIP, LTC_NETWORK_BIP, DOGECOIN_NETWORK_BIP, ];
142-159: Consider using a switch expression for version matchingThe sequential if statements in
fromVersioncould be replaced with a more concise switch expression for better readability.BIP32Prefixes? fromVersion(int version) { - if (version == bip32.private || version == bip32.public) { - return bip32; - } - if (version == bip49.private || version == bip49.public) { - return bip49; - } - if (version == bip84.private || version == bip84.public) { - return bip84; - } - if (version == bip49MultiSig.private || version == bip49MultiSig.public) { - return bip49MultiSig; - } - if (version == bip84MultiSig.private || version == bip84MultiSig.public) { - return bip84MultiSig; - } - return null; + return switch (version) { + _ when version == bip32.private || version == bip32.public => bip32, + _ when version == bip49.private || version == bip49.public => bip49, + _ when version == bip84.private || version == bip84.public => bip84, + _ when version == bip49MultiSig.private || version == bip49MultiSig.public => bip49MultiSig, + _ when version == bip84MultiSig.private || version == bip84MultiSig.public => bip84MultiSig, + _ => null, + }; }
161-186: Remove commented-out codeThe commented-out
getForWalletTypemethod appears to be obsolete and has been replaced by the newgetForPurposemethod. Consider removing this commented code for cleaner maintenance.
218-222: Consider consistent naming convention for constantsThe dust threshold constants follow different capitalization patterns:
BTC_DUSTTRESHOLD,NO_DUSTTRESHOLD, andDOGE_DUSTTRESHOLD. Also, there's a typo: "TRESHOLD" should be "THRESHOLD".-const BTC_DUSTTRESHOLD = (legacy: 546, segwit: 294); -const NO_DUSTTRESHOLD = (legacy: 0, segwit: 0); -const DOGE_DUSTTRESHOLD = (legacy: 100000000, segwit: 100000000); +const BTC_DUST_THRESHOLD = (legacy: 546, segwit: 294); +const NO_DUST_THRESHOLD = (legacy: 0, segwit: 0); +const DOGE_DUST_THRESHOLD = (legacy: 100000000, segwit: 100000000);Then update all references to these constants in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/crypto/network_type.dart(6 hunks)lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart(11 hunks)lib/src/crypto/utxo/utils/pubkey_to_address.dart(5 hunks)lib/src/domain/constants.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/crypto/utxo/utils/pubkey_to_address.dart
- lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart
- lib/src/domain/constants.dart
🔇 Additional comments (7)
lib/src/crypto/network_type.dart (7)
3-3: New import introducedThe addition of
package:collection/collection.dartis appropriate for usingnonNullsandfirstOrNullmethods in thefindPrefixesFromVersionstatic method.
79-80: Good usage of type alias for BIP32 prefixesCreating a type alias for the BIP32 prefixes as a record type with
privateandpublicfields provides better type safety and readability compared to having individual prefix integers.
81-86: Well-structured NetworkNodeInfo classThe new
NetworkNodeInfoclass encapsulates the WIF (Wallet Import Format) key and BIP32 prefixes together, providing a clean abstraction for node information.
91-95: Improved organization of BIP prefixesReplacing individual prefix integers with structured
BIP32Prefixesobjects for different BIP standards enhances code organization and maintainability.
108-119: Well-implemented lookup functionThe
findPrefixesFromVersionmethod efficiently searches through network defaults to find matching prefixes for a given version, making good use of Dart's functional programming capabilities.
121-131: Clear purpose-based node info selectorThe
getForPurposemethod provides a straightforward way to get the appropriate network node info based on the HD wallet purpose.
403-431: Good addition of Dogecoin network supportThe Dogecoin network implementation is well-structured and follows the same pattern as other UTXO networks. One minor observation is that there are commented-out endpoints that could be cleaned up if they're no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
4-7: Enhanced Library Description
The introduction (line 6) now concisely outlines support for a variety of blockchain networks. Consider explicitly mentioning the new Wallet CLI or wallet interface—especially since this PR focuses on wallet improvements—to better align the documentation with the current feature set.
10-25: Structured Features Section
The features section is well-organized into bullet points detailing Multi-Chain Support, Transaction Management, Wallet Features, Performance Optimization, and Comprehensive Testing. This structured approach improves readability. Additionally, if the new Wallet CLI is a core update, consider adding a dedicated bullet to highlight its capabilities.
98-101: Polish the Contributing Section
The contributing section (lines 98–101) is inviting; however, consider rephrasing "Please feel free to submit a Pull Request" to a more professional tone. For example, "We encourage you to submit a Pull Request" sounds more assertive and polished.🧰 Tools
🪛 LanguageTool
[style] ~100-~100: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## License Thi...(FEEL_FREE_TO_STYLE_ME)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
LICENSE(1 hunks)README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- LICENSE
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~100-~100: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## License Thi...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (10)
README.md (10)
1-1: Badge and CI Status Update
The Test Suite badge on line 1 provides clear visual feedback of the CI status, enhancing the perceived reliability of the project.
26-48: Clear Getting Started Instructions
The “Getting Started” section (lines 26–48) provides clear, step-by-step setup instructions. The inclusion of submodule initialization, pubspec updates, and initialization commands ensures that new users have a smooth onboarding process.
50-54: Explicit Requirements Section
The requirements section (lines 50–54) effectively specifies the Dart SDK version and points users to detailed dependency information inpubspec.yaml. This clarity helps prevent setup issues.
55-63: Comprehensive Architecture Overview
The architecture section (lines 55–63) succinctly describes how WalletKit-Dart handles UTXO-based chains, mentioning key aspects like unspent transaction outputs and efficient value transfers. This provides a solid conceptual background for users.
64-68: Detailed EVM Chains Section
The EVM chains section (lines 64–68) clearly outlines support for Ethereum and compatible networks, including smart contract functionality and state-based architecture. The details are precise and informative.
69-75: Effective API Design Philosophy Description
The API Design Philosophy section (lines 69–75) effectively communicates the design choices—such as statelessness and memory efficiency—using a concise bullet-point format. This aligns well with modern API practices.
76-80: Robust Backend Integration Details
The backend integration section (lines 76–80) distinguishes the protocols used for UTXO (ElectrumX Protocol) and EVM (JSON-RPC and Etherscan-style APIs) chains, providing a clear view of how data is sourced and managed.
81-87: Clear Examples Section
The examples section (lines 81–87) includes links to practical implementations and API documentation, serving as helpful references for users.
88-97: Concise Test Examples Listing
The test examples section (lines 88–97) is succinct and clearly lists practical test cases, which will aid users in understanding functionality and validating different use cases.
102-105: License Information is Clear
The license section (lines 102–105) clearly states the project's MIT License and directs users to the LICENSE file for further details. This is straightforward and effective.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (18)
lib/src/wallet/bip32/hd_node.dart (5)
36-41: Refine code to allow uncompressed public keys for neutered nodes.If you'd like to retrieve an uncompressed public key from a neutered HDNode, you could compute it from
_qalone usingecurve.pointFromScalar. Right now, requiring_pto build an uncompressed public key may be limiting if you only hold a neutered node.
43-67: Regex-based path validation might be too restrictive.Your
derivePathmethod only matches decimal indexes with an optional'suffix for hardened keys. If you plan to support more sophisticated hierarchical paths, you may need a more flexible parser to accommodate extended syntax and additional path features.
141-193: Validate approach for skipping invalid child indices.Your implementation re-derives the next index if the scalar part (
IL) is invalid. This approach aligns with BIP32’s handling of invalid keys, but be aware of potential performance overhead if you encounter many invalid children repeatedly. Consider adding debug logs or metrics if that becomes an issue.
281-303: Add documentation clarifying the “Bitcoin seed” label in BIP32.Though the label “Bitcoin seed” is part of the BIP32 spec, your code appears to support multiple networks. Consider adding doc comments explaining that this string is a hard-coded requirement from BIP32, not a limitation to BTC usage alone.
375-432: Consider additional validation checks in the WIF class.You might want to confirm that the provided
versionaligns with known networks. Also consider verifying that the resulting base58 string meets expected length and structure to detect malformed WIF outputs early.lib/src/crypto/wallet_utils.dart (4)
6-7: Remove or utilize the newly added imports.The pipeline indicates these bip39 and hd_node imports are unused. If they are no longer necessary, remove them to avoid warnings; otherwise, ensure they’re actually referenced in the file.
🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 6-6: Unused import: 'package:walletkit_dart/src/wallet/bip39/bip39.dart'. Try removing the import directive.
[warning] 7-7: Unused import: 'package:walletkit_dart/src/wallet/bip32/hd_node.dart'. Try removing the import directive.
22-27: Add error handling strategy for partial failures inFuture.wait.With
eagerError: true, one failing call aborts the entire collection. Verify if you prefer a more robust approach (e.g., capturing partial results) before failing the full token info retrieval.
73-73: Enhance error message to include the actual length encountered.Improving this message can make troubleshooting easier. For instance: “Invalid public key length: $found. Expected 65 bytes (or 64 bytes without prefix).”
85-139: Remove or clarify large blocks of commented-out Ethereum utility code.Leaving extensive commented code may confuse future maintainers. If it’s deprecated, remove it; if it’s staged for reintroduction, add a note linking to the relevant issue or PR.
lib/src/domain/constants.dart (2)
47-56: Good approach to consolidate Bitcoin’s BIP32, BIP49, BIP84, and BIP86 in a single structure.Storing these prefix sets together improves maintainability. Ensure your tests or integration checks cover multi-sig usage for each prefix pair.
101-111: Alert about reusing prefixes for all LTC wallet paths.
LITECOIN_NETWORK_BIP_WK_COMPATIBILITYuses a single set of BIP version bytes across BIP32, BIP49, BIP84, etc. This can be confusing, so be sure to add clarity on why they’re duplicated or confirm if this is correct.lib/src/wallet/bip32/hd_wallet_type.dart (3)
79-107: Consider stronger path validation.
While the regex and switch logic handle many valid BIP32 paths, you might want to handle additional checks or parse partial paths more gracefully.
109-116: Validate change/index arguments inwithChangeAndIndex.
Consider asserting thatchange >= 0andindex >= 0to prevent negative or invalid derivation paths.
130-141: Tidy up duplication across path classes.
TheBip32HdDerivationPathAccountandBip32HdDerivationPathCoinTypeclasses duplicate some logic fromHdDerivationPath. You could consider a single specialized class hierarchy or factory to reduce maintenance overhead.lib/src/crypto/network_type.dart (1)
228-259: Storing large endpoint lists inline.
These endpoints might be better stored in a configuration file or fetched from a well-known source. This helps avoid frequent code changes when endpoints update.lib/src/crypto/utxo/utils/derivation.dart (2)
7-59: Remove or retain commented-out code.
This code block is fully commented out. Either remove it if it’s obsolete or leave a clear comment explaining its future use.
157-159: Provide network parameter for consistency.
deriveNodeomits an optionalnetwork. If you need network-aware derivations, consider adding aNetworkNodeInfoparameter for consistency.lib/src/crypto/utxo/utxo_analyzer.dart (1)
1-1: Remove unused import.The
dart:typed_dataimport is unused according to the pipeline warning.-import 'dart:typed_data';🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 1-1: Unused import: 'dart:typed_data'. Try removing the import directive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
lib/src/crypto/network_type.dart(6 hunks)lib/src/crypto/tron/entities/tron_transaction_utils.dart(7 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(28 hunks)lib/src/crypto/utxo/utils/derivation.dart(3 hunks)lib/src/crypto/utxo/utils/eurocoin_signing.dart(6 hunks)lib/src/crypto/utxo/utils/proof_of_payment.dart(6 hunks)lib/src/crypto/utxo/utils/send.dart(21 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(23 hunks)lib/src/crypto/wallet_utils.dart(3 hunks)lib/src/domain/constants.dart(3 hunks)lib/src/domain/entities/hd_wallet_type.dart(0 hunks)lib/src/domain/entities/node.dart(4 hunks)lib/src/wallet/bip32/hd_node.dart(1 hunks)lib/src/wallet/bip32/hd_wallet_type.dart(1 hunks)
💤 Files with no reviewable changes (1)
- lib/src/domain/entities/hd_wallet_type.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/crypto/utxo/utils/proof_of_payment.dart
- lib/src/domain/entities/node.dart
- lib/src/crypto/utxo/utils/send.dart
- lib/src/crypto/utxo/utils/eurocoin_signing.dart
🧰 Additional context used
🪛 GitHub Actions: TestSuite
lib/src/crypto/wallet_utils.dart
[warning] 1-1: Unused import: 'dart:convert'. Try removing the import directive.
[warning] 4-4: Unused import: 'package:convert/convert.dart'. Try removing the import directive.
[warning] 6-6: Unused import: 'package:walletkit_dart/src/wallet/bip39/bip39.dart'. Try removing the import directive.
[warning] 7-7: Unused import: 'package:walletkit_dart/src/wallet/bip32/hd_node.dart'. Try removing the import directive.
lib/src/crypto/utxo/utils/derivation.dart
[error] 71-71: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
[warning] 134-134: The declaration 'toBase58wkCompatibility' isn't referenced. Try removing the declaration of 'toBase58wkCompatibility'.
lib/src/crypto/utxo/utxo_analyzer.dart
[warning] 1-1: Unused import: 'dart:typed_data'. Try removing the import directive.
lib/src/crypto/network_type.dart
[error] 123-123: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
[error] 124-124: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 125-125: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 126-126: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 127-127: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 128-128: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart
[error] 150-150: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
🔇 Additional comments (27)
lib/src/crypto/wallet_utils.dart (1)
77-79: LGTM!Stripping off the
0x04prefix before hashing follows Ethereum address derivation conventions. No concerns here.lib/src/domain/constants.dart (2)
42-42:✅ Verification successful
Confirm “qq” prefix for Bitcoin Cash.
Appending “qq” to the BCH prefixes may have been an intentional addition; however, some BCH address schemes differ. Make sure it aligns with your code’s address parsing logic and recognized address formats.
🏁 Script executed:
#!/bin/bash # Searching references to BFS addresses or 'qq' usage in the codebase for correctness rg "\bqq\b" -A 5 -B 5Length of output: 1657
BCH "qq" Prefix Confirmation
After verifying the codebase, the constant in
lib/src/domain/constants.dartexplicitly defines the BCH native segwit prefixes as[ "1q", "qq" ], and no conflicting usage of these prefixes was found. Please ensure that the inclusion of"qq"aligns with your address parsing logic and recognized Bitcoin Cash address formats. If your design intentionally supports this variant, the usage is confirmed.
37-39:❓ Verification inconclusive
Verify Dogecoin address prefixes.
Dogecoin commonly uses 'D' for P2PKH. Confirm that 'A' and 'dc1' reflect consensus usage for P2SH and native SegWit, respectively. Some Dogecoin wallets adopt alternative prefixes, so verifying is crucial.
🏁 Script executed:
#!/bin/bash # Searching references to 'P2SH dogecoin' in the codebase or docs to confirm rg 'p2sh' -A 5 -B 5Length of output: 3404
Address Prefix Usage Verified – Confirm External Consensus
- The codebase consistently uses the defined Dogecoin address prefixes. In particular, usage in
lib/src/crypto/utxo/entities/payments/p2h.dartconfirms thatP2SH_PREFIX_DOGEis employed for compatibility purposes.- The legacy P2PKH prefix
'D'is correctly in place.- The constants for
P2SH_PREFIX_DOGE('A') andP2WPKH_PREFIX_DOGE('dc1') are actively used, matching our current implementation. However, because some Dogecoin wallets adopt alternative prefixes, please verify these values against the latest Dogecoin network specifications or official documentation to ensure they reflect the true consensus.lib/src/wallet/bip32/hd_wallet_type.dart (3)
12-13: Handle fallback for unknown HDWalletPurpose strings.
If no enum value matches the given string,firstWherewill throw aStateError. Consider providing a fallback or meaningful error, e.g. a default enum value or a more descriptive exception.Would you like me to generate a verification script to search for any unhandled strings in the codebase, or do you prefer to handle it manually?
143-196: Confirm path levels inHdDerivationPathWithPublic.
The constructor enforces at least 5 levels insplit. Ensure this meets your use cases (e.g.m/44'/0'/0'/0/0). If deeper levels are required (like custom sub-accounts), you may need more robust checks.
18-21:✅ Verification successful
Verify Tron coin type.
SLIP-0044 typically designates 195 for Tron, which seems correct. Ensure other parts of the codebase reference the same coin type for consistent derivation.
🌐 Web query:
Is coin type 195 officially assigned to Tron in SLIP-0044?💡 Result:
Yes, coin type 195 is officially assigned to Tron (TRX) under the SLIP-0044 standard. This is documented in multiple authoritative sources:
- The SLIP-0044 GitHub registry explicitly lists Tron with the identifier
195[9][11].- The
ex3-node-typesRust crate usesChainType::Tron = 195for BIP-44-compliant wallet derivations[1].- Cryptographic libraries like
bip_utilsandslip44(used by wallets and tools) defineTRON = 195for interoperability[4][16].- Exodus Wallet and other major platforms implement Tron addresses using the path
m/44'/195'/0'/0'/0'[13].This standard ensures Tron wallets and addresses follow a consistent derivation scheme compatible with BIP-44 hierarchical deterministic wallets.
Citations:
- 1: https://docs.rs/ex3-node-types/latest/ex3_node_types/chain/enum.ChainType.html
- 2: https://learnmeabitcoin.com/technical/keys/hd-wallets/derivation-paths/
- 3: https://bip-utils.readthedocs.io/_/downloads/en/stable/pdf/
- 4: https://bip-utils.readthedocs.io/en/stable/bip_utils/slip/slip44/slip44.html
- 5: https://docs.trezor.io/trezor-firmware/misc/coins-bip44-paths.html
- 6: https://www.bia.gov/sites/default/files/dup/assets/as-ia/oig/pdf/508%20Compliant%202021.08.11%20Seminole%20Tribe%20Gaming%20Compact.pdf
- 7: https://docs.rs/slip44/latest/slip44/enum.Coin.html
- 8: https://www.getty.edu/publications/virtuallibrary/temp/9780892366385.pdf
- 9: https://github.com/satoshilabs/slips/blob/master/slip-0044.md
- 10: https://www.doi.gov/sites/doi.opengov.ibmcloud.com/files/uploads/doi_decisions_085.pdf
- 11: https://github.com/satoshilabs/slips/blob/master/slip-0044.md?plain=1
- 12: https://www.armyresilience.army.mil/ard/images/pdf/Policy/AR%20215-1%20Military%20Morale,%20Welfare,%20and%20Recreation%20Programs%20and%20Nonappropriated%20Fund%20Instrumentalities%20%20.pdf
- 13: https://support.exodus.com/support/en/articles/8598933-derivation-paths-in-exodus
- 14: https://docs.turnkey.com/concepts/wallets
- 15: https://trustwallet.github.io/docc/documentation/walletcore/cointype/
- 16: https://pkg.go.dev/github.com/wealdtech/go-slip44
Confirmed Tron Coin Type (195) per SLIP-0044
The coin type 195 is correctly assigned to Tron (TRX) as stated in SLIP-0044. Please ensure that this same coin type is used consistently across all wallet derivation implementations.lib/src/crypto/network_type.dart (2)
218-222: Confirm usage of dust thresholds.
You added constants for BTC, none, and DOGE dust thresholds. Confirm they’re properly enforced within other logic (e.g. fee calculations).
403-430:✅ Verification successful
Verify Dogecoin coin type.
You’re usingcoinType: 3for Dogecoin. SLIP-0044 also references 3 for DOGE. Confirm usage is consistent throughout the codebase.
🌐 Web query:
Is coin type 3 officially assigned to Dogecoin in SLIP-0044?💡 Result:
Yes, coin type 3 is officially assigned to Dogecoin in the SLIP-0044 standard. This is consistently documented across multiple technical sources:
SLIP-0044 Specifications explicitly list Dogecoin with ID 3 in both the original GitHub documentation [6][8] and its implementation in libraries like
bip-utils[1].Wallet Implementations follow this standard:
- Exodus Wallet uses derivation path
m/44'/3'/0'/0/0for Dogecoin[11]- MetaMask's Snap API references coin type 3 for Dogecoin key derivation[10]
- Rust's
slip44crate explicitly definesDogecoinwithcoin_type: 3[2]Industry Practices like Coinbase's BIP-44 guide show Dogecoin’s path as
m/44'/3'/0'/0/0[4], while technical articles about HD wallets confirm this assignment[12].This standardization ensures compatibility across wallets and tools using hierarchical deterministic key generation.
Citations:
- 1: https://bip-utils.readthedocs.io/en/stable/bip_utils/slip/slip44/slip44.html
- 2: https://docs.rs/slip44/latest/slip44/enum.Coin.html
- 3: https://pro-blockchain.com/en/dogecoin-jumps-10-as-new-doge-government-website-adopts-dogecoin-logo
- 4: https://www.coinbase.com/blog/https-medium-com-the-coinbase-blog-capture-the-coin-blockchain-category
- 5: https://www.mitrade.com/insights/news/live-news/article-3-705203-20250319
- 6: https://github.com/satoshilabs/slips/blob/master/slip-0044.md
- 7: https://www.barchart.com/story/news/30560458/why-dogecoin-is-sinking-today
- 8: https://github.com/satoshilabs/slips/blob/master/slip-0044.md?plain=1
- 9: https://news.va.gov/138326/va-secretary-doug-collins-veterans-benefits/
- 10: https://docs.metamask.io/snaps/reference/snaps-api/
- 11: https://support.exodus.com/support/en/articles/8598933-derivation-paths-in-exodus
- 12: https://learnmeabitcoin.com/technical/keys/hd-wallets/derivation-paths/
- 13: https://trustwallet.github.io/docc/documentation/walletcore/cointype/
Dogecoin Coin Type Verified
The coin type of 3 is correct and officially assigned to Dogecoin as per SLIP-0044, and its usage here is consistent with the standard and industry practices. No changes are required.lib/src/crypto/utxo/utils/derivation.dart (1)
133-139: Unused method warning.
toBase58wkCompatibilityis not referenced, according to the pipeline warning. If unneeded, remove it or add usage in the codebase to avoid dead code.🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 134-134: The declaration 'toBase58wkCompatibility' isn't referenced. Try removing the declaration of 'toBase58wkCompatibility'.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (8)
25-30: Excellent architectural improvement with abstract weight getter.Converting
weightto an abstract getter allows subclasses to implement their own weight calculation logic, making the design more flexible and extensible.
187-211: Good implementation of weight calculation.The new weight implementation properly handles SegWit transactions and calculates weight based on inputs and outputs. This is critical for accurate fee estimation.
209-211: Clear isSegwit property implementation.The isSegwit property provides a clean way to determine if a transaction contains SegWit inputs.
507-510: Good weight implementation for EC8 transactions.The weight calculation for EC8 transactions follows a different pattern than BTC, and this implementation properly reflects that difference.
40-40: Fee per byte calculation improvement.The addition of the
feePerBytegetter provides a convenient way to calculate fee rates, which is useful for fee optimization and comparison with network fee recommendations.
63-71: Enhanced type safety with BTCLockingScript.Changing the parameter type from
Uint8Listto a more specificBTCLockingScripttype improves type safety and makes the code more maintainable.
405-415: Added assertion for hashType validation.Adding assertions for hash type validation is a good defensive programming practice that helps catch errors early.
669-671: Consider improving input selection logic.
singleWhereOrNullassumes only one input has a non-empty script, which might not work in multi-input signing scenarios.lib/src/crypto/tron/entities/tron_transaction_utils.dart (2)
5-5: Updated import for crypto utilities.The import has been changed from
pubkey_to_address.darttocrypto.dart, which is part of the broader refactoring to consolidate crypto utilities.
181-185: Improved signature creation formatting.The vertical alignment of parameters in
createSignatureimproves readability.lib/src/crypto/utxo/utxo_analyzer.dart (8)
9-9: Updated import for HDNode.Importing
HDNodefrom the new path is part of the transition away from the BIP32 package.
173-173: Parameter type updated to use HDNode.Changing from
walletTypestoaccountLevelHdNodesaligns with the broader refactoring to use the new HDNode class.
199-216: Improved code structure for transaction search.The updated approach for searching transactions using HDNode is more maintainable and consistent with the new wallet architecture.
235-235: Updated parameter type in searchTransactionsForWalletType.Using HDNode instead of BipNode improves type consistency across the codebase.
271-271: Updated parameter type in searchForTransactions.Using HDNode instead of BIP32 aligns with the broader refactoring.
404-404: Added useSmartFee option for fee estimation.The new parameter allows for more advanced fee estimation strategies.
420-420: Improved fee calculation method.Using multiplication instead of division for fee calculation can help avoid floating-point precision issues.
607-611: Simplified getSpendableOutputs with list comprehension.The updated implementation is more concise and readable.
| const DOGECOIN_NETWORK_BIP = NetworkBIP( | ||
| bip32: (private: 0x02fac398, public: 0x02facafd), | ||
| bip49: (private: 0x02fac398, public: 0x02facafd), | ||
| bip84: (private: 0x02fac398, public: 0x02facafd), | ||
| bip49MultiSig: (private: 0x02fac398, public: 0x02facafd), | ||
| bip84MultiSig: (private: 0x02fac398, public: 0x02facafd), | ||
| bip86: (private: 0x045f18bc, public: 0x045f1cf6), // trprv, trpub | ||
| bip86MultiSig: (private: 0x02575483, public: 0x02575048), // Trprv, Trpub | ||
| wif: 0x9e, // WIF prefix for Dogecoin | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check DOGECOIN extended key prefixes.
It’s worth confirming that these prefix values are recognized by major Dogecoin wallets. If they’re non-standard, add a note clarifying usage limitations.
| const LTC_NETWORK_BIP = NetworkBIP( | ||
| bip32: (private: 0x019D9CFE, public: 0x019DA462), // Ltub, Ltpv | ||
| bip49: (private: 0x01B26792, public: 0x01B26EF6), // Mtpv, Mtub | ||
| bip84: (private: 0x04B2430C, public: 0x04B24746), // zprv, zpub | ||
| bip49MultiSig: (private: 0x01B26792, public: 0x01B26EF6), // Mtpv, Mtub | ||
| bip84MultiSig: (private: 0x02AA7A99, public: 0x02AA7ED3), // Zprv, Zpub | ||
| bip86: (private: 0x045f18bc, public: 0x045f1cf6), // trprv, trpub | ||
| bip86MultiSig: (private: 0x02575483, public: 0x02575048), // Trprv, Trpub | ||
| wif: 0xB0, // 176 | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check the LTC extended key prefixes.
Litecoin’s extended keys sometimes deviate from BTC’s standard. Add references or comment on official LTC proposals to confirm correctness. This avoids confusion for maintainers who expect certain version bytes.
🏁 Script executed:
#!/bin/bash
rg 'LTC_NETWORK_BIP'Length of output: 282
Double-check LTC Extended Key Prefixes – Add Documentation References
The constants in lib/src/domain/constants.dart (lines 59-68) appear to be correctly referenced in tests (e.g., in test/ci/bip32/bip32-dart_test.dart), and they align with typical Litecoin extended key usage. However, because Litecoin’s extended key prefixes deviate from Bitcoin’s standard values, it would improve maintainability to include inline comments or documentation references (such as links to the official Litecoin documentation or Litecoin Improvement Proposals) that confirm the origin and correctness of these version bytes.
| const ethereumBip44HDPath = Bip32HdDerivationPathCoinType( | ||
| purpose: HDWalletPurpose.BIP44, | ||
| coinType: 44, | ||
| ); |
There was a problem hiding this comment.
Potential coin type mismatch for Ethereum.
Ethereum is generally assigned coin type 60 in SLIP-0044. Using 44 might cause wallet compatibility issues.
- coinType: 44,
+ coinType: 60,📝 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 ethereumBip44HDPath = Bip32HdDerivationPathCoinType( | |
| purpose: HDWalletPurpose.BIP44, | |
| coinType: 44, | |
| ); | |
| const ethereumBip44HDPath = Bip32HdDerivationPathCoinType( | |
| purpose: HDWalletPurpose.BIP44, | |
| coinType: 60, | |
| ); |
| NetworkNodeInfo getForPurpose(HDWalletPurpose purpose) => switch (purpose) { | ||
| HDWalletPurpose.NO_STRUCTURE || | ||
| HDWalletPurpose.BIP44 => NetworkNodeInfo(wif: wif, keyPrefixes: bip32), | ||
| HDWalletPurpose.BIP49 => NetworkNodeInfo(wif: wif, keyPrefixes: bip49), | ||
| HDWalletPurpose.BIP84 => NetworkNodeInfo(wif: wif, keyPrefixes: bip84), | ||
| HDWalletPurpose.BIP86 => NetworkNodeInfo(wif: wif, keyPrefixes: bip86), | ||
| }; |
There was a problem hiding this comment.
Missing import for HDWalletPurpose.
The pipeline failures show that HDWalletPurpose is undefined here. Add an import to reference hd_wallet_type.dart or define HDWalletPurpose before usage.
+ import 'package:walletkit_dart/src/wallet/bip32/hd_wallet_type.dart';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NetworkNodeInfo getForPurpose(HDWalletPurpose purpose) => switch (purpose) { | |
| HDWalletPurpose.NO_STRUCTURE || | |
| HDWalletPurpose.BIP44 => NetworkNodeInfo(wif: wif, keyPrefixes: bip32), | |
| HDWalletPurpose.BIP49 => NetworkNodeInfo(wif: wif, keyPrefixes: bip49), | |
| HDWalletPurpose.BIP84 => NetworkNodeInfo(wif: wif, keyPrefixes: bip84), | |
| HDWalletPurpose.BIP86 => NetworkNodeInfo(wif: wif, keyPrefixes: bip86), | |
| }; | |
| import 'package:walletkit_dart/src/wallet/bip32/hd_wallet_type.dart'; | |
| NetworkNodeInfo getForPurpose(HDWalletPurpose purpose) => switch (purpose) { | |
| HDWalletPurpose.NO_STRUCTURE || | |
| HDWalletPurpose.BIP44 => NetworkNodeInfo(wif: wif, keyPrefixes: bip32), | |
| HDWalletPurpose.BIP49 => NetworkNodeInfo(wif: wif, keyPrefixes: bip49), | |
| HDWalletPurpose.BIP84 => NetworkNodeInfo(wif: wif, keyPrefixes: bip84), | |
| HDWalletPurpose.BIP86 => NetworkNodeInfo(wif: wif, keyPrefixes: bip86), | |
| }; |
🧰 Tools
🪛 GitHub Actions: TestSuite
[error] 123-123: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
[error] 124-124: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 125-125: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 126-126: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 127-127: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
[error] 128-128: Undefined name 'HDWalletPurpose'. Try correcting the name to one that is defined, or defining the name.
| NodeWithAddress deriveNodeFromSeed({ | ||
| required Uint8List seed, | ||
| required HDWalletPath walletPath, | ||
| UTXONetworkType? networkType, | ||
| }) { | ||
| final bipNetworkType = | ||
| networkType?.networkBIP.getForWalletType(walletPath.purpose); | ||
|
|
||
| final parentNode = BipNode.fromSeed(seed, bipNetworkType); | ||
| final derivationPath = switch (walletPath.basePath) { | ||
| "m/44'/2'" => walletPath.account0Path, | ||
| _ => walletPath.purpose.string, | ||
| }; | ||
| final node = | ||
| parentNode.derivePath(derivationPath); // TODO: Use base Path with Account | ||
|
|
||
| return node; | ||
| } | ||
|
|
||
| BipNode deriveMasterNodeFromExtendedKeyWithCheck({ | ||
| required String ePubKey, | ||
| required UTXONetworkType networkType, | ||
| required String path, | ||
| required HDWalletPurpose purpose, | ||
| required UTXONetworkType networkType, | ||
| required Iterable<AddressType> addressTypes, | ||
| }) { |
There was a problem hiding this comment.
HDWalletPurpose reference missing.
Line 71 uses HDWalletPurpose but the pipeline log says it’s undefined, indicating a missing import of hd_wallet_type.dart.
Also applies to: 75-78
🧰 Tools
🪛 GitHub Actions: TestSuite
[error] 71-71: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
| RawTransaction sign({ | ||
| required Uint8List seed, | ||
| required HDWalletPath walletPath, | ||
| required HDWalletPurpose purpose, |
There was a problem hiding this comment.
Fix undefined class error.
The pipeline is failing because HDWalletPurpose is not defined. You need to import the class or define it.
Run the following script to find where this class might be defined:
#!/bin/bash
# Search for HDWalletPurpose definition in the codebase
rg -t dart "class HDWalletPurpose" --include "*.dart"
# Search for potential imports
rg -t dart "HDWalletPurpose" --include "*.dart" | grep "import"🧰 Tools
🪛 GitHub Actions: TestSuite
[error] 150-150: Undefined class 'HDWalletPurpose'. Try changing the name to the name of an existing class, or creating a class with the name 'HDWalletPurpose'.
| throw UnimplementedError(); | ||
| // final node = deriveNode(seed, tronBip44HDPath.defaultPath); | ||
| // return (node.privateKey!, node.publicKeyUncompressed); |
There was a problem hiding this comment.
Unimplemented getTronCredentials function.
The function has been disabled with an UnimplementedError, which needs to be addressed before release.
This function appears to have been using a BIP32 node that's no longer available. Implement using the new HDNode approach:
(Uint8List, Uint8List) getTronCredentials({required Uint8List seed}) {
- throw UnimplementedError();
- // final node = deriveNode(seed, tronBip44HDPath.defaultPath);
- // return (node.privateKey!, node.publicKeyUncompressed);
+ final node = HDNode.fromSeed(seed).derivePath(tronBip44HDPath.defaultPath);
+ return (node.privateKey, node.publicKeyUncompressed);
}📝 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.
| throw UnimplementedError(); | |
| // final node = deriveNode(seed, tronBip44HDPath.defaultPath); | |
| // return (node.privateKey!, node.publicKeyUncompressed); | |
| (Uint8List, Uint8List) getTronCredentials({required Uint8List seed}) { | |
| final node = HDNode.fromSeed(seed).derivePath(tronBip44HDPath.defaultPath); | |
| return (node.privateKey, node.publicKeyUncompressed); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/src/utils/var_uint.dart (1)
3-17: De-duplicate logic and add input guard for varIntLengthThis repeats encoding rules already in encodingLength and lacks a non-negative check.
Apply this diff to delegate to encodingLength and guard negatives:
extension VarLengthUtil on int { - int get varIntLength { - if (this < 0xfd) { - return 1; - } - if (this <= 0xffff) { - return 3; - } - if (this <= 0xffffffff) { - return 5; - } - - return 9; - } + /// Returns the number of bytes required to varint-encode this unsigned value. + /// Throws [ArgumentError] if negative. + int get varIntLength { + if (this < 0) { + throw ArgumentError('varint requires a non-negative integer'); + } + return encodingLength(this); + } }lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (2)
291-309: Updated witness script handling.The witness parsing has been updated to use the new
ScriptWitnessclass and proper script addition viaaddScript(). The variable namingwittnessScriptshas a typo but the logic is sound.Apply this diff to fix the typo:
- List<(BTCUnlockingScript, BTCInput)> wittnessScripts = []; + List<(BTCUnlockingScript, BTCInput)> witnessScripts = [];And update the reference:
- wittnessScripts.add((witness, input)); + witnessScripts.add((witness, input));And:
- for (final (wittnessScript, input) in wittnessScripts) { + for (final (witnessScript, input) in witnessScripts) {
688-695: Consistent byte calculation formatting.The byte length calculation formatting has been made consistent with other similar calculations in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(28 hunks)lib/src/utils/var_uint.dart(4 hunks)
🔇 Additional comments (22)
lib/src/utils/var_uint.dart (4)
98-109: Align with existing read methods: return (value, bytesRead) and fix endianness*Current signature/behavior is inconsistent (returns int only, takes Endian). Also, “length” actually represents the varint prefix. Use little-endian consistently.
Apply this diff:
- int readVarIntFromLength(int offset, int length, Endian endian) { - if (length < 0xfd) { - return getUint8(offset); - } - if (length == 0xfd) { - return getUint16(offset, endian); - } - if (length == 0xfe) { - return getUint32(offset, endian); - } - return getUint64(offset, endian); - } + /// Reads a varint's value from [offset] based on the already-read prefix byte. + /// [offset] must point to the first byte of the value (immediately after the prefix). + /// Returns (value, bytesReadForValue). + (int, int) readVarIntFromLength(int offset, int prefix) { + if (prefix < 0xfd) { + // Value is the prefix itself; no additional bytes read. + return (prefix, 1); + } + if (prefix == 0xfd) { + return (getUint16(offset, Endian.little), 2); + } + if (prefix == 0xfe) { + return (getUint32(offset, Endian.little), 4); + } + return (getUint64(offset, Endian.little), 8); + }Optional: consider renaming to readVarIntFromPrefix for clarity.
133-136: LGTM: readability improvementMulti-line call improves clarity without behavior change.
150-154: LGTM: no behavior changeTernary formatting change only; logic preserved.
156-167: Duplicate of encodingLength — remove or document differenceThis repeats encodingLength’s logic. Prefer a single source of truth.
Apply this diff to remove the duplicate:
-int getVarIntSize(int value) { - if (value < 0xfd) { - return 1; - } - if (value <= 0xffff) { - return 3; - } - if (value <= 0xffffffff) { - return 5; - } - return 9; -}Use encodingLength(value) or value.varIntLength instead.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (18)
4-4: Import oftx_structure.dartadded.This import brings in the new Bitcoin script structures that are now being used throughout the file.
24-30: Good abstraction with weight and virtual size calculations.The transition from concrete implementation to abstract getter for
weightallows for proper subclass-specific implementations, and thevSizecalculation correctly implements the SegWit virtual size formula.
40-40: Fee per byte calculation added.The
feePerBytecalculation provides a useful metric for transaction efficiency analysis.
62-72: Improved signature with proper type safety.The
legacySigHashmethod now usesBTCLockingScriptinstead of a generic parameter and includes network type validation with assertion. This provides better type safety and prevents invalid signature configurations.
77-81: Script handling updated to use new structure.The script handling now properly uses the new
BTCLockingScriptandEmptyUnlockingScriptclasses, which provides better type safety and cleaner abstraction.
186-207: Excellent weight calculation implementation for Bitcoin transactions.The weight calculation properly accounts for:
- Base transaction data (8 bytes * 4 for version + locktime)
- SegWit marker and flag (2 bytes * 4 when SegWit is present)
- Input and output weights from their respective calculations
This implementation correctly follows the BIP 141 weight calculation formula.
209-211: Good SegWit detection logic.The
isSegwithelper method correctly identifies SegWit transactions by checking if any input is SegWit-enabled.
263-278: Improved parsing with readVarInt.The transaction parsing now uses the more robust
readVarIntmethod instead of manual parsing, which handles variable-length integers correctly.
325-346: Improved byte calculation with proper variable length handling.The byte calculation now uses
varIntLengthfor input and output counts, which correctly accounts for variable-length integer encoding based on the actual count values.
352-356: SegWit input size calculation updated.The calculation now uses
input.script!.sizeinstead of direct field access, which is consistent with the new script structure. The assertion ensures type safety.
407-423: Enhanced BIP143 signature with proper validation.The BIP143 signature hash method now includes proper network type validation and uses
BTCLockingScriptfor better type safety. The assertion prevents invalid hash type usage.
456-456: Script bytes access updated.The output script access now uses
.bytesproperty, which is consistent with the new script structure.
508-512: EC8 weight calculation implementation.The EC8 weight calculation sums input and output weights directly, which appears to be the correct approach for this transaction type based on the different weight model.
523-531: Consistent variable length handling in EC8 transactions.The EC8 transaction byte calculation follows the same pattern as Bitcoin transactions, using
varIntLengthfor proper encoding of input and output counts.Also applies to: 537-546
654-654: Input signing logic maintains existing behavior.The
bytesForSigningmethod call with explicit parameters maintains the existing signing logic while adapting to the new input interface.
675-675: Single input detection logic maintained.This preserves the existing logic for identifying which input needs signing in multi-input scenarios. The past review comment about potential issues with multi-input signing is noted but this change doesn't alter that behavior.
755-770: EC8 parsing updated with readVarInt.The EC8 transaction parsing now uses
readVarIntconsistently, matching the improvements made to Bitcoin transaction parsing.
148-162: Critical issue: HDWalletPurpose is undefined.The
HDWalletPurposeclass is not defined anywhere in the codebase, causing compilation failures. This needs to be resolved before the code can function.Based on the search results and past review comments, this issue has been identified previously but remains unresolved. The class needs to be either:
- Imported from the correct location
- Defined within this codebase
- Replaced with an existing equivalent type
#!/bin/bash # Search for HDWalletPurpose definition or similar purpose/derivation path classes rg -t dart "class.*Purpose" --include "*.dart" -A 5 -B 2 rg -t dart "HDWallet" --include "*.dart" -A 3 -B 2 rg -t dart "derivation.*path\|wallet.*purpose" --include "*.dart" -i
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.md(1 hunks)example/pubspec.yaml(1 hunks)lib/src/crypto/network_type.dart(9 hunks)lib/src/wallet/OVERVIEW_REPORT.md(1 hunks)lib/src/wallet/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- lib/src/wallet/README.md
- AGENTS.md
- lib/src/wallet/OVERVIEW_REPORT.md
🔇 Additional comments (1)
lib/src/crypto/network_type.dart (1)
143-160: Add the missing BIP86 lookups infromVersion.
fromVersionnever returns thebip86/bip86MultiSigprefixes, so any BIP86 extended key version falls through tonull, breaking the new purpose support you added. Please wire those cases in.if (version == bip84MultiSig.private || version == bip84MultiSig.public) { return bip84MultiSig; } + if (version == bip86.private || version == bip86.public) { + return bip86; + } + if (version == bip86MultiSig.private || version == bip86MultiSig.public) { + return bip86MultiSig; + } return null; }
| @@ -0,0 +1 @@ | |||
| # nocterm: ^0.1.0 No newline at end of file | |||
There was a problem hiding this comment.
Don’t comment out the new dependency
Housing the nocterm entry in a comment means Pub ignores it, so the example app still lacks the package and any code using it will fail to compile. Please add it as a real dependency under the appropriate section.
-# nocterm: ^0.1.0
+ nocterm: ^0.1.0📝 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.
| # nocterm: ^0.1.0 | |
| nocterm: ^0.1.0 |
🤖 Prompt for AI Agents
In example/pubspec.yaml around lines 1 to 1, the nocterm dependency is commented
out so Pub ignores it; remove the leading '#' and add nocterm: ^0.1.0 under the
appropriate dependencies section (or create a dependencies: block if missing) so
the package is declared as a real dependency and the example app can compile.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores