Skip to content
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

Jump to bottom

enhancement(utxo): improve trade and withdraw fee calculations #2083

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from
Open

enhancement(utxo): improve trade and withdraw fee calculations #2083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2cd22fd
save dev state
borngraced Mar 8, 2024
f45b5dc
fix get_sender_trade_fee and qrc20 unit tests
borngraced Mar 8, 2024
944fd57
minro changes
borngraced Mar 8, 2024
0f8426e
impl calculate_fee helper macro and other minor changes
borngraced Mar 8, 2024
cd019f6
save dev state
borngraced Mar 12, 2024
8e14509
fix WASM build
borngraced Mar 12, 2024
0a8f110
implement utxo coin withdrawal fee by priority
borngraced Mar 18, 2024
f3f56cc
Merge remote-tracking branch 'origin/dev' into utxo-tx-fee-calc
borngraced Mar 18, 2024
d445554
fix unnecessary clone in test_sender_trade_preimage_with_allowance
borngraced Mar 18, 2024
2081c25
remove debug println
borngraced Apr 4, 2024
817a479
merge with dev and fix conflicts
borngraced Apr 8, 2024
d26490f
rename total_fee -> qrc20_payment_fee and minor changes in z_coin
borngraced May 12, 2024
9ec826a
merge and fix conflicts
borngraced May 12, 2024
15f6f0f
fix test_generate_tx_doge_fee
borngraced May 12, 2024
da566ca
use estimate_fee_sat for calculating fee based on priority
borngraced May 13, 2024
f3a10a8
merge with dev and fix conflicts
borngraced May 27, 2024
e4fdfb2
fix qrc20 unit tests and minor changes
borngraced May 27, 2024
ac97404
fix test_withdraw_p2pk_balance unit test
borngraced May 28, 2024
0fc8f98
add setting n_block from coin conf for withdrawal
borngraced May 28, 2024
026c5cf
implement BTC mempool fee comp for low priority
borngraced May 31, 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
20 changes: 10 additions & 10 deletions mm2src/coins/eth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5506,25 +5506,22 @@ impl MmCoin for EthCoin {
fn history_sync_status(&self) -> HistorySyncState { self.history_sync_state.lock().unwrap().clone() }

fn get_trade_fee(&self) -> Box<dyn Future<Item = TradeFee, Error = String> + Send> {
let coin = self.clone();
let selfi = self.clone();
Box::new(
async move {
let pay_for_gas_option = coin
.get_swap_pay_for_gas_option(coin.get_swap_transaction_fee_policy())
.await
.map_err(|e| e.to_string())?;

let fee = calc_total_fee(U256::from(gas_limit::ETH_MAX_TRADE_GAS), &pay_for_gas_option)
.map_err(|e| e.to_string())?;
let fee_coin = match &coin.coin_type {
EthCoinType::Eth => &coin.ticker,
let gas_price = try_s!(selfi.get_gas_price().await);
let fee = gas_price * U256::from(gas_limit::ETH_MAX_TRADE_GAS);
let fee_coin = match &selfi.coin_type {
EthCoinType::Eth => &selfi.ticker,
EthCoinType::Erc20 { platform, .. } => platform,
EthCoinType::Nft { .. } => return ERR!("Nft Protocol is not supported yet!"),
};

Ok(TradeFee {
coin: fee_coin.into(),
amount: try_s!(u256_to_big_decimal(fee, ETH_DECIMALS)).into(),
paid_from_trading_vol: false,
tx_size: 0,
})
}
.boxed()
Expand Down Expand Up @@ -5592,6 +5589,7 @@ impl MmCoin for EthCoin {
coin: fee_coin.into(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
})
}

Expand All @@ -5618,6 +5616,7 @@ impl MmCoin for EthCoin {
coin: fee_coin.into(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
})
};
Box::new(fut.boxed().compat())
Expand Down Expand Up @@ -5667,6 +5666,7 @@ impl MmCoin for EthCoin {
coin: fee_coin.into(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
})
}

Expand Down
4 changes: 4 additions & 0 deletions mm2src/coins/eth/eth_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ fn get_sender_trade_preimage() {
coin: "ETH".to_owned(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
}
}

Expand Down Expand Up @@ -382,6 +383,7 @@ fn get_erc20_sender_trade_preimage() {
coin: "ETH".to_owned(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
}
}

Expand Down Expand Up @@ -474,6 +476,7 @@ fn get_receiver_trade_preimage() {
coin: "ETH".to_owned(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
};

let actual = coin
Expand All @@ -498,6 +501,7 @@ fn test_get_fee_to_send_taker_fee() {
coin: "ETH".to_owned(),
amount: amount.into(),
paid_from_trading_vol: false,
tx_size: 0,
};

let dex_fee_amount = u256_to_big_decimal(DEX_FEE_AMOUNT.into(), 18).expect("!u256_to_big_decimal");
Expand Down
3 changes: 3 additions & 0 deletions mm2src/coins/lightning.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,6 +1342,7 @@ impl MmCoin for LightningCoin {
coin: self.ticker().to_owned(),
amount: Default::default(),
paid_from_trading_vol: false,
tx_size: 0,
})
}

