-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
feat(ETH): eip1559 gas fee estimator and rpcs #2051
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about how the feature is supposed to work in general: is it really required to run the background loop and keep additional context? What are the benefits of this approach over using "on demand" approach?
PS, by "on demand" I mean e.g., performing external API requests and all calculations in get_eth_gas_price_estimated
.
cc @shamardy
This is essentially how other wallets work. |
I see, thanks for the explanation 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next review iteration.
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
|
||
#[allow(dead_code)] | ||
impl InfuraGasApiCaller { | ||
const INFURA_GAS_API_URL: &'static str = "https://gas.api.infura.io/networks/1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the network ID (1
) should be configurable through coins file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another provider (which in fact we are going to use) supports only mainnet. I think won't support testnets for this feature. I added a check for chain_id (that it can be only mainnet) in this commit ee85d20
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
|
||
fn get_infura_gas_api_url() -> (String, Vec<(&'static str, &'static str)>) { | ||
let url = format!("{}/{}", Self::INFURA_GAS_API_URL, Self::INFURA_GAS_FEES_CALL); | ||
let headers = vec![("Authorization", INFURA_GAS_API_AUTH_TEST.as_str())]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infura API key should be configurable, most likely through MM2.json because the key can be likely shared between multiple coins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a config in MM2.json
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
let resp = slurp_url_with_headers(&url, headers).await.mm_err(|e| e.to_string())?; | ||
if resp.0 != StatusCode::OK { | ||
let error = format!("{} failed with status code {}", Self::INFURA_GAS_FEES_CALL, resp.0); | ||
info!("gas api error: {}", error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be error!
instead of info!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is propagated to the GUI. Here this logging is for debug purposes (so I have just changed it to debug) - I think we do not want to flood logs with this kind of messages
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
Self::BLOCKNATIVE_GAS_PRICES_CALL, | ||
Self::BLOCKNATIVE_GAS_PRICES_PARAMS | ||
); | ||
let headers = vec![("Authorization", BLOCKNATIVE_GAS_API_AUTH_TEST.as_str())]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, block native AUTH token should be configurable via file, not environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a config value for the gas api in MM2.json. However I'd like to leave the api key const like it is for now. This is a temp solution, for testing gas api, until we switch to use komodo proxy instead of direct connection to the api. With the komodo proxy the authentication would be different and this const would be removed
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
fn calculate_with_history(fee_history: &FeeHistoryResult) -> FeePerGasEstimated { | ||
let base_fee = *fee_history.base_fee_per_gas.first().unwrap_or(&U256::from(0)); | ||
let base_fee = u256_to_big_decimal(base_fee, ETH_GWEI_DECIMALS).unwrap_or_else(|_| BigDecimal::from(0)); | ||
let mut priority_fees = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using vec![]
is overhead here, as it is always supposed to have len == 3
. It can be a fixed-size array, pre-initialized with default values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored this code
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
let max_priority_fee_per_gas = u256_to_big_decimal(max_priority_fee_per_gas, ETH_GWEI_DECIMALS) | ||
.unwrap_or_else(|_| BigDecimal::from(0)); | ||
let max_fee_per_gas = base_fee.clone() | ||
* BigDecimal::from_f64(Self::ADJUST_MAX_FEE[i]).unwrap_or_else(|| BigDecimal::from(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All elements of ADJUST_MAX_FEE are 1.0
. Is access by index needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I ve seen in another wallet impl similarly used values are different. So I did this the same way to tune the values after more tests are done
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
let max_fee_per_gas = base_fee.clone() | ||
* BigDecimal::from_f64(Self::ADJUST_MAX_FEE[i]).unwrap_or_else(|| BigDecimal::from(0)) | ||
+ max_priority_fee_per_gas.clone() | ||
* BigDecimal::from_f64(Self::ADJUST_MAX_PRIORITY_FEE[i]).unwrap_or_else(|| BigDecimal::from(0)); // TODO maybe use checked ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All elements of ADJUST_MAX_PRIORITY_FEE are 1.0
. Is access by index needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to be tuned yet. Tests need to be done to adjust those coefficients for better estimations
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
low: priority_fees[0].clone(), | ||
medium: priority_fees[1].clone(), | ||
high: priority_fees[2].clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of vector and array, there should be even 3 separate low
, medium
and high
variables. This will help to avoid unnecessary cloning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored this code
mm2src/coins/eth/eip1559_gas_fee.rs
Outdated
Self::BLOCKNATIVE_GAS_PRICES_CALL, | ||
resp.0 | ||
); | ||
info!("gas api error: {}", error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it should be error!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, I suggest changing it to debug, not to flood log files. The error is sent to the gui nonetheless
mm2src/coins/eth.rs
Outdated
// To call infura use: | ||
// let provider_estimator_fut = InfuraGasApiCaller::fetch_infura_fee_estimation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to other comments, this should be uncommented and made configurable. I find quite confusing to have a working feature that requires code modification to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a config "gas_api" in MM2 json
@dimxy please keep in mind, that PR started to have conflicts in |
|
||
/// Get latest estimated fee per gas | ||
/// | ||
/// Param: coin ticker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting function signature isn't worth, please remove them from all documentations in this PR
ref #2049 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
let coin = match lp_coinfind(&ctx, &req.coin).await { | ||
Ok(Some(coin)) => coin, | ||
Ok(None) | Err(_) => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | ||
}; | ||
let coin = match coin { | ||
MmCoinEnum::EthCoin(eth) => eth, | ||
_ => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single match can cover both of these cases
let coin = match lp_coinfind(&ctx, &req.coin).await { | |
Ok(Some(coin)) => coin, | |
Ok(None) | Err(_) => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | |
}; | |
let coin = match coin { | |
MmCoinEnum::EthCoin(eth) => eth, | |
_ => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | |
}; | |
let coin = match lp_coinfind(&ctx, &req.coin).await { | |
Ok(Some(MmCoinEnum::EthCoin(eth))) => eth, | |
Ok(Some(_)) => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | |
Ok(None) | Err(_) => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thank you for the improvement
let coin = match lp_coinfind(&ctx, &req.coin).await { | ||
Ok(Some(coin)) => coin, | ||
Ok(None) | Err(_) => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | ||
}; | ||
let coin = match coin { | ||
MmCoinEnum::EthCoin(eth) => eth, | ||
_ => return MmError::err(FeeEstimatorError::CoinNotFoundOrSupported), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
|
||
impl FeeEstimatorStartStopResponse { | ||
#[allow(dead_code)] | ||
pub fn get_result(&self) -> String { self.result.clone() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit suggestion: I typically prefer returning the reference of the value and letting the caller decide whether a clone is needed or not. For example, if caller needs only the reference of this value, with this approach you will clone it regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to ref returned
#[display(fmt = "Coin not activated or not a EVM coin")] | ||
CoinNotFoundOrSupported, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should seperate these errors to be more as they are quite different scenarios (Not activated / Not an EVM coin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/// Fee estimation update period in secs, basically equals to eth blocktime | ||
fn get_refresh_interval() -> f64 { FEE_ESTIMATOR_REFRESH_INTERVAL as f64 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not removing FEE_ESTIMATOR_REFRESH_INTERVAL
and using const function?
like:
const fn get_refresh_interval() -> f64 { 15. }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this feature!
I have some notes for the first review iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some notes and suggestion related to module structure
added gas api config in mm2.conf use platform coin for web3 conn check chain id
* dev: feat(zcoin): ARRR WASM implementation (#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (#2046) fix(indexeddb): set stop on success cursor condition (#2067) feat(config): add `max_concurrent_connections` to mm2 config (#2063) feat(stats_swaps): add gui/mm_version in stats db (#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (#2028) security bump for `h2` (#2062) fix(makerbot): allow more than one prices url in makerbot (#2027) fix(wasm worker env): refactor direct usage of `window` (#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (#2039) refactor(utxo): refactor utxo output script creation (#1960) feat(ETH): balance event streaming for ETH (#2041) chore(release): bump mm2 version to 2.1.0-beta (#2044) feat(trezor): add segwit support for withdraw with trezor (#1984)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't do it typically, but this time I thought that it is faster to write code rather than review comments 🙂
I have tried to simplify the estimator_loop
as much as I could, please check the following diff (refactoring proposal): https://github.com/KomodoPlatform/komodo-defi-framework/compare/eip1559-gas-fee-estimate...KomodoPlatform:komodo-defi-framework:eip1559-gas-fee-estimate-refactoring-proposal?expand=1
Thank you for this, studying...
If this is an overkill we can certainly reduce my code and just allow for GUI to handle estimation itself (when it needs it) |
@dimxy
|
I understand now Arc is not good here. I guess I should have used simply Mutex. Thank you for that. |
Yes I think it's better to use your code and let the GUI to control fee per gas estimation running |
If possible, Mutex should be avoided too 🙂 The |
For future reference: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes! I I have one note for now that caught my eye.
@dimxy note that PR started to have conflicts 👀 |
updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing Work 🔥 ! All below comments are non-blockers!
Please add comments documenting new coin configs and new APIs, or API changes for QA team. You can add them in the tracking issue or in this PR. You can follow the way I used here to document changes #1962 (comment), #2014 (comment)
@@ -2477,7 +2479,7 @@ pub async fn taker_swap_trade_preimage( | |||
|
|||
let preimage_value = TradePreimageValue::Exact(my_coin_volume.to_decimal()); | |||
let my_coin_trade_fee = my_coin | |||
.get_sender_trade_fee(preimage_value, stage) | |||
.get_sender_trade_fee(preimage_value, stage, NO_REFUND_FEE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@KomodoPlatform/qa this PR adds support for eth EIP-1559 gas priority fee, for withdrawals and swaps. Allow eip-1559 transactions for coins
Note, that the "chain_id" param must be also set, when the max_eth_tx_type is set to 2. (I guess we have recently set chain_id always mandatory). Withdrawals with new priority fee per gas Setting swap policy to eip-1559 Getting suggested fee per gas To configure the gas fee provider url a new param in MM2.json was added: In coins file there is a new parameter added indicating which gas api provider should be used:
where When gas fee estimator configured, GUI should first start it with a GUI can query the suggested gas fees with New param in sign_raw_transaction rpc to support eip-1559
See sample.
|
@KomodoPlatform/qa this PR changes trade fees as part of initiatives to solve problems with high transaction fees. "trade_preimage" rpc does not include refund fee At the same time To discuss: maybe it is worth to add a param to the "get_trade_fee" rpc unchanged Gas limits adjusted for swap operations This all is experimental at this stage and needs to be tested and discussed. |
done in #2051 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Huge job!
pub const ETH_PAYMENT: u64 = 65_000; | ||
/// Gas limit for swap payment tx with ERC20 tokens | ||
/// real values are 98,9K | ||
pub const ERC20_PAYMENT: u64 = 120_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to set this higher, see #2135
maybe ERC20_RECEIVER_SPEND too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this definitely should be tuned.
Should we maybe add different gas limit sets for different coins?
Or, maybe allow to override gas limit in coins file
Add eth EIP1559 priority fee estimator (WiP):
Add tx priority fee support for withdraw and swaps, improve gas limit for swap txns:
This PR covers requirements in #1848 issue for Eth coin and ERC20 tokens, allowing more predictable priority-based gas fee and lesser gas limit for swap transactions