Fix Segwit Fee Calculation #127
Conversation
WalkthroughThis update introduces a new smart fee estimation method within the ElectrumXClient alongside enhancements to fee calculation logic and logging in UTXO transaction functions. The modifications include changes to transaction input handling with refined witness size and weight calculations, abstract getters in raw transactions, and adaptations in fee estimation methods in the UTXO analyzer to support a new smart fee option. Additionally, a utility function to compute variable-length integer sizes is added, and new tests verify smart fee estimation across multiple networks. Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer as UTXO Analyzer
participant Client as ElectrumXClient
Analyzer->>Client: estimateFeeForPriority(blocks, ..., useSmartFee=true)
alt Use Smart Fee
Client->>Client: estimateSmartFee(blocks)
else Use Legacy Fee
Client->>Client: estimateFee(blocks)
end
Client-->>Analyzer: Fee estimation result
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart (1)
218-228: Consider enhancing error handling for fee estimation.While the implementation is correct, the error handling could be more informative. The current implementation throws a generic "Fee estimation failed" message for both null and zero fee cases.
Consider this improved implementation:
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) throw Exception("Fee estimation failed: received null response"); + if (fee == 0) throw Exception("Fee estimation failed: received zero fee"); return fee; }lib/src/crypto/utxo/utils/send.dart (1)
Line range hint
218-228: Consider documenting fee estimation strategy.With the addition of
estimateSmartFee, the codebase now has two methods for fee estimation. Consider documenting:
- The differences between
estimateFeeandestimateSmartFee- When to use each method
- The trade-offs between them
This will help maintain consistency in fee estimation across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart(1 hunks)lib/src/crypto/utxo/utils/send.dart(1 hunks)
🔇 Additional comments (1)
lib/src/crypto/utxo/utils/send.dart (1)
596-599: LGTM! Efficient handling of client availability.
The addition of this condition is a good optimization that prevents unnecessary iterations when no clients are available for rebroadcasting.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/ci/gasfees_test.dart (1)
11-19: Consider enhancing test validation and reducing duplication.The tests follow a good pattern but could benefit from the following improvements:
- Add specific assertions for fee ranges to ensure reasonable values.
- Compare smart fee with regular fee to validate the difference.
- Extract common test logic to reduce duplication.
Consider refactoring to a shared test function:
Future<void> testNetworkFees(UTXONetworkType network, String networkName) async { final gasEntity = await getNetworkFees(network: network); expect(gasEntity, isNotNull); expect(gasEntity.nextBlock.value > BigInt.zero, true, reason: '$networkName next block fee should be positive'); print('$networkName standard fees: $gasEntity'); final smartGasEntity = await getNetworkFees( network: network, useSmartFee: true, ); expect(smartGasEntity, isNotNull); expect(smartGasEntity.nextBlock.value > BigInt.zero, true, reason: '$networkName smart fee should be positive'); print('$networkName smart fees: $smartGasEntity'); } void main() { test('Estimate Fees BTC', () => testNetworkFees(BitcoinNetwork, 'BTC')); test('Estimate Fees LTC', () => testNetworkFees(LitecoinNetwork, 'LTC')); test('Estimate Fees BCH', () => testNetworkFees(BitcoincashNetwork, 'BCH')); test('Estimate Fees Zeniq', () => testNetworkFees(ZeniqNetwork, 'ZENIQ')); }Also applies to: 28-36, 45-53, 62-70
lib/src/crypto/utxo/utxo_analyzer.dart (3)
493-498: Document the smart fee parameter and improve error handling.The smart fee implementation looks good, but consider these improvements:
- Add documentation explaining the difference between smart fee and regular fee estimation.
- Consider handling specific error cases from the ElectrumX server.
Add documentation and improve error handling:
Future<Amount> estimateFeeForPriority({ required int blocks, required UTXONetworkType network, required ElectrumXClient? initalClient, - bool useSmartFee = false, + /// When true, uses the ElectrumX estimatesmartfee method which provides more accurate + /// fee estimation based on recent block history and mempool state. + /// When false, uses the basic fee estimation method. + bool useSmartFee = false, }) async { final (fee, _, error) = await fetchFromRandomElectrumXNode( (client) => useSmartFee ? client.estimateSmartFee(blocks: blocks) : client.estimateFee(blocks: blocks), client: initalClient, endpoints: network.endpoints, token: network.coin, ); - if (fee == null) throw Exception("Fee estimation failed"); + if (fee == null) { + throw Exception("Fee estimation failed: ${error ?? 'Unknown error'}"); + }
508-508: Consider adding bounds checking for fee calculation.The fee calculation could benefit from minimum/maximum bounds to prevent unreasonable values.
- final feePerB = feePerKb.multiplyAndCeil(0.001); + final feePerB = feePerKb.multiplyAndCeil(0.001); + // Ensure fee is within reasonable bounds (e.g., 1-1000 sat/byte) + if (feePerB.value < BigInt.from(1) || feePerB.value > BigInt.from(1000)) { + Logger.logWarning("Fee outside reasonable bounds: ${feePerB.value} sat/byte"); + }
516-516: Document the multiplier parameter's purpose and constraints.The multiplier parameter needs documentation to explain its purpose and valid range.
Future<UtxoNetworkFees> getNetworkFees({ required UTXONetworkType network, - double multiplier = 1.0, + /// Fee multiplier to adjust the estimated fees. + /// Must be greater than 0. Values > 1 increase fees, values < 1 decrease fees. + /// @throws ArgumentError if multiplier <= 0 + double multiplier = 1.0, bool useSmartFee = false, }) async { + if (multiplier <= 0) { + throw ArgumentError.value(multiplier, 'multiplier', 'Must be greater than 0'); + }lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (2)
40-41: Fee per byte division by zero check
Since size can theoretically be zero for certain edge cases, consider either bounding or verifying that size is non-zero before dividing. Otherwise, a potential runtime exception may occur.-double get feePerByte => fee.toInt() / size; +double get feePerByte { + if (size == 0) return 0; + return fee.toInt() / size; +}
509-520: Summation-based weight in EC8
EC8RawTransaction weight simply accumulates input + output weights. Confirm that additional overhead fields (version, validFrom, validUntil, etc.) don’t also need weighting. To avoid confusion, add a small comment or a helper function to incorporate overhead if applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/crypto/utxo/entities/raw_transaction/input.dart(4 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(4 hunks)lib/src/crypto/utxo/utils/send.dart(3 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(2 hunks)lib/src/utils/var_uint.dart(1 hunks)test/ci/gasfees_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/crypto/utxo/utils/send.dart
🔇 Additional comments (7)
lib/src/crypto/utxo/entities/raw_transaction/input.dart (2)
46-46: Abstract getter is well-structured
Declaring "BigInt get weight;" in the base class ensures that each subclass correctly implements its own weight logic. Nicely done.
239-239: Dynamic size calculation for scriptSig
Using getVarIntSize(scriptSig.length) is a good approach to handle variable-length integers. The usage here appears correct and more future-proof than a fixed-size approach.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (4)
24-26: Abstract getter design
Defining "BigInt get weight;" as an abstract in RawTransaction cleanly enforces the specialized logic in each subclass. Good job providing a strong contract for transaction classes.
27-29: vSize calculation
Dividing weight by 4 matches standard meal-of-bytes for vSize. This logic is correct for standard Bitcoin usage.
213-215: isSegwit logic
Checking return inputs.any((input) => input.isSegwit); is concise. If partial Segwit usage is expected, you may want to confirm that all inputs must be segwit to treat the transaction as segwit.
194-211: Verify the combined weight calculation
You're adding a base overhead (line 197) if isSegwit is true, then summing input/output weights. Ensure that the overhead properly accounts for all fields (including marker & flag) and that you don’t double-count them in the individual inputs.
✅ Verification successful
Let me gather more information about the weight calculation by checking the input and output weight implementations.
Let me check one more thing about the script weight calculation.
Let me check the script weight calculation implementation.
Based on the comprehensive review of the weight calculation implementation, I can now provide the final response:
Weight calculation implementation is correct
The transaction weight calculation properly accounts for all components:
- Base transaction overhead (version + locktime) * 4
- Segwit marker and flag overhead (2 bytes * 4) when applicable
- Input weights are calculated separately and include their script weights
- Output weights are calculated separately with their script weights
- Script weight calculation properly handles different opcodes with appropriate weights
The implementation follows the BIP-141 weight calculation rules, and there is no double-counting of segwit fields as the marker and flag are only added once at the transaction level, while individual input weights are calculated independently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Cross-check references to isSegwit and the segwit overhead calculation
rg -A 5 "isSegwit"
Length of output: 6078
Script:
#!/bin/bash
# Check input weight calculation
ast-grep --pattern 'get weight {
$$$
}'
# Check for any weight-related constants
rg "weight" -B 2 -A 2
Length of output: 17028
Script:
#!/bin/bash
# Check for script weight calculation
ast-grep --pattern 'getScriptWeight'
rg "getScriptWeight"
Length of output: 473
Script:
#!/bin/bash
# Check script weight calculation implementation
cat lib/src/crypto/utxo/entities/script.dart
Length of output: 3931
lib/src/utils/var_uint.dart (1)
124-136: New function getVarIntSize
This function aligns with variable integer encoding logic (BIP141 style). Consider adding validation for negative values to avoid potential edge cases. Otherwise, it looks correct.
| BigInt get witnessSize { | ||
| if (_wittnessScript == null || _wittnessScript!.isEmpty) return 0.toBI; | ||
|
|
||
| BigInt size = getVarIntSize(_wittnessScript!.length) | ||
| .toBI; // Count of witness elements | ||
|
|
||
| for (final element in witness!) { | ||
| size += getVarIntSize(element.length).toBI; // Size of this element | ||
| size += element.length.toBI; // The element itself | ||
| } | ||
|
|
||
| return size; |
There was a problem hiding this comment.
Potential missing 'witness' property reference and spelling concern
It looks like line 38 refers to "for (final element in witness!)", but this property or getter named "witness" is not defined within the class. This might be an unintentional reference, a leftover from a refactor, or a missing getter. Also note that the internal field is spelled "_wittnessScript" instead of "_witnessScript." Consider renaming and ensuring code references match.
| @override | ||
| BigInt get weight { | ||
| if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI; | ||
| return calculateWeight(_prevScriptPubKey!, _scriptSig!); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid returning negative weight
Returning -1.toBI from the weight getter might lead to unexpected behavior if the rest of the codebase assumes weight is non-negative. A better approach might be to return 0.toBI or throw an exception if the data is insufficient to compute the weight.
- if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI;
+ if (_scriptSig == null || _prevScriptPubKey == null) {
+ // Consider returning 0 or throwing an informative exception
+ return 0.toBI;
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| @override | ||
| BigInt get weight { | ||
| BigInt weight = (txid.length + output_index_length + sequence_length).toBI * | ||
| 4.toBI; // (32 + 4 + 4) * 4 | ||
|
|
||
| weight += | ||
| (scriptSig.length + getVarIntSize(scriptSig.length)).toBI * 4.toBI; | ||
|
|
||
| if (_wittnessScript != null) { | ||
| weight += | ||
| (_wittnessScript!.length + getVarIntSize(_wittnessScript!.length)) | ||
| .toBI * | ||
| 4.toBI; | ||
| } | ||
|
|
||
| return weight; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Weight calculation for witness data needs correction
Based on the code review and search results, the current implementation incorrectly applies a weight multiplier of 4 to witness data (_wittnessScript). According to the Segwit specification (BIP141):
- Non-witness data (txid, output_index, sequence, scriptSig) should have a weight of 4 units per byte
- Witness data (_wittnessScript) should have a weight of 1 unit per byte
The code needs to be modified to multiply _wittnessScript length by 1 instead of 4.
Locations needing attention:
lib/src/crypto/utxo/entities/raw_transaction/input.dart: Change the witness data weight calculation from4.toBIto1.toBI
🔗 Analysis chain
Verify correctness of Segwit weighting
Current logic multiplies all scriptSig and witness data lengths by 4. Typically, non-witness data is counted as 4 weight units/byte, while witness data is 1 weight unit/byte. Mixing them at 4 might incorrectly inflate the transaction weight. Confirm or correct the weighting to accurately reflect Segwit’s weighting rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for weight calculations that multiply witness data by 4
rg -A 3 "weight.*=.*(_wittnessScript|witnessSize).*4"
Length of output: 55
Script:
#!/bin/bash
# Let's search for all weight-related calculations in the codebase
rg -A 5 "weight.*=.*\*.*4" lib/
Length of output: 868
Script:
#!/bin/bash
# Let's also check for any BIP documentation or comments about Segwit weight calculation
rg -i "segwit.*weight|weight.*segwit" lib/
Length of output: 285
Script:
#!/bin/bash
# Let's look at the entire input class implementation to understand the context better
ast-grep --pattern 'class Input {
$$$
}'
Length of output: 44
Script:
#!/bin/bash
# Let's also check if there are any test files validating the weight calculation
fd -e dart test_weight -x cat {}
fd -e dart weight_test -x cat {}
Length of output: 73
Script:
#!/bin/bash
# Let's try a different pattern to find the Input class implementation
ast-grep --pattern 'class Input {
BigInt get weight {
$$$
}
}'
Length of output: 72
Script:
#!/bin/bash
# Let's also check for any test files related to transaction weight or segwit
rg -l "test.*weight|test.*segwit" --type dart
Length of output: 47
Script:
#!/bin/bash
# Let's look for any documentation about weight calculation in comments
rg -B 2 -A 2 "weight.*=|weight.*{" --type dart
Length of output: 150
…o segwit_fee_fix
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/ci/fetching/gasfees_test.dart (3)
5-71: Consider refactoring tests to reduce duplication.The test structure is identical across all networks, which presents an opportunity to improve maintainability through parameterized testing.
Consider refactoring using a helper function and test parameters:
void main() { final networks = [ BitcoinNetwork, LitecoinNetwork, BitcoincashNetwork, ZeniqNetwork, ]; Future<void> testNetworkFees(Network network) async { test('Estimate Fees ${network.runtimeType}', () async { final gasEntity = await getNetworkFees(network: network); expect(gasEntity, isNotNull); print(gasEntity); final smartGasEntity = await getNetworkFees( network: network, useSmartFee: true, ); expect(smartGasEntity, isNotNull); print(smartGasEntity); }); } for (final network in networks) { testNetworkFees(network); } }
8-8: Enhance test assertions for better coverage.The current assertions only verify that the returned entities are not null. Consider adding more specific assertions to validate:
- The structure of the returned entities
- The relationship between standard and smart fee values
- The range of acceptable fee values
Example of enhanced assertions:
expect(gasEntity.fee, isPositive); expect(gasEntity.fee, lessThan(maxExpectedFee)); expect(smartGasEntity.fee, isPositive); expect(smartGasEntity.fee, lessThan(maxExpectedFee)); // Optionally, verify smart fee is within a reasonable range of standard fee expect((smartGasEntity.fee - gasEntity.fee).abs(), lessThan(maxFeeDifference));Also applies to: 17-17, 25-25, 34-34, 42-42, 51-51, 59-59, 68-68
10-10: Consider enhancing test documentation and error handling.The tests would benefit from:
- Using a proper logging framework instead of print statements
- Adding documentation about expected behavior
- Including error case scenarios
Example improvements:
// Add test documentation /// Tests fee estimation for different networks. /// Expected behavior: /// - Standard fees should reflect current network conditions /// - Smart fees should provide more accurate estimates for different priorities /// - Both methods should handle network errors gracefully // Add error case tests test('Handle network errors gracefully', () async { // Test with invalid network expect( () => getNetworkFees(network: invalidNetwork), throwsA(isA<NetworkException>()), ); // Test with network timeout expect( () => getNetworkFees(network: network, timeout: Duration.zero), throwsA(isA<TimeoutException>()), ); }); // Use proper logging import 'package:logging/logging.dart'; final _logger = Logger('GasFeesTest'); // ... _logger.info('Standard fee result: $gasEntity');Also applies to: 19-19, 27-27, 36-36, 44-44, 53-53, 61-61, 70-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart(1 hunks)lib/src/crypto/utxo/utils/send.dart(3 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(2 hunks)lib/src/utils/var_uint.dart(1 hunks)test/ci/fetching/gasfees_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart
- lib/src/utils/var_uint.dart
- lib/src/crypto/utxo/utils/send.dart
- lib/src/crypto/utxo/utxo_analyzer.dart
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
lib/src/crypto/utxo/utils/pubkey_to_address.dart (1)
130-133: Function name typo and missing documentationThe newly added function has a name typo - it should be "ripemd160Hash" instead of "ripmed160Hash" to match the standard naming convention for this hash algorithm. Also, unlike other hash functions in this file, this one lacks a documentation comment.
-Uint8List ripmed160Hash(Uint8List buffer) { +/// +/// Ripmed160 Hash +/// +Uint8List ripemd160Hash(Uint8List buffer) { final ripmed160 = RIPEMD160Digest(); return ripmed160.process(buffer); }Note: Since this function is likely being added to support segwit fee calculation, ensure any references to this function name are updated if you decide to fix the typo.
lib/src/crypto/utxo/entities/op_codes.dart (1)
153-155: Useful utility method, but needs documentation.The new
fromHexstatic method provides a helpful way to look up an opcode by its hex value, which is valuable for the Segwit fee calculation improvements. However, it would benefit from documentation explaining its purpose and behavior.+ /// Returns the OPCODE enum value corresponding to the given hex value, + /// or null if no matching opcode is found. static OPCODE? fromHex(int hex) { return OPCODE.values.singleWhereOrNull((element) => element.hex == hex); }test/ci/raw_transaction/btc_raw_transaction_test.dart (1)
1-14: Fix unused import and variable warningsThere are two pipeline warnings in this test file:
- The import on line 2 for
input.dartis unused- The
txvariable created on line 9 is not used anywhereApply these changes to fix the warnings:
-import 'package:walletkit_dart/src/crypto/utxo/entities/raw_transaction/input.dart';And either add assertions to use the
txvariable or remove it:- final tx = BTCRawTransaction( + // Create a BTC transaction (just verifying it constructs properly) + BTCRawTransaction(🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 2-2: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/raw_transaction/input.dart'. Try removing the import directive.
[warning] 9-9: The value of the local variable 'tx' isn't used. Try removing the variable or using it.
lib/src/crypto/utxo/utils/proof_of_payment.dart (2)
5-7: Remove unused importThere's an unused import of
op_codes.dartthat can be removed according to the pipeline warning.-import 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart';🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 5-5: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart'. Try removing the import directive.
94-104: Improved script creation for OP_RETURNThe code now correctly uses the
OPReturnScriptclass instead of manually constructing a script with OP_RETURN. This approach is more structured and aligns with the refactoring done in the codebase for script handling.Consider adding a comment explaining why the size is calculated as
2 + 32 + nonceBytes.length + 1for better maintainability:- final pop_output_script_data = Uint8List(2 + 32 + nonceBytes.length + 1); + // Size: 2 bytes (version) + 32 bytes (txid) + variable length nonce + 1 byte (VarInt for nonce) + final pop_output_script_data = Uint8List(2 + 32 + nonceBytes.length + 1);lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart (1)
541-549: Check whether additional fields should be preserved.
This extension only mapsvalueandscriptPubKey.lockingScriptinto aBTCOutput, ignoring otherElectrumOutputfields likebelongsToUs,spent, ornode. If those fields are relevant downstream, consider including them or documenting why they’re omitted.lib/src/crypto/utxo/entities/raw_transaction/input.dart (1)
32-47: Consider removing or finalizing commented-out witness code.
Leaving large blocks of commented code can create confusion. If no longer needed, remove them. If still relevant, reintroduce them or add clearer TODO comments.lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (3)
40-41: Potential precision concern infeePerByte.
Castingfeetointcould overflow with very large fees, and using adoublemay lose precision.
299-300: Minor naming inconsistency.
Consider renamingwittnessScripts→witnessScriptsfor clarity.-List<(BTCUnlockingScript, BTCInput)> wittnessScripts = []; +List<(BTCUnlockingScript, BTCInput)> witnessScripts = [];
413-416: Support for onlySIG_HASH_ALLis documented.
If you plan to add other types (e.g., SIGHASH_SINGLE), ensure the code handles them gracefully.lib/src/crypto/utxo/entities/raw_transaction/tx_structure.dart (2)
434-446:OPReturnScript
Limits data to 80 bytes. Suitable for typical OP_RETURN usage, but double-check any protocol requiring smaller OP_RETURN.
504-526:SegwitFormat
UnimplementedbuildBufferagain. Mark or track TODO so it’s not forgotten.Do you want a helper function or a follow-up commit to implement it fully?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/src/crypto/utxo/entities/op_codes.dart(2 hunks)lib/src/crypto/utxo/entities/payments/p2h.dart(0 hunks)lib/src/crypto/utxo/entities/payments/pk_script_converter.dart(0 hunks)lib/src/crypto/utxo/entities/raw_transaction/input.dart(10 hunks)lib/src/crypto/utxo/entities/raw_transaction/output.dart(7 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(14 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/utils/proof_of_payment.dart(2 hunks)lib/src/crypto/utxo/utils/pubkey_to_address.dart(1 hunks)lib/src/crypto/utxo/utils/send.dart(13 hunks)lib/src/domain/entities/generic_transaction.dart(1 hunks)test/ci/proof_of_payment/pop_test.dart(12 hunks)test/ci/raw_transaction/btc_raw_transaction_test.dart(1 hunks)test/no_ci/input_simulation_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/src/crypto/utxo/entities/payments/p2h.dart
- lib/src/crypto/utxo/entities/payments/pk_script_converter.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 (1)
- lib/src/crypto/utxo/utils/send.dart
🧰 Additional context used
🪛 GitHub Actions: TestSuite
test/ci/raw_transaction/btc_raw_transaction_test.dart
[warning] 2-2: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/raw_transaction/input.dart'. Try removing the import directive.
[warning] 9-9: The value of the local variable 'tx' isn't used. Try removing the variable or using it.
lib/src/crypto/utxo/utils/proof_of_payment.dart
[warning] 5-5: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart'. Try removing the import directive.
lib/src/crypto/utxo/entities/raw_transaction/input.dart
[warning] 6-6: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'. Try removing the import directive.
[warning] 7-7: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart'. Try removing the import directive.
[warning] 10-10: Unused import: 'package:walletkit_dart/walletkit_dart.dart'. Try removing the import directive.
[warning] 80-80: The value of the local variable 'redeemScript' isn't used. Try removing the variable or using it.
lib/src/crypto/utxo/entities/raw_transaction/output.dart
[warning] 6-6: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'. Try removing the import directive.
🔇 Additional comments (72)
lib/src/crypto/utxo/entities/op_codes.dart (3)
5-5: Good import addition for required functionality.The collection package provides the necessary
singleWhereOrNullextension method used in the newfromHexmethod.
153-155: Consider handling duplicate hex values consistently.There are cases in the enum where multiple opcodes share the same hex value (e.g., OP_0/OP_FALSE, OP_NOP2/OP_CHECKLOCKTIMEVERIFY). The current implementation will return the first matching opcode, which may not always be the expected one.
Consider whether this behavior is acceptable for your use case. If not, you might need to implement a more sophisticated lookup strategy or document this limitation.
3-3: Existing TODO should be addressed in a future PR.There's an existing TODO to refactor this class for better readability. While not related to the current changes, this technical debt should be tracked and addressed in a future PR.
test/no_ci/input_simulation_test.dart (2)
308-309: Property name change from scriptPubKey to script looks goodCorrectly updated BTCOutput to use the new
scriptproperty name instead ofscriptPubKey.
312-313: Property name change from scriptPubKey to script looks goodCorrectly updated EC8Output to use the new
scriptproperty name instead ofscriptPubKey.test/ci/proof_of_payment/pop_test.dart (12)
52-53: Property access pattern updated correctlyThe code has been correctly updated to access script data via the new structure.
74-75: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
133-134: Property access pattern updated correctlyScript data access has been correctly updated to use the new structure.
155-156: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
197-198: Property access pattern updated correctlyScript data access has been correctly updated to use the new structure.
219-220: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
265-266: Property access pattern updated correctlyScript data access has been correctly updated to use the new structure.
287-288: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
348-349: Property access pattern updated correctlyScript data access has been correctly updated to use the new structure.
370-371: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
426-427: Property access pattern updated correctlyScript data access has been correctly updated to use the new structure.
448-449: Property access pattern updated correctlyInput script access has been correctly updated to use the new structure.
lib/src/crypto/utxo/entities/transactions/utxo_transaction.dart (1)
509-510: Consider handling invalid hex strings.
IfhexStringis malformed or empty,hex.decode(hexString)will throw. It may be prudent to add an assertion or error-handling mechanism to prevent runtime exceptions.lib/src/crypto/utxo/entities/raw_transaction/output.dart (12)
14-14: Typed script property looks good.
Replacing a rawUint8Listwith aBTCLockingScriptclarifies responsibility for script parsing and is consistent with the new design.
18-18: Simplified scriptHex getter is clear.
This change helps unify script representation with the rest of the codebase.
26-27: Validate the feasibility of script-based weight.
Ensure thatscript.weightcorrectly accounts for witness vs non-witness data if the script can be Segwit. Otherwise, this approach might over/under-calculate transaction weight.
30-30: Constructor update is consistent.
Requiringscriptexplicitly is a good move toward more robust, non-nullable design.
48-49: Validate reading the script from the buffer.
Double-check offset correctness infromBuffer(). Any mismatch can corrupt subsequent reads.
53-53: ConstructingBTCLockingScriptfrom buffer is a great improvement.
Typing the script data asBTCLockingScriptclarifies its usage and prevents confusion with raw byte slices.
58-58: Allocate buffer capacity carefully.
value_length + script.size + 1works, but ensure that script size plus the VarInt prefix reliably fits. If script becomes large, you might need additional length checks to avoid RangeError.
66-66: VarInt usage is correct here.
Writing the script length withwriteVarSlice()is a standard approach. No issues spotted.
80-82: Confirm weight logic for EC8Output.
Allocatingvalue_length + weight_length + script.size + 1might be correct for your protocol, but be sure you’re not ignoring SegWit specifics if intended.
108-109: Edge case: zero weight
Here, the code sets weight to 0. If a script is actually standard or non-segwit, the weight might differ. Confirm this approach is correct for your scenario, or handle potential future expansions.
111-112: Check for sufficient space.
Writing the script to the buffer requires the allocated length to accommodate both the VarInt prefix and the script bytes. Any mismatch can cause an out-of-bounds error.
133-134: Ensure consistent usage ofBTCLockingScript.fromBuffer().
This matches the design pattern used inBTCOutput.fromBuffer(). Good consistency across your outputs.lib/src/crypto/utxo/entities/raw_transaction/input.dart (6)
19-23: New fields strongly typed.
IntroducingprevOutputandscriptclarifies the relation between an input and its previous output. This is a solid improvement toward structured data handling.
55-55: Weight as an abstract getter is fine, but be careful with unimplemented states.
Any subclass must implement this carefully to avoid negative or inaccurate weight, especially with SegWit rules.
74-75: hasWitness logic.
script is ScriptWitnessis a simplified check. Ensure no edge cases exist if the script’s type changes or if other script kinds might also hold witness data.
77-82: EnsurepublicKeyFromSigcovers all script variants.
Currently, the switch throws forRedeemScriptand unknown scripts. Confirm this is intentional or if more integration with multi-sig or advanced scripts is required.🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 80-80: The value of the local variable 'redeemScript' isn't used. Try removing the variable or using it.
85-85: addScript(...) signature is clear.
This pattern of returning a new instance with updated script fosters immutability.
132-134: NullableprevOutputin factory constructor.
Currently,prevOutputisnullupon creation. Confirm a follow-up mechanism is in place to set the actual reference if needed, or clarify if a separate path is used for non-lookup scenarios.lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (11)
4-4: No issues with the new import.
It references needed classes fromtx_structure.dart.
24-25: Good introduction of abstractweight.
Makingweightabstract ensures each subclass implements its own specialized calculation.
27-29:vSizecalculation is standard for Bitcoin-like transactions.
Integer division by 4 matches typical SegWitvSize = weight / 4.
194-215: Verify weight components include varint overhead.
Although adding base fields and segwit markers is correct, ensure input/output counts’ varints are accounted for ininput.weight/output.weight. Otherwise, weight may be slightly underestimated.
217-219:isSegwitproperty is straightforward.
Treating the transaction as SegWit if one input is SegWit aligns with typical Bitcoin node logic.
308-319: Check boundary conditions when reading witness data.
If the buffer is malformed or an input is not found ininputs, code may throw or incorrectly parse.
424-426: Enforced assertion forSIGHASH_ALL.
This is a helpful runtime check, but consider returning a descriptive exception.
430-430: Forcibly creating a P2PKH script fromprevScript.data.
Validate or guard ifprevScriptis not truly P2PKH to avoid unexpected behavior.
464-464: Writing script bytes for outputs.
This looks consistent with typical varint + script buffer format.
470-471: Index bounds check recommended.
Ensureindex < inputs.lengthbefore referencinginputs[index].
481-481: Appending p2pkh script is correct for standard signatures.
No immediate issues.lib/src/crypto/utxo/entities/raw_transaction/tx_structure.dart (25)
1-13: Imports look appropriate.
All imported packages appear valid for address encoding/decoding and script handling.
14-22:InputTypeenum covers major script types.
It’s a clear, organized approach for classification.
23-34: BaseBTCScriptclass usesgetScriptWeightexternally.
EnsuregetScriptWeightis defined and tested. The added+1might slightly overcount weight.
35-163:BTCLockingScripthandles multiple address formats.
Implementation covers P2PK, P2PKH, P2SH, P2WPKH, etc. Ensure more exotic cases or edge addresses throw early. Watch out for the BCH block wherebitcoincash:is transformed into a statement akin to P2WPKH—verify correctness if truly intended.
165-178:BTCUnlockingScriptbase.
Factory approach for script types is neat; watch for unknown or unhandled leading bytes.
179-203:RedeemScriptstructure is consistent.
Parses signature and script correctly for P2SH. No critical issues identified.
204-223:ScriptSignaturedesign is fine for typical P2PKH unlocking.
Double-check large public keys or non-standard scriptSig lengths.
225-245:ScriptWitnessdesign.
Implementation appears aligned with SegWit witness data. Confirm no mismatch in lengths if there are multiple pushes.
247-271:TimeLockedScriptfor CSV/CLTV logic.
Check for corner cases around large lockTime or negative indexes. Currently it’s a single byte for lockTime.
273-287:PayToPublicKeyScript
Straightforward. Just confirm the last op is indeedOP_CHECKSIG.
289-306:PayToPublicKeyHashScript
Implementation matches standard OP_DUP / OP_HASH160 structure. Looks correct.
308-323:PayToScriptHashScript
Good usage ofOP_HASH160. The sublist constraints are correct.
325-339:PayToWitnessPublicKeyHashScript
Standard P2WPKH structure recognized. Ensure length checks for unusual data.
341-355:PayToWitnessScriptHashScript
Similarly standard for P2WSH. Confirm the 32-byte script hash is always present.
357-363:NestedSegwitScript
Holds a nested script hash. If used in place of real P2SH-wrapped segwit, ensure it’s tested.
364-379:PayToWitnessPublicKeyHashNestedScript
Implements nested P2WPKH. Check theripmed160Hashusage for correctness.
380-395:PayToWitnessScriptHashNestedScript
Creates a nested P2WSH usingsha256Hash. Check that the script is always 32 bytes.
397-409:PayToTaprootScript
Implementation is minimal.OP_1indicates v1 witness. Confirm a 32-byte key is enforced.
448-450:AnyoneCanSpendScript
Just setsOP_TRUE. This is a typical dev/test convenience script.
452-454:EmptyScript
Represents a zero-length script. No issues identified.
456-458:EmptyUnlockingScript
Same idea—script length zero for unlocking. Looks correct for placeholders.
460-462:EmptyLockingScript
Again, zero-length script for placeholders.
464-484:BTCTransactionStructure
A sealed base class for transaction formats. Thecreatefactory is unimplemented, so ensure usage is appropriate.
486-502:LegacyFormat
EmptybuildBuffersuggests incomplete or placeholder logic. Ensure final usage is implemented.
528-541:TaprootFormat
ExtendsSegwitFormatwith a new version and extra input type. ThebuildBufferis also unimplemented.
| test( | ||
| "Legacy BTC Raw Transaction Test", | ||
| () async { | ||
| final tx = BTCRawTransaction( | ||
| version: BitcoinNetwork.txVersion, | ||
| lockTime: 0, | ||
| inputs: [], | ||
| outputs: [], | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test is incomplete - missing assertions
This test creates a BTCRawTransaction but doesn't verify any of its properties or behavior. This makes the test ineffective for detecting regressions.
Add assertions to verify the transaction properties, especially those related to Segwit fee calculation which is the focus of this PR:
final tx = BTCRawTransaction(
version: BitcoinNetwork.txVersion,
lockTime: 0,
inputs: [],
outputs: [],
);
+
+ // Verify basic properties
+ expect(tx.version, equals(BitcoinNetwork.txVersion));
+ expect(tx.lockTime, equals(0));
+ expect(tx.inputs, isEmpty);
+ expect(tx.outputs, isEmpty);
+
+ // Verify Segwit-related properties
+ expect(tx.isSegwit, isFalse);
+ expect(tx.hasWitness, isFalse);
+ expect(tx.weight, equals(tx.size * 4)); // Non-Segwit tx weight = size * 4📝 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.
| test( | |
| "Legacy BTC Raw Transaction Test", | |
| () async { | |
| final tx = BTCRawTransaction( | |
| version: BitcoinNetwork.txVersion, | |
| lockTime: 0, | |
| inputs: [], | |
| outputs: [], | |
| ); | |
| }, | |
| ); | |
| test( | |
| "Legacy BTC Raw Transaction Test", | |
| () async { | |
| final tx = BTCRawTransaction( | |
| version: BitcoinNetwork.txVersion, | |
| lockTime: 0, | |
| inputs: [], | |
| outputs: [], | |
| ); | |
| // Verify basic properties | |
| expect(tx.version, equals(BitcoinNetwork.txVersion)); | |
| expect(tx.lockTime, equals(0)); | |
| expect(tx.inputs, isEmpty); | |
| expect(tx.outputs, isEmpty); | |
| // Verify Segwit-related properties | |
| expect(tx.isSegwit, isFalse); | |
| expect(tx.hasWitness, isFalse); | |
| expect(tx.weight, equals(tx.size * 4)); // Non-Segwit tx weight = size * 4 | |
| }, | |
| ); |
🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 9-9: The value of the local variable 'tx' isn't used. Try removing the variable or using it.
| @@ -2,6 +2,7 @@ import 'dart:typed_data'; | |||
|
|
|||
| import 'package:convert/convert.dart'; | |||
| import 'package:walletkit_dart/src/crypto/utxo/entities/raw_transaction/input.dart'; | |||
| import 'package:walletkit_dart/src/crypto/utxo/entities/raw_transaction/tx_structure.dart'; | |||
| import 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'; | |||
There was a problem hiding this comment.
Remove the unused import.
The pipeline flagged import 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'; as unused. Please remove it to satisfy lint rules and keep the codebase clean.
-import 'package:walletkit_dart/src/crypto/utxo/entities/script.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.
| import 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'; |
🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 6-6: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'. Try removing the import directive.
| import 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'; | ||
| import 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart'; |
There was a problem hiding this comment.
Remove or comment out the unused imports.
The pipeline flagged these imports as unused:
package:walletkit_dart/src/crypto/utxo/entities/script.dartpackage:walletkit_dart/src/crypto/utxo/entities/op_codes.dartpackage:walletkit_dart/walletkit_dart.dart
Please remove them or use them to satisfy lint rules.
-import 'package:walletkit_dart/src/crypto/utxo/entities/script.dart';
-import 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart';
-import 'package:walletkit_dart/walletkit_dart.dart';Also applies to: 10-10
🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 6-6: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/script.dart'. Try removing the import directive.
[warning] 7-7: Unused import: 'package:walletkit_dart/src/crypto/utxo/entities/op_codes.dart'. Try removing the import directive.
| return switch (script) { | ||
| ScriptSignature scripSig => scripSig.publicKey, | ||
| ScriptWitness scriptWitness => scriptWitness.publicKey, | ||
| RedeemScript redeemScript => throw Exception("Redeem Script"), |
There was a problem hiding this comment.
Remove the unused 'redeemScript' variable if it still exists.
The pipeline indicates a local redeemScript is declared but unused around line 80. Consider removing or using it to avoid warnings.
🧰 Tools
🪛 GitHub Actions: TestSuite
[warning] 80-80: The value of the local variable 'redeemScript' isn't used. Try removing the variable or using it.
Summary by CodeRabbit
New Features
ElectrumOutputclass with a new method for converting toBTCOutput.Refactor
Tests
BTCRawTransactionclass to validate instantiation.