Draft: adjust EVM feeInformation #123
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EvmRpcInterface
participant FeeEstimator
User->>EvmRpcInterface: buildUnsignedTransaction(null, null)
EvmRpcInterface->>FeeEstimator: estimateNetworkFees()
FeeEstimator-->>EvmRpcInterface: gasPrice, gasLimit
EvmRpcInterface-->>User: Transaction built with gasPrice and gasLimit
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
lib/src/domain/entities/fee.dart (1)
Line range hint
93-99: Incorrect fee calculation inEvmLegacyFeeInformationThe
feegetter inEvmLegacyFeeInformationcurrently returnsgasPriceor defaults to zero, which does not accurately reflect the total transaction fee. The correct calculation should multiply thegasPriceby thegasLimitto compute the total fee.Apply this diff to correct the fee calculation:
Amount get fee { - return gasPrice ?? - Amount.zero * Amount.convert(value: gasLimit ?? 0, decimals: 0); + return (gasPrice ?? Amount.zero) * + Amount( + value: BigInt.from(gasLimit ?? 0), + decimals: 0, + ); }lib/src/domain/entities/tx_gasFee_entity.dart (1)
87-98: Clarify variable naming in JSON deserializationIn the factory constructor, the JSON key
'suggestBaseFee'is assigned to the variablelast. For better clarity, consider renaminglasttosuggestBaseFeeto match the JSON key.Apply this diff to improve readability:
if (json case { - 'suggestBaseFee': String last, + 'suggestBaseFee': String suggestBaseFee, 'SafeGasPrice': String safe, 'ProposeGasPrice': String propose, 'FastGasPrice': String fast, }) { final safe_num = toGwei(safe); final propose_num = toGwei(propose); final fast_num = toGwei(fast); - final last_num = toGwei(last); + final baseFee_num = toGwei(suggestBaseFee); return EvmNetworkFees( - lastBlock: last_num, + lastBlock: baseFee_num, safe: safe_num, average: propose_num, fast: fast_num, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(2 hunks)lib/src/domain/entities/fee.dart(3 hunks)lib/src/domain/entities/tx_gasFee_entity.dart(3 hunks)
🔇 Additional comments (2)
lib/src/domain/entities/fee.dart (1)
81-90: Appropriate use of sealed classes for fee information
Changing EvmFeeInformation from a final class to a sealed class allows controlled subclassing within the same file, which is suitable for this design.
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (1)
Line range hint 219-233: Ensure accurate gas estimation in buildUnsignedTransaction
The updated logic in the switch expression handles scenarios where gasPrice and gasLimit may be null. When both are null, estimateNetworkFees is called to obtain estimates. Ensure that estimateNetworkFees provides accurate and efficient estimates to avoid transaction failures due to incorrect gas settings.
lib/src/domain/entities/fee.dart
Outdated
| return maxFeePerGas ?? | ||
| Amount.zero * Amount.convert(value: gasLimit ?? 0, decimals: 0); | ||
| } |
There was a problem hiding this comment.
Incorrect fee calculation in EvmType2FeeInformation
Similar to EvmLegacyFeeInformation, the fee getter does not compute the total transaction fee correctly. It should multiply maxFeePerGas by gasLimit to calculate the fee accurately.
Apply this diff to correct the fee calculation:
Amount get fee {
- return maxFeePerGas ??
- Amount.zero * Amount.convert(value: gasLimit ?? 0, decimals: 0);
+ return (maxFeePerGas ?? Amount.zero) *
+ Amount(
+ value: BigInt.from(gasLimit ?? 0),
+ decimals: 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.
| return maxFeePerGas ?? | |
| Amount.zero * Amount.convert(value: gasLimit ?? 0, decimals: 0); | |
| } | |
| return (maxFeePerGas ?? Amount.zero) * | |
| Amount( | |
| value: BigInt.from(gasLimit ?? 0), | |
| decimals: 0, | |
| ); | |
| } |
| 'maxPriorityFeePerGas': maxPriorityFeePerGas.toString(), | ||
| 'gasPrice': gasPrice?.toJson() ?? Amount.zero.toJson(), | ||
| 'maxFeePerGas': maxFeePerGas.toString(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent serialization in EvmType2FeeInformation.toJson()
In EvmType2FeeInformation.toJson(), maxPriorityFeePerGas and maxFeePerGas are serialized using toString(), whereas in EvmLegacyFeeInformation, gasPrice is serialized using toJson(). This inconsistency may lead to issues during deserialization. Consider serializing using toJson() for consistency.
Apply this diff to ensure consistent serialization:
Json toJson() {
return {
'gasLimit': gasLimit,
- 'maxPriorityFeePerGas': maxPriorityFeePerGas.toString(),
- 'maxFeePerGas': maxFeePerGas.toString(),
+ 'maxPriorityFeePerGas': maxPriorityFeePerGas?.toJson() ?? Amount.zero.toJson(),
+ 'maxFeePerGas': maxFeePerGas?.toJson() ?? Amount.zero.toJson(),
};
}Committable suggestion skipped: line range outside the PR's diff.
| BigInt get safePriority => safe - lastBlock; | ||
| BigInt get averagePriority => average - lastBlock; | ||
| BigInt get fastPriority => fast - lastBlock; | ||
|
|
There was a problem hiding this comment.
Potential negative values in priority fee calculations
The getters safePriority, averagePriority, and fastPriority subtract lastBlock from safe, average, and fast respectively. If safe, average, or fast are less than lastBlock, this could result in negative values, which may not be valid.
Consider ensuring the result is non-negative:
BigInt get safePriority => (safe - lastBlock).isNegative ? BigInt.zero : safe - lastBlock;
BigInt get averagePriority => (average - lastBlock).isNegative ? BigInt.zero : average - lastBlock;
BigInt get fastPriority => (fast - lastBlock).isNegative ? BigInt.zero : fast - lastBlock;📝 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.
| BigInt get safePriority => safe - lastBlock; | |
| BigInt get averagePriority => average - lastBlock; | |
| BigInt get fastPriority => fast - lastBlock; | |
| BigInt get safePriority => (safe - lastBlock).isNegative ? BigInt.zero : safe - lastBlock; | |
| BigInt get averagePriority => (average - lastBlock).isNegative ? BigInt.zero : average - lastBlock; | |
| BigInt get fastPriority => (fast - lastBlock).isNegative ? BigInt.zero : fast - lastBlock; | |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/domain/entities/fee.dart(3 hunks)lib/src/domain/entities/transfer_intent.dart(2 hunks)
🔇 Additional comments (6)
lib/src/domain/entities/transfer_intent.dart (2)
72-74: LGTM: Fee calculation logic updated correctly
The switch expression correctly handles the new fee structure, using maxFee for EVM transactions and maintaining existing behavior for other cases.
30-30: Verify the impact of switching from fee to maxFee
The change from fee to maxFee for EVM transactions aligns with the new fee structure, but we should verify this change doesn't impact existing transaction calculations.
✅ Verification successful
Let me gather more specific information about the usage of EvmFeeInformation to better understand the impact.
Based on the comprehensive search results, I can now provide the final response:
The change from fee to maxFee is correct and safe
The switch from fee to maxFee in TransferIntent is a proper alignment with the EVM fee structure. The evidence shows:
maxFeeis already the established getter inEvmFeeInformationclass that correctly calculates fees based on gas parameters- All EVM-based transaction tests (Arbitrum, Optimism, Avalanche, etc.) are using
TransferIntent<EvmFeeInformation>and rely on this fee calculation - The change maintains consistency with EVM Type 2 transactions that use
maxFeePerGasfor fee calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of the old `fee` property from EvmFeeInformation
rg -l "evmFeeInformation.*\.fee" --type dart
# Search for any fee calculations that might be affected
rg "calculateFee|computeFee|estimateFee" --type dart
Length of output: 1274
Script:
#!/bin/bash
# Search for EvmFeeInformation class definition and usage
ast-grep --pattern 'class EvmFeeInformation'
# Search for any references to maxFee in the codebase
rg "maxFee" --type dart -C 2
# Search for TransferIntent usage
rg "TransferIntent" --type dart -C 2
Length of output: 27474
lib/src/domain/entities/fee.dart (4)
81-89: Good architectural decision using sealed class
The use of sealed for EvmFeeInformation ensures type safety and exhaustive pattern matching while allowing for different EVM fee types. The abstract maxFee getter provides a unified interface for fee calculations.
132-142:
Same fee calculation issue in EvmType2FeeInformation
The maxFee calculation in EvmType2FeeInformation has the same potential overflow issue as in EvmLegacyFeeInformation.
Apply similar overflow protection as suggested for EvmLegacyFeeInformation.
153-154:
Inconsistent serialization detected
The toString() usage for fee serialization differs from other Amount serializations in the codebase which use toJson().
This issue was previously identified in an earlier review. Please apply the suggested fix to ensure consistent serialization.
94-102:
Verify gas calculation overflow protection
The gas fee calculation could potentially overflow with large values. Consider adding overflow checks or using SafeMath patterns.
Consider adding overflow protection:
Amount? get maxFee {
if (gasPrice == null) {
return null;
}
if (gasLimit == null) {
return null;
}
- return gasPrice! * Amount.convert(value: gasLimit!, decimals: 0);
+ try {
+ final gasLimitAmount = Amount.convert(value: gasLimit!, decimals: 0);
+ if (gasLimitAmount.value <= BigInt.zero || gasPrice!.value <= BigInt.zero) {
+ return null;
+ }
+ return gasPrice! * gasLimitAmount;
+ } catch (e) {
+ return null;
+ }
}
lib/src/domain/entities/fee.dart
Outdated
| EvmLegacyFeeInformation( | ||
| gasLimit: gasLimit, | ||
| gasPrice: Amount.fromJson(gasPrice), | ||
| ), | ||
| { | ||
| 'gasLimit': int gasLimit, | ||
| 'maxFeePerGas': Map maxFeePerGas, | ||
| 'maxPriorityFeePerGas': Map maxPriorityFeePerGas, | ||
| } => | ||
| EvmType2FeeInformation( | ||
| gasLimit: gasLimit, | ||
| maxFeePerGas: Amount.fromJson(maxFeePerGas), | ||
| maxPriorityFeePerGas: Amount.fromJson(maxPriorityFeePerGas), | ||
| ), |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results from multiple verification attempts, I can now generate a final response:
Add validation for gasLimit and Amount values
The codebase shows no existing validation for gasLimit or Amount values in the FeeInformation class. Since gas values are critical for transaction execution, validation should be added:
- gasLimit must be positive and within network limits
- Amount values should be non-negative
- Add validation in the factory constructor
factory FeeInformation.fromJson(Map json) {
return switch (json) {
{
'gasLimit': int gasLimit,
'gasPrice': Map gasPrice,
} =>
+ gasLimit <= 0 ? throw FormatException('Invalid gasLimit: must be positive') :
EvmLegacyFeeInformation(
gasLimit: gasLimit,
gasPrice: Amount.fromJson(gasPrice),
),
{
'gasLimit': int gasLimit,
'maxFeePerGas': Map maxFeePerGas,
'maxPriorityFeePerGas': Map maxPriorityFeePerGas,
} =>
+ gasLimit <= 0 ? throw FormatException('Invalid gasLimit: must be positive') :
EvmType2FeeInformation(
gasLimit: gasLimit,
maxFeePerGas: Amount.fromJson(maxFeePerGas),
maxPriorityFeePerGas: Amount.fromJson(maxPriorityFeePerGas),
),🔗 Analysis chain
Verify type safety in fromJson factory
The factory method should validate the numeric ranges for gasLimit and ensure Amount values are non-negative.
Add validation:
factory FeeInformation.fromJson(Map json) {
return switch (json) {
{
'gasLimit': int gasLimit,
'gasPrice': Map gasPrice,
} =>
+ gasLimit <= 0 ? throw FormatException('Invalid gasLimit') :
EvmLegacyFeeInformation(
gasLimit: gasLimit,
gasPrice: Amount.fromJson(gasPrice),
),
{
'gasLimit': int gasLimit,
'maxFeePerGas': Map maxFeePerGas,
'maxPriorityFeePerGas': Map maxPriorityFeePerGas,
} =>
+ gasLimit <= 0 ? throw FormatException('Invalid gasLimit') :
EvmType2FeeInformation(
gasLimit: gasLimit,
maxFeePerGas: Amount.fromJson(maxFeePerGas),
maxPriorityFeePerGas: Amount.fromJson(maxPriorityFeePerGas),
),🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar factory methods to ensure consistent validation
rg "fromJson.*gasLimit|fromJson.*Amount" --type dart
# Look for existing validation patterns
rg "validate.*gas|validate.*amount" --type dart
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search for Amount validation patterns
rg "Amount\.fromJson" -A 5 --type dart
# Search for any validation in factory methods
rg "factory.*fromJson" -A 10 --type dart
# Look for gas-related validations with broader patterns
rg "gas.*throw|invalid.*gas" --type dart
Length of output: 112122
Script:
#!/bin/bash
# Let's look at the Amount class implementation to understand its validation
ast-grep --pattern 'class Amount {
$$$
}'
# Search for any gas validation patterns in the codebase
rg "gasLimit.*<|gasLimit.*>|gasLimit.*=|gasLimit.*if" --type dart -A 2
Length of output: 6468
Script:
#!/bin/bash
# Let's look for Amount validation patterns
rg "Amount.*value.*<|Amount.*value.*>|Amount.*value.*if|Amount.*throw" --type dart -A 2
# Check for any existing gas validation patterns in the codebase
rg "gas.*validate|validate.*gas" --type dart -A 2
# Look for any existing validation in the FeeInformation class
ast-grep --pattern 'class FeeInformation {
$$$
}'
Length of output: 1796
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
lib/src/crypto/evm/entities/contract/internal_contract.dart (1)
42-42: Add documentation for fee handling behaviorWith
feeInfobecoming optional, it would be helpful to document the expected behavior when fees are not provided. Consider adding documentation explaining how fees are calculated in such cases.Add documentation above the methods:
+ /// Interacts with the contract by building and broadcasting a transaction. + /// If [feeInfo] is not provided, the fee will be estimated by the network. Future<String> interact({Also applies to: 72-72
lib/src/crypto/evm/repositories/rpc/evm_client.dart (1)
287-296: Add documentation for getPriorityFee return value unitThe implementation looks good, but the method would benefit from documentation clarifying that the returned priority fee is in wei.
Add this documentation:
+ /// Returns the suggested priority fee in wei. + /// Throws an exception if the fee cannot be parsed. Future<BigInt> getPriorityFee() async {lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (1)
10-11: Consider making type2Multiplier configurableThe hardcoded multiplier of 1.5 for type 2 gas price calculations might need adjustment based on network conditions.
Consider making this configurable:
-const type2Multiplier = 1.5; +final class EvmFeeConfig { + final double type2Multiplier; + const EvmFeeConfig({this.type2Multiplier = 1.5}); +}lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_router.dart (1)
1294-1304: LGTM! Consider adding parameter grouping for better readability.The implementation correctly structures the parameters for the Uniswap V2 Router's addLiquidity function. The order matches the contract ABI and all required parameters are present.
Consider grouping related parameters with newlines for better readability:
function: function.addValues( values: [ tokenA, tokenB, + amountADesired, amountBDesired, amountAMin, amountBMin, + to, deadline, ], ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
lib/src/crypto/evm/entities/abi/uniswap_v2/uniswap_v2_router.dart(1 hunks)lib/src/crypto/evm/entities/contract/internal_contract.dart(2 hunks)lib/src/crypto/evm/repositories/rpc/evm_client.dart(1 hunks)lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart(9 hunks)lib/src/crypto/network_type.dart(2 hunks)lib/src/crypto/utxo/utils/send.dart(1 hunks)lib/src/domain/entities/fee.dart(2 hunks)lib/src/domain/entities/transfer_intent.dart(3 hunks)test/no_ci/arb_test.dart(0 hunks)test/no_ci/ava_test.dart(1 hunks)test/no_ci/base_test.dart(0 hunks)test/no_ci/contract_test.dart(0 hunks)test/no_ci/moon_test.dart(0 hunks)test/no_ci/op_test.dart(0 hunks)test/no_ci/sign_evmtx_test.dart(0 hunks)test/no_ci/wallet_test.dart(2 hunks)test/no_ci/zkSync_test.dart(0 hunks)
💤 Files with no reviewable changes (7)
- test/no_ci/op_test.dart
- test/no_ci/moon_test.dart
- test/no_ci/zkSync_test.dart
- test/no_ci/arb_test.dart
- test/no_ci/base_test.dart
- test/no_ci/contract_test.dart
- test/no_ci/sign_evmtx_test.dart
🔇 Additional comments (16)
test/no_ci/ava_test.dart (1)
11-11: Consider adding test cases for both fee scenarios
While setting feeInfo: null is now valid with the new changes, it would be beneficial to have test coverage for both scenarios: with and without fee information. This ensures the system handles both cases correctly.
Consider adding a test case with explicit fee information:
+ test("Avalanche send test with explicit fees", () async {
+ final intent = TransferIntent<EvmFeeInformation>(
+ recipient: arbitrumTestWallet,
+ amount: Amount.convert(value: 0.001, decimals: 18),
+ feeInfo: EvmFeeInformation(...), // Add appropriate fee values
+ token: avalanche,
+ memo: null,
+ );
+ // ... rest of the test
+ });lib/src/domain/entities/transfer_intent.dart (3)
5-5: LGTM: Improved type safety with nullable fee information
The change to make T extends FeeInformation while making feeInfo nullable is a good design choice. It provides better type safety while maintaining flexibility in fee handling.
Also applies to: 11-11
31-31: Verify maxFee usage across the codebase
The change to use maxFee instead of fee for EVM transactions is more accurate, but we should ensure consistent usage across the codebase.
✅ Verification successful
The change to use maxFee is consistent across the codebase
The search results confirm that:
- The
maxFeegetter inEvmFeeInformationis the only fee-related property being used for EVM transactions - There are no remaining usages of direct
.feeaccess onEvmFeeInformation - The implementation properly handles both legacy and Type 2 gas price calculations through the
maxFeegetter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct fee access on EVM transactions
rg "\.fee\b.*EvmFeeInformation" --type dart
Length of output: 45
Script:
#!/bin/bash
# Let's search for all usages of EvmFeeInformation to understand the fee-related patterns
rg "EvmFeeInformation" -B 2 -A 2 --type dart
# Also search for maxFee to see its usage pattern
rg "maxFee" -B 2 -A 2 --type dart
Length of output: 27964
72-74: Verify null safety in fee calculations
The pattern matching with when info.maxFee != null is good for null safety, but we should ensure all code paths are properly handled.
✅ Verification successful
Based on the search results, I can now generate the final response:
Null safety in fee calculations is properly handled
The code has appropriate null safety checks in place:
- The pattern matching with
when info.maxFee != nullis correctly used before accessinginfo.maxFee! - The
maxFeegetter inEvmFeeInformationhas proper null checks forgasLimit - All fee-related calculations in
EvmType2GasPriceuse non-nullableAmounttypes - The pattern matching exhaustively handles all cases with a default
_ => amountcase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential null safety issues in fee calculations
ast-grep --pattern 'maxFee' --lang dart
Length of output: 148
Script:
#!/bin/bash
# Search for fee-related calculations and null checks
rg "maxFee" -A 3 -B 3
Length of output: 21382
lib/src/crypto/evm/entities/contract/internal_contract.dart (1)
42-44: Verify fee estimation behavior
Need to ensure that the RPC interface properly handles null fee information and performs appropriate fee estimation.
Also applies to: 72-74
✅ Verification successful
Let me analyze the fee handling implementation more thoroughly to verify the concern.
Based on the analysis of the code, I can now provide a final response:
Fee handling is properly implemented with null safety
The codebase shows proper handling of null fee information:
- The RPC interface (
evm_rpc_interface.dart) has robust fee estimation logic that handles null fee cases by fetching network fees when needed - When
feeInfois null, the system automatically fetches current network fees viafetchNetworkFees() - The fee estimation includes both legacy gas price and EIP-1559 fee types
- The contract interaction methods properly propagate fee information through the call chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fee estimation logic in RPC interface
rg "estimateGas|gasPrice" --type dart
# Check for proper null handling in transaction building
ast-grep --pattern 'buildUnsignedTransaction.*feeInfo.*null' --lang dart
Length of output: 11731
Script:
#!/bin/bash
# Search for fee handling in internal contract and transaction building
rg "feeInfo" --type dart -A 5
# Search for fee estimation and validation logic
ast-grep --pattern 'Fee|fee' -l ts
Length of output: 21973
lib/src/domain/entities/fee.dart (3)
50-62: LGTM: Improved fee information handling with proper type safety
The changes correctly implement both legacy and type 2 gas price formats using the new EvmGasPrice hierarchy, providing better type safety and clearer separation of concerns.
92-108: LGTM: Robust fee calculation with proper null handling
The maxFee getter implementation:
- Properly handles null cases for both gasLimit and gasPrice
- Correctly calculates fees for both legacy and type 2 transactions using pattern matching
- Maintains type safety throughout the calculation
160-161:
Fix inconsistent serialization in EvmType2GasPrice
The toJson() method uses toString() for serializing maxPriorityFeePerGas and maxFeePerGas, while other implementations use toJson(). This inconsistency could lead to parsing issues.
Apply this fix:
- 'maxPriorityFeePerGas': maxPriorityFeePerGas.toString(),
- 'maxFeePerGas': maxFeePerGas.toString(),
+ 'maxPriorityFeePerGas': maxPriorityFeePerGas.toJson(),
+ 'maxFeePerGas': maxFeePerGas.toJson(),test/no_ci/wallet_test.dart (1)
60-65: LGTM: Test cases properly updated for new fee handling
The test cases have been appropriately updated to:
- Use the new TransferIntent construction pattern
- Test both scenarios with and without explicit feeInfo
- Maintain test coverage for different transaction types
Also applies to: 121-127
lib/src/crypto/network_type.dart (2)
22-22: LGTM: EIP-1559 support flag added to EVMNetworkType
The addition of useEIP1559 with a default value of true is appropriate as most EVM networks support the EIP-1559 fee structure.
Also applies to: 29-29
387-387: LGTM: ZENIQ_SMART_NETWORK correctly opts out of EIP-1559
Explicitly setting useEIP1559: false for ZENIQ network is appropriate as it doesn't support the EIP-1559 fee structure.
lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart (4)
132-137: LGTM: Gas price conversion to Amount
Clean implementation of converting gas price to Amount with appropriate decimals.
222-231: LGTM: Type 2 gas price calculation
The implementation correctly calculates EIP-1559 gas parameters:
- Base fee multiplied by type2Multiplier
- Priority fee added to the adjusted base fee
283-289: LGTM: Network fees fetching
Good implementation of fetching network fees with proper handling of existing fee information.
214-220: Verify priority fee decimals
The priority fee is converted to Amount with 9 decimals. Ensure this is the correct decimal count for all supported networks.
lib/src/crypto/utxo/utils/send.dart (1)
24-24: LGTM: Improved type safety for fee information
The change from TransferIntent to TransferIntent<UtxoFeeInformation> improves type safety by explicitly specifying the fee information type.
| Future<(int gasLimit, EvmGasPrice gasPrice)> fetchNetworkFees({ | ||
| EvmFeeInformation? existing, | ||
| required String recipient, | ||
| required String sender, | ||
| required Uint8List? data, | ||
| required BigInt? value, | ||
| }) async { | ||
| var gasLimit = existing?.gasLimit; | ||
| try { | ||
| gasLimit ??= await estimateGasLimit( | ||
| recipient: recipient, | ||
| sender: sender, | ||
| data: data, | ||
| value: value, | ||
| ); | ||
| } catch (e) { | ||
| Logger.logError(e, hint: "Gas estimation failed"); | ||
|
|
||
| // Only Debug | ||
| assert(true, "Gas estimation failed"); | ||
|
|
||
| gasLimit = 1E6.toInt(); | ||
| } | ||
|
|
||
| final EvmGasPrice gasPrice = switch (existing?.gasPrice) { | ||
| EvmLegacyGasPrice feeInfo => feeInfo, | ||
| EvmType2GasPrice feeInfo => feeInfo, | ||
| null when type.useEIP1559 => await getType2GasPrice(), | ||
| null => EvmLegacyGasPrice( | ||
| gasPrice: await getGasPriceAmount(), | ||
| ), | ||
| }; | ||
|
|
||
| return (gasLimit, gasPrice); | ||
| } |
There was a problem hiding this comment.
Review gas estimation fallback value
The fallback gas limit of 1E6 in case of estimation failure might be too high for some contract interactions.
Consider:
- Using different fallbacks based on transaction type
- Adding a safety margin to the last successful estimation
- Logging more details about the estimation failure
- gasLimit = 1E6.toInt();
+ // Use transaction-type specific fallback
+ gasLimit = switch(data?.isNotEmpty) {
+ true => 150000, // Contract interaction
+ false => 21000, // Simple transfer
+ };
+ Logger.logWarning(
+ 'Gas estimation failed. Using fallback: $gasLimit\n'
+ 'Error: $e\n'
+ 'Transaction: recipient=$recipient, data=${data?.length ?? 0} bytes'
+ );📝 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<(int gasLimit, EvmGasPrice gasPrice)> fetchNetworkFees({ | |
| EvmFeeInformation? existing, | |
| required String recipient, | |
| required String sender, | |
| required Uint8List? data, | |
| required BigInt? value, | |
| }) async { | |
| var gasLimit = existing?.gasLimit; | |
| try { | |
| gasLimit ??= await estimateGasLimit( | |
| recipient: recipient, | |
| sender: sender, | |
| data: data, | |
| value: value, | |
| ); | |
| } catch (e) { | |
| Logger.logError(e, hint: "Gas estimation failed"); | |
| // Only Debug | |
| assert(true, "Gas estimation failed"); | |
| gasLimit = 1E6.toInt(); | |
| } | |
| final EvmGasPrice gasPrice = switch (existing?.gasPrice) { | |
| EvmLegacyGasPrice feeInfo => feeInfo, | |
| EvmType2GasPrice feeInfo => feeInfo, | |
| null when type.useEIP1559 => await getType2GasPrice(), | |
| null => EvmLegacyGasPrice( | |
| gasPrice: await getGasPriceAmount(), | |
| ), | |
| }; | |
| return (gasLimit, gasPrice); | |
| } | |
| Future<(int gasLimit, EvmGasPrice gasPrice)> fetchNetworkFees({ | |
| EvmFeeInformation? existing, | |
| required String recipient, | |
| required String sender, | |
| required Uint8List? data, | |
| required BigInt? value, | |
| }) async { | |
| var gasLimit = existing?.gasLimit; | |
| try { | |
| gasLimit ??= await estimateGasLimit( | |
| recipient: recipient, | |
| sender: sender, | |
| data: data, | |
| value: value, | |
| ); | |
| } catch (e) { | |
| Logger.logError(e, hint: "Gas estimation failed"); | |
| // Only Debug | |
| assert(true, "Gas estimation failed"); | |
| // Use transaction-type specific fallback | |
| gasLimit = switch(data?.isNotEmpty) { | |
| true => 150000, // Contract interaction | |
| false => 21000, // Simple transfer | |
| }; | |
| Logger.logWarning( | |
| 'Gas estimation failed. Using fallback: $gasLimit\n' | |
| 'Error: $e\n' | |
| 'Transaction: recipient=$recipient, data=${data?.length ?? 0} bytes' | |
| ); | |
| } | |
| final EvmGasPrice gasPrice = switch (existing?.gasPrice) { | |
| EvmLegacyGasPrice feeInfo => feeInfo, | |
| EvmType2GasPrice feeInfo => feeInfo, | |
| null when type.useEIP1559 => await getType2GasPrice(), | |
| null => EvmLegacyGasPrice( | |
| gasPrice: await getGasPriceAmount(), | |
| ), | |
| }; | |
| return (gasLimit, gasPrice); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
lib/src/domain/entities/tx_gasFee_entity.dart (2)
40-42: Add documentation for the new priority fee properties.Consider adding documentation comments to explain the purpose and expected values of these priority fee properties.
+ /// The priority fee for safe (low) priority transactions, calculated as (safe - lastBlock) final BigInt safePriority; + /// The priority fee for average (medium) priority transactions, calculated as (average - lastBlock) final BigInt averagePriority; + /// The priority fee for fast (high) priority transactions, calculated as (fast - lastBlock) final BigInt fastPriority;
61-66: Extract the GWEI decimal constant.The GWEI decimal value (18) is duplicated. Consider extracting it into a constant.
+ static const int _GWEI_DECIMALS = 18; + BigInt getPriorityGWEI(FeePriority feePriority) => switch (feePriority) { FeePriority.high => fastPriority, FeePriority.medium => averagePriority, FeePriority.low => safePriority, _ => safePriority, }; Amount getPriorityAmount(FeePriority feePriority) => Amount( value: getPriorityGWEI(feePriority), - decimals: 18, // GWEI + decimals: _GWEI_DECIMALS, );Also applies to: 73-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/src/crypto/network_type.dart(3 hunks)lib/src/domain/entities/tx_gasFee_entity.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/crypto/network_type.dart
🔇 Additional comments (3)
lib/src/domain/entities/tx_gasFee_entity.dart (3)
49-51:
Prevent negative priority fee values.
The priority fees could be negative if calculated values are less than lastBlock.
Consider adding assertions in the constructor:
const EvmNetworkFees({
required this.lastBlock,
required this.safe,
required this.average,
required this.fast,
required this.safePriority,
required this.averagePriority,
required this.fastPriority,
- });
+ }) : assert(safePriority >= BigInt.zero, 'Priority fee cannot be negative'),
+ assert(averagePriority >= BigInt.zero, 'Priority fee cannot be negative'),
+ assert(fastPriority >= BigInt.zero, 'Priority fee cannot be negative');104-106:
Ensure non-negative priority fees in fromJson factory.
The priority fee calculations could result in negative values when the base fee (last_num) is greater than the respective fee levels.
- final safePriority = safe_num - last_num;
- final averagePriority = propose_num - last_num;
- final fastPriority = fast_num - last_num;
+ final safePriority = (safe_num - last_num).isNegative ? BigInt.zero : safe_num - last_num;
+ final averagePriority = (propose_num - last_num).isNegative ? BigInt.zero : propose_num - last_num;
+ final fastPriority = (fast_num - last_num).isNegative ? BigInt.zero : fast_num - last_num;Likely invalid or redundant comment.
79-88: Review the priority fee initialization strategy.
Currently, all priority levels (safe, average, fast) use the same maxPriorityFeePerGas value. This might not provide meaningful differentiation between priority levels.
Consider if this aligns with the intended fee prioritization strategy:
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 (1)
lib/src/domain/entities/fee.dart(2 hunks)
🔇 Additional comments (3)
lib/src/domain/entities/fee.dart (3)
160-161:
Inconsistent serialization in EvmType2GasPrice.toJson()
The serialization of Amount values using toString() is inconsistent with other parts of the codebase where toJson() is used.
Apply this diff to ensure consistent serialization:
- 'maxPriorityFeePerGas': maxPriorityFeePerGas.toString(),
- 'maxFeePerGas': maxFeePerGas.toString(),
+ 'maxPriorityFeePerGas': maxPriorityFeePerGas.toJson(),
+ 'maxFeePerGas': maxFeePerGas.toJson(),50-62: 🛠️ Refactor suggestion
Add input validation and improve error messages
While the pattern matching is robust, the factory method should validate the input values and provide more context in error messages.
Apply this diff to add validation and improve error handling:
{
'gasLimit': int gasLimit,
'gasPrice': Map gasPrice,
} =>
+ gasLimit <= 0
+ ? throw FormatException(
+ 'Invalid gasLimit: must be positive, got $gasLimit')
+ :
EvmFeeInformation(
gasLimit: gasLimit,
gasPrice: EvmLegacyGasPrice(gasPrice: Amount.fromJson(gasPrice)),
),
{
'gasLimit': int gasLimit,
'maxFeePerGas': Map maxFeePerGas,
'maxPriorityFeePerGas': Map maxPriorityFeePerGas,
} =>
+ gasLimit <= 0
+ ? throw FormatException(
+ 'Invalid gasLimit: must be positive, got $gasLimit')
+ : maxFeePerGas.isEmpty || maxPriorityFeePerGas.isEmpty
+ ? throw FormatException(
+ 'Invalid gas price parameters: values cannot be empty')
+ :
EvmFeeInformation(
gasLimit: gasLimit,
gasPrice: EvmType2GasPrice(
maxFeePerGas: Amount.fromJson(maxFeePerGas),
maxPriorityFeePerGas: Amount.fromJson(maxPriorityFeePerGas),
),
),Likely invalid or redundant comment.
121-125: Prevent potential JSON key conflicts
The spread operator usage in toJson could lead to key conflicts if the gas price implementation includes 'gasLimit' as a key.
Let's verify if there are any potential key conflicts in the codebase:
Consider explicitly naming the spread fields to make the structure more obvious:
Json toJson() {
return {
'gasLimit': gasLimit,
- ...?gasPrice?.toJson(),
+ if (gasPrice != null)
+ 'gasPriceInfo': gasPrice.toJson(),
};
}| const EvmType2GasPrice({ | ||
| required this.maxFeePerGas, | ||
| required this.maxPriorityFeePerGas, | ||
| }); |
There was a problem hiding this comment.
Add EIP-1559 fee validation
According to EIP-1559, maxFeePerGas must be greater than or equal to maxPriorityFeePerGas, but this constraint is not enforced.
Apply this diff to add validation:
const EvmType2GasPrice({
required this.maxFeePerGas,
required this.maxPriorityFeePerGas,
- });
+ }) : assert(
+ maxFeePerGas >= maxPriorityFeePerGas,
+ 'maxFeePerGas must be >= maxPriorityFeePerGas according to EIP-1559',
+ );📝 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 EvmType2GasPrice({ | |
| required this.maxFeePerGas, | |
| required this.maxPriorityFeePerGas, | |
| }); | |
| const EvmType2GasPrice({ | |
| required this.maxFeePerGas, | |
| required this.maxPriorityFeePerGas, | |
| }) : assert( | |
| maxFeePerGas >= maxPriorityFeePerGas, | |
| 'maxFeePerGas must be >= maxPriorityFeePerGas according to EIP-1559', | |
| ); |
Summary by CodeRabbit
New Features
EvmLegacyGasPriceandEvmType2GasPrice) with additional properties for better fee calculations.EvmRpcInterfaceandEvmRpcClient.EvmNetworkFeesclass.EVMNetworkType.Bug Fixes
Documentation