Skip to content

Draft: adjust EVM feeInformation #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 12, 2024
Merged

Draft: adjust EVM feeInformation #123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b46300b
Initial
dev2-nomo Dec 5, 2024
05f7ed4
refactor: rename fee to maxFee
dev2-nomo Dec 9, 2024
6b1ab42
Refactor EvmFee
ThomasFercher Dec 10, 2024
d508556
Include Priority in EvmNetworkFees
ThomasFercher Dec 10, 2024
cbaa9a5
refactor: make EvmFeeInformation generic for gasPrice type
dev2-nomo Dec 12, 2024
630904c
Merge
dev2-nomo Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions lib/src/crypto/evm/repositories/rpc/evm_rpc_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,7 @@ final class EvmRpcInterface {
List<AccessListItem>? accessList,
}) async {
final (gasPrice, gasLimit) = switch ((feeInfo.gasPrice, feeInfo.gasLimit)) {
(null, int gasLimit) => (
Amount(value: await getGasPrice(), decimals: 18),
gasLimit
),
(Amount gasPrice, int gasLimit) => (gasPrice, gasLimit),
(Amount gasPrice, null) => (
gasPrice,
await estimateGasLimit(
Expand All @@ -229,7 +226,11 @@ final class EvmRpcInterface {
value: value,
)
),
_ => await estimateNetworkFees(
(null, int gasLimit) => (
Amount(value: await getGasPrice(), decimals: 18),
gasLimit
),
(null, null) => await estimateNetworkFees(
recipient: recipient, sender: sender, data: data, value: value),
};

Expand Down
74 changes: 50 additions & 24 deletions lib/src/domain/entities/fee.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,20 @@ sealed class FeeInformation {
'gasLimit': int gasLimit,
'gasPrice': Map gasPrice,
} =>
EvmFeeInformation(
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),
),
Copy link

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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

{
'feePerByte': Map feePerByte,
'fee': Map? fee,
Expand All @@ -68,18 +78,28 @@ sealed class FeeInformation {
}
}

final class EvmFeeInformation extends FeeInformation {
sealed class EvmFeeInformation extends FeeInformation {
final int? gasLimit;
final Amount? gasPrice;

Amount? get maxFee;

const EvmFeeInformation({
required this.gasLimit,
required this.gasPrice,
});
}

final class EvmLegacyFeeInformation extends EvmFeeInformation {
final Amount? gasPrice;

Amount get fee {
return gasPrice ??
Amount.zero * Amount.convert(value: gasLimit ?? 0, decimals: 0);
Amount? get maxFee {
if (gasPrice == null) {
return null;
}
if (gasLimit == null) {
return null;
}

return gasPrice! * Amount.convert(value: gasLimit!, decimals: 0);
}

Json toJson() {
Expand All @@ -89,54 +109,60 @@ final class EvmFeeInformation extends FeeInformation {
};
}

EvmFeeInformation copyWith({
EvmLegacyFeeInformation copyWith({
int? gasLimit,
Amount? gasPrice,
}) {
return EvmFeeInformation(
return EvmLegacyFeeInformation(
gasLimit: gasLimit ?? this.gasLimit,
gasPrice: gasPrice ?? this.gasPrice,
);
}

static EvmFeeInformation get zero {
return EvmFeeInformation(gasLimit: null, gasPrice: null);
}
const EvmLegacyFeeInformation({
required super.gasLimit,
required this.gasPrice,
});
}

final class EvmType2FeeInformation extends EvmFeeInformation {
final Amount? maxFeePerGas;
final Amount? maxPriorityFeePerGas;

Amount? get maxFee {
if (gasLimit == null) {
return null;
}

if (maxFeePerGas == null) {
return null;
}

return maxFeePerGas! * Amount.convert(value: gasLimit!, decimals: 0);
}

const EvmType2FeeInformation({
required super.gasLimit,
required super.gasPrice,
required this.maxFeePerGas,
required this.maxPriorityFeePerGas,
});

static EvmType2FeeInformation get zero {
return EvmType2FeeInformation(
gasLimit: null,
gasPrice: null,
maxPriorityFeePerGas: null,
);
}

Json toJson() {
return {
'gasLimit': gasLimit,
'maxPriorityFeePerGas': maxPriorityFeePerGas.toString(),
'gasPrice': gasPrice?.toJson() ?? Amount.zero.toJson(),
'maxFeePerGas': maxFeePerGas.toString(),
Comment on lines 160 to +161
Copy link

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

};
}

EvmType2FeeInformation copyWith({
int? gasLimit,
Amount? gasPrice,
Amount? maxFeePerGas,
Amount? maxPriorityFeePerGas,
}) {
return EvmType2FeeInformation(
gasLimit: gasLimit ?? this.gasLimit,
gasPrice: gasPrice ?? this.gasPrice,
maxFeePerGas: maxFeePerGas ?? this.maxFeePerGas,
maxPriorityFeePerGas: maxPriorityFeePerGas ?? this.maxPriorityFeePerGas,
);
}
Expand Down
4 changes: 2 additions & 2 deletions lib/src/domain/entities/transfer_intent.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class TransferIntent<T extends FeeInformation?> {

Amount? get fee {
if (feeInfo is EvmFeeInformation) {
return (feeInfo as EvmFeeInformation).fee;
return (feeInfo as EvmFeeInformation).maxFee;
}
if (feeInfo is TronFeeInformation) {
return (feeInfo as TronFeeInformation).feeLimit;
Expand Down Expand Up @@ -69,7 +69,7 @@ class TransferIntent<T extends FeeInformation?> {
}) {
final newTargetValue = switch ((balance, feeInfo)) {
(Amount balance, EvmFeeInformation info) when token.isERC20 == false =>
_calcTargetAmount(balance, info.fee),
_calcTargetAmount(balance, info.maxFee),
(Amount balance, TronFeeInformation info) when token.isERC20 == false =>
_calcTargetAmount(balance, info.feeLimit),
_ => amount,
Expand Down
21 changes: 19 additions & 2 deletions lib/src/domain/entities/tx_gasFee_entity.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ final class EvmNetworkFees extends NetworkFees {
final BigInt average;
final BigInt fast;

BigInt get safePriority => safe - lastBlock;
BigInt get averagePriority => average - lastBlock;
BigInt get fastPriority => fast - lastBlock;

Copy link

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;

const EvmNetworkFees({
required this.lastBlock,
required this.safe,
Expand All @@ -51,11 +55,23 @@ final class EvmNetworkFees extends NetworkFees {
_ => safe,
};

BigInt getPriorityGWEI(FeePriority feePriority) => switch (feePriority) {
FeePriority.high => fastPriority,
FeePriority.medium => averagePriority,
FeePriority.low => safePriority,
_ => safePriority,
};

Amount getFeeAmount(FeePriority feePriority) => Amount(
value: getFeeGWEI(feePriority),
decimals: 18, // GWEI
);

Amount getPriorityAmount(FeePriority feePriority) => Amount(
value: getPriorityGWEI(feePriority),
decimals: 18, // GWEI
);

const EvmNetworkFees.fromBigInt({
required BigInt lastBlock,
}) : this(
Expand All @@ -68,17 +84,18 @@ final class EvmNetworkFees extends NetworkFees {
factory EvmNetworkFees.fromJson(Map<String, dynamic> json) {
if (json
case {
// 'suggestBaseFee': String last,
'suggestBaseFee': String last,
'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);

return EvmNetworkFees(
lastBlock: safe_num,
lastBlock: last_num,
safe: safe_num,
average: propose_num,
fast: fast_num,
Expand Down