Expand All @@ -1351,6 +1352,7 @@ impl MmCoin for LightningCoin {
coin: self.ticker().to_owned(),
amount: Default::default(),
paid_from_trading_vol: false,
tx_size: 0,
}))
}

Expand All @@ -1364,6 +1366,7 @@ impl MmCoin for LightningCoin {
coin: self.ticker().to_owned(),
amount: Default::default(),
paid_from_trading_vol: false,
tx_size: 0,
})
}

Expand Down
21 changes: 20 additions & 1 deletion mm2src/coins/lp_coins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1936,6 +1936,17 @@ pub trait MarketCoinOps {
fn is_trezor(&self) -> bool;
}

/// Priority levels for UTXO fee estimation for withdrawal.
#[derive(Clone, Debug, Deserialize, PartialEq)]
pub enum UtxoFeePriority {
/// Low priority.
Low,
/// Normal priority.
Normal,
/// High priority.
High,
}

#[derive(Clone, Debug, Deserialize, PartialEq)]
#[serde(tag = "type")]
pub enum EthGasLimitOption {
Expand All @@ -1948,12 +1959,19 @@ pub enum EthGasLimitOption {
#[derive(Clone, Debug, Deserialize, PartialEq)]
#[serde(tag = "type")]
pub enum WithdrawFee {
/// encapsulates the fixed fee amount for a withdrawal transaction, regardless of transaction size.
UtxoFixed {
amount: BigDecimal,
},
/// encapsulates the fee amount for a withdrawal transaction calculated based on the transaction size in kilobytes.
UtxoPerKbyte {
amount: BigDecimal,
},
/// encapsulates the priority of a withdrawal transaction, indicating the desired fee
/// level for transaction processing.
UtxoPriority {
priority: UtxoFeePriority,
},
Comment on lines +1970 to +1974
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is how I envision the GUI implementation of this part.

We present the user with the three levels of fees and let them make a choice. The GUI would potentially make three separate withdrawal requests, each using a different priority level. This would generate three different raw transaction hexes, each associated with a different fee and expected confirmation time.

Once the user selects the fee they prefer, the corresponding transaction hex would be used.

What are your thoughts on this approach? Would it be feasible to make some changes that simplifies the process for the GUI to handle this with a single request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, here's mine view.

The api receives a priority withdrawal request from the gui, the api generates fee for the available priorities(currently 3) and send the response (each fee with it's tx hex) and finally, gui can render this three options for user to pick a choice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The api receives a priority withdrawal request from the gui, the api generates fee for the available priorities(currently 3) and send the response (each fee with it's tx hex) and finally, gui can render this three options for user to pick a choice.

This is how I see it as well, but it might be too complicated as we would probably need to return 3 TransactionDetails objects in the response which is different from the current withdraw response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI for eth I also added priority levels for tx fees:
For withdrawals user can add a new WithdrawFee option and set explicit fee per gas.
For swaps there is an rpc to set the desired level of low/medium/priority for the whole swap (so the API will by itself query the current fee value before each swap step).
There is also an rpc for GUI to query fees for all supported 3 levels (to show fe values to the user so he could select desired level). Initially user should start a loop in the API which periodically calculates priority fees, so the query rpc just immediately returns the recent estimated fee values.

Maybe we could do this similar both for utxo and eth.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Some explanation why a there is loop in API to obtain fees. This is how other eth wallet apps do it: API does fee estimations getting data from the eth provider. The GUI subscribes to the fee updates from API so it can in realtime notify the user about changes)

EthGas {
/// in gwei
gas_price: BigDecimal,
Expand Down Expand Up @@ -2320,6 +2338,7 @@ pub struct TradeFee {
pub coin: String,
pub amount: MmNumber,
pub paid_from_trading_vol: bool,
pub tx_size: u64,
}

/// A type alias for a HashMap where the key is a String representing the coin/token ticker,
Expand Down Expand Up @@ -4843,7 +4862,7 @@ pub async fn my_tx_history(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>, S
}

/// `get_trade_fee` rpc implementation.
/// There is some consideration about this rpc:
/// There is some consideration about this rpc:
/// for eth coin this rpc returns max possible trade fee (estimated for maximum possible gas limit for any kind of swap).
/// However for eth coin, as part of fixing this issue https://github.com/KomodoPlatform/komodo-defi-framework/issues/1848,
/// `max_taker_vol' and `trade_preimage` rpc now return more accurate required gas calculations.
Expand Down
50 changes: 30 additions & 20 deletions mm2src/coins/qrc20.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ use crate::utxo::tx_cache::{UtxoVerboseCacheOps, UtxoVerboseCacheShared};
use crate::utxo::utxo_builder::{UtxoCoinBuildError, UtxoCoinBuildResult, UtxoCoinBuilder, UtxoCoinBuilderCommonOps,
UtxoFieldsWithGlobalHDBuilder, UtxoFieldsWithHardwareWalletBuilder,
UtxoFieldsWithIguanaSecretBuilder};
use crate::utxo::utxo_common::{self, big_decimal_from_sat, check_all_utxo_inputs_signed_by_pub, UtxoTxBuilder};
use crate::utxo::{qtum, ActualTxFee, AdditionalTxData, AddrFromStrError, BroadcastTxErr, FeePolicy, GenerateTxError,
GetUtxoListOps, HistoryUtxoTx, HistoryUtxoTxMap, MatureUnspentList, RecentlySpentOutPointsGuard,
UnsupportedAddr, UtxoActivationParams, UtxoAddressFormat, UtxoCoinFields, UtxoCommonOps,
UtxoFromLegacyReqErr, UtxoTx, UtxoTxBroadcastOps, UtxoTxGenerationOps, VerboseTransactionFrom,
UTXO_LOCK};
use crate::utxo::utxo_common::{self, big_decimal_from_sat, check_all_utxo_inputs_signed_by_pub,
PreImageTradeFeeResult, UtxoTxBuilder};
use crate::utxo::{qtum, AdditionalTxData, AddrFromStrError, BroadcastTxErr, FeePolicy, GenerateTxError,
GetUtxoListOps, HistoryUtxoTx, HistoryUtxoTxMap, HtlcSpendFeeResult, MatureUnspentList,
RecentlySpentOutPointsGuard, UnsupportedAddr, UtxoActivationParams, UtxoAddressFormat,
UtxoCoinFields, UtxoCommonOps, UtxoFromLegacyReqErr, UtxoTx, UtxoTxBroadcastOps,
UtxoTxGenerationOps, VerboseTransactionFrom, UTXO_LOCK};
use crate::{BalanceError, BalanceFut, CheckIfMyPaymentSentArgs, CoinBalance, CoinFutSpawner, ConfirmPaymentInput,
DexFee, Eip1559Ops, FeeApproxStage, FoundSwapTxSpend, HistorySyncState, IguanaPrivKey, MakerSwapTakerCoin,
MarketCoinOps, MmCoin, MmCoinEnum, NegotiateSwapContractAddrErr, PaymentInstructionArgs,
Expand Down Expand Up @@ -492,9 +493,7 @@ impl Qrc20Coin {
/// `gas_fee` should be calculated by: gas_limit * gas_price * (count of contract calls),
/// or should be sum of gas fee of all contract calls.
pub async fn get_qrc20_tx_fee(&self, gas_fee: u64) -> Result<u64, String> {
match try_s!(self.get_tx_fee().await) {
ActualTxFee::Dynamic(amount) | ActualTxFee::FixedPerKb(amount) => Ok(amount + gas_fee),
}
Ok(try_s!(self.get_tx_fee_per_kb().await) + gas_fee)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to add a fee rate (rate per kb) to the gas fee?
Is the gas fee here a rate as well (per kb)?

Copy link
Member Author

Choose a reason for hiding this comment

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

assuming that gas_fee represents the gas costs and self.get_tx_fee_per_kb() represents the per-kb costs associated with the tx size

}

/// Generate and send a transaction with the specified UTXO outputs.
Expand Down Expand Up @@ -582,7 +581,7 @@ impl Qrc20Coin {
&self,
contract_outputs: Vec<ContractCallOutput>,
stage: &FeeApproxStage,
) -> TradePreimageResult<BigDecimal> {
) -> TradePreimageResult<PreImageTradeFeeResult> {
let decimals = self.as_ref().decimals;
let mut gas_fee = 0;
let mut outputs = Vec::with_capacity(contract_outputs.len());
Expand All @@ -595,7 +594,10 @@ impl Qrc20Coin {
UtxoCommonOps::preimage_trade_fee_required_to_send_outputs(self, outputs, fee_policy, Some(gas_fee), stage)
.await?;
let gas_fee = big_decimal_from_sat(gas_fee as i64, decimals);
Ok(miner_fee + gas_fee)
Ok(PreImageTradeFeeResult {
tx_size: miner_fee.tx_size,
fee: miner_fee.fee + gas_fee,
})
}
}

Expand All @@ -612,7 +614,7 @@ impl UtxoTxBroadcastOps for Qrc20Coin {
#[cfg_attr(test, mockable)]
impl UtxoTxGenerationOps for Qrc20Coin {
/// Get only QTUM transaction fee.
async fn get_tx_fee(&self) -> UtxoRpcResult<ActualTxFee> { utxo_common::get_tx_fee(&self.utxo).await }
async fn get_tx_fee_per_kb(&self) -> UtxoRpcResult<u64> { utxo_common::get_tx_fee_per_kb(&self.utxo).await }

async fn calc_interest_if_required(
&self,
Expand Down Expand Up @@ -653,7 +655,7 @@ impl GetUtxoListOps for Qrc20Coin {
#[async_trait]
#[cfg_attr(test, mockable)]
impl UtxoCommonOps for Qrc20Coin {
async fn get_htlc_spend_fee(&self, tx_size: u64, stage: &FeeApproxStage) -> UtxoRpcResult<u64> {
async fn get_htlc_spend_fee(&self, tx_size: u64, stage: &FeeApproxStage) -> UtxoRpcResult<HtlcSpendFeeResult> {
utxo_common::get_htlc_spend_fee(self, tx_size, stage).await
}

Expand Down Expand Up @@ -718,7 +720,7 @@ impl UtxoCommonOps for Qrc20Coin {
fee_policy: FeePolicy,
gas_fee: Option<u64>,
stage: &FeeApproxStage,
) -> TradePreimageResult<BigDecimal> {
) -> TradePreimageResult<PreImageTradeFeeResult> {
utxo_common::preimage_trade_fee_required_to_send_outputs(
self,
self.platform_ticker(),
Expand Down Expand Up @@ -1352,6 +1354,7 @@ impl MmCoin for Qrc20Coin {
coin: selfi.platform.clone(),
amount: big_decimal_from_sat(fee as i64, selfi.utxo.decimals).into(),
paid_from_trading_vol: false,
tx_size: 0,
})
};
Box::new(fut.boxed().compat())
Expand Down Expand Up @@ -1400,15 +1403,19 @@ impl MmCoin for Qrc20Coin {
self.preimage_trade_fee_required_to_send_outputs(vec![sender_refund_output], &stage)
.await?
} else {
BigDecimal::from(0) // No refund fee if not included.
// No refund fee if not included.
PreImageTradeFeeResult {
fee: 0.into(),
tx_size: 0,
}
};

let total_fee = erc20_payment_fee + sender_refund_fee;
borngraced marked this conversation as resolved.
Show resolved Hide resolved

let qrc20_payment_fee = erc20_payment_fee.fee + sender_refund_fee.fee;
Ok(TradeFee {
coin: self.platform.clone(),
amount: total_fee.into(),
amount: qrc20_payment_fee.into(),
paid_from_trading_vol: false,
tx_size: sender_refund_fee.tx_size,
})
}

Expand All @@ -1429,10 +1436,12 @@ impl MmCoin for Qrc20Coin {
let total_fee = selfi
.preimage_trade_fee_required_to_send_outputs(vec![output], &stage)
.await?;

Ok(TradeFee {
coin: selfi.platform.clone(),
amount: total_fee.into(),
amount: total_fee.fee.into(),
paid_from_trading_vol: false,
tx_size: total_fee.tx_size,
})
};
Box::new(fut.boxed().compat())
Expand All @@ -1456,8 +1465,9 @@ impl MmCoin for Qrc20Coin {

Ok(TradeFee {
coin: self.platform.clone(),
amount: total_fee.into(),
amount: total_fee.fee.into(),
paid_from_trading_vol: false,
tx_size: total_fee.tx_size,
})
}

Expand Down
Loading
Loading