Conversation
…s fee adjust coin params, rename "set_swap_transaction_fee_policy" rpc to "set_swap_gas_fee_policy", force set_swap_gas_fee_policy to store policy in the platform coin
|
How can i set eip1559 for all enabled tokens without having to call "set_swap_transaction_fee_policy" for each of them in part? I want to avoid this since i have several dozen tokens... can i also use this in the old enable call? i need to use the old i can't use the same endpoints because kdf does lots of requests to the RPC endpoints and if i use the same for both tokens of a pair in orderbook, it will be too slow and public endpoints also tend to block you or slow you down if you send too many requests |
Yes, I forgot to mention this in this PR description, added: there is a |
So, in my example from above, what will happen if i pass |
Didn't look at the code yet, but how it worked before is that tokens inherit from platform in v2 activations, so you need to use |
I can't use v2 activation for the base coin, in this case USDC-PLG20 because i need to use other RPC endpoints for the base coin then for all other PLG20 tokens. If i use the same public server for both, it will not work, i need to spread the load over several distinct servers. |
Yes, in this PR the gas fee policy is stored only in the platform coin (MATIC) so for USDC-PLG20 and any other token one and the same policy will be used. I think, this is consistent. |
|
switched to this branch for testing and the console is flooded with entries like this how can i disable them? |
Will remove this println now, it was done for testing by @dimxy I suppose |
- remove another println
Already removed here ea071a8 :) |
|
Sorry I forgot to remove println's |
|
i used this in RPC call |
I can add this param to the coins file as well. It should not break old kdf's. |
onur-ozkan
left a comment
There was a problem hiding this comment.
Thanks!
Some notes from my side after the initial review:
mm2src/coins/eth/eth_utils.rs
Outdated
| const MAX_ETH_TX_TYPE_SUPPORTED: &str = "max_eth_tx_type"; | ||
| const LEGACY_GAS_PRICE_MULTIPLIER: &str = "gas_price_mult"; | ||
| const GAS_FEE_BASE_ADJUST: &str = "gas_fee_base_adjust"; | ||
| const GAS_FEE_PRIORITY_ADJUST: &str = "gas_fee_priority_adjust"; |
There was a problem hiding this comment.
What are these? I haven't looked at the use cases of them but it shouldn't be necessary to do that just to know what they are. An example, the one that called LEGACY_GAS_PRICE_MULTIPLIER implies it's a multiplier but it's a plain string value. It's worth to document them (or give an explicit/clear name).
mm2src/coins/eth/eth_utils.rs
Outdated
| if let Some(val) = get_conf_param(conf, param)? { | ||
| Ok(Some(val)) | ||
| } else { | ||
| get_conf_param(&coin_conf(ctx, platform), param) | ||
| } |
mm2src/coins/eth/eth_utils.rs
Outdated
| fn get_conf_param(conf: &Json, param: &str) -> Result<Option<Json>, String> { | ||
| if !conf[param].is_null() { | ||
| Ok(Some(conf[param].clone())) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is it returning Result? You could also use get on conf and remove is_null and Some/None dances.
mm2src/coins/eth/eth_utils.rs
Outdated
| match get_conf_param_or_from_plaform(ctx, conf, MAX_ETH_TX_TYPE_SUPPORTED, coin_type) { | ||
| Ok(Some(val)) => { | ||
| let max_eth_tx_type = val | ||
| .as_u64() | ||
| .ok_or_else(|| format!("{MAX_ETH_TX_TYPE_SUPPORTED} in coins is invalid"))?; | ||
| if max_eth_tx_type > ETH_MAX_TX_TYPE { | ||
| return Err(format!("{MAX_ETH_TX_TYPE_SUPPORTED} in coins is too big")); | ||
| } | ||
| Ok(Some(max_eth_tx_type)) | ||
| }, | ||
| Ok(None) => Ok(None), | ||
| Err(err) => Err(err), |
There was a problem hiding this comment.
The Err(err) => Err(err) part isn't necessary. You can do match get_conf_param_or_from_plaform(ctx, conf, MAX_ETH_TX_TYPE_SUPPORTED, coin_type)? and match on Some and None directly.
This applies to all the other functions below.
mm2src/coins/eth/eth_utils.rs
Outdated
| let gas_price_mult = val | ||
| .as_f64() | ||
| .ok_or_else(|| format!("{LEGACY_GAS_PRICE_MULTIPLIER} in coins is invalid"))?; | ||
| if gas_price_mult <= 0.0 { | ||
| return Err(format!("{LEGACY_GAS_PRICE_MULTIPLIER} in coins is negative")); |
There was a problem hiding this comment.
Should we create a good first issue for validating the coins file on the fly using proper types instead of relying on plain Json values and validating fields on demand? Using proper types would allow the coins file to be automatically validated at KDF startup.
We could also reject unknown fields to help keep the file clean and up to date.
cc @shamardy
mm2src/coins/nft.rs
Outdated
| Some(gas_price) => EthTxFeeDetails::new( | ||
| gas_used, | ||
| PayForGasOption::Legacy(LegacyGasPrice { gas_price }), | ||
| PayForGasOption::Legacy { gas_price }, // TODO: is this always legacy? |
There was a problem hiding this comment.
nit:
| PayForGasOption::Legacy { gas_price }, // TODO: is this always legacy? | |
| // TODO: is this always legacy? | |
| PayForGasOption::Legacy { gas_price }, |
mm2src/coins/lp_coins.rs
Outdated
| /// Use legacy gas price | ||
| #[default] | ||
| Unsupported, | ||
| Internal, | ||
| Legacy, |
There was a problem hiding this comment.
What does "legacy gas price" mean? Could you elaborate the comment a bit more?
There was a problem hiding this comment.
Added more doc comments (btw Legacy means pre-EIp1559)
This is due to this https://github.com/KomodoPlatform/komodo-defi-framework/blob/6bd2a172999220bb10e4973e3b860df02a85b5eb/mm2src/coins/eth.rs#L6734-L6745 P.S. it should be fixed in this other PR #2454 if it needs fixing Edit: nevermind as I saw this https://github.com/KomodoPlatform/komodo-defi-framework/blob/6bd2a172999220bb10e4973e3b860df02a85b5eb/mm2src/coins/eth.rs#L6731 Why the increase then as when I tried using withdraw it used exactly 0.1 gwei for low priority |
…ram in activation request overrides it
… const and gas variants,
|
fixed @onur-ozkan notes, tyvm 0788385 |
… mutex (to prevent dup nonce in enable legacy rpc)
* dev: (24 commits) fix(ordermatch): ignore loop-back; clear on null root; reject stale keep-alives (#2580) fix(clippy): fix clippy warnings for #2565 (#2589) fix(Trezor): fix utxo and eth calls due to firmware changes (#2565) fix(utxo): calculate min_trading_vol based on fixed tx fees (#2564) feat(protocol): [0] solana support (#2586) fix(utxo): fix header deserialization; guard AuxPoW (#2583) chore(rust 1.89): make CI clippy/fmt pass (wasm32, all-targets) (#2581) fix(utxo): deserialize sapling root for PIVX block headers (#2572) improvement(dep-stack): security bumps (#2562) fix(utxo): correct block header deserialization for AuxPow and KAWPOW coins (#2563) feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (#2454) ci(pull-requests): review notification bot (#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538) bless clippy (#2560) refactor(toolchain): use latest available stable compiler (#2557) feat(wallet): implement unified offline private key export API (#2542) improve note for docker test start failure (#2550) fix(DOCS): add note for macos to fix docker containers startup failure (#2544) refactor(toolchain): general stabilization for stable rust (#2528) ... # Conflicts: # mm2src/coins/eth.rs # mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs # mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs # mm2src/coins/eth/eth_tests.rs # mm2src/coins/eth/eth_withdraw.rs # mm2src/coins/eth/v2_activation.rs # mm2src/coins/nft.rs # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs # mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs # mm2src/mm2_main/src/rpc/lp_commands/tokens.rs # mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
934f4e1 to
093afd5
Compare
|
Merged latest dev. |
* dev: fix build script failing to find .git/HEAD (#2601) refactor(EVM): rename fn, fix timeouts, add activation req validation (#2543) improvement(`static mut`s): `static mut` removal (#2590) fix(orders): set subscription on kickstart and skip GC of own pubkeys (#2597) # Conflicts: # mm2src/coins/eth.rs
shamardy
left a comment
There was a problem hiding this comment.
Next review iteration, most of these comments are non blockers.
| ))?; | ||
| let gas_price = pay_for_gas_option.get_gas_price(); | ||
| let (max_fee_per_gas, max_priority_fee_per_gas) = pay_for_gas_option.get_fee_per_gas(); | ||
| // TODO: we should get _nonce_lock here (when WalletConnect is supported for swaps) |
There was a problem hiding this comment.
Swaps are already supported for WalletConnect, I suppose it works the same way as metamask if we pass nonce as None. But we use our internal nonce fetching logic here https://github.com/KomodoPlatform/komodo-defi-framework/blob/093afd5280e678562743d22ae37c4e96725fe59e/mm2src/coins/eth/wallet_connect.rs#L286 so I think we should do this for WC asap c.c. @mariocynicys
In the future, we should make nonce usage configurable for metamask and WC, the config option should be about if we should let the connected wallets decide it or not, it can be paired with the broadcast parameter or separate from it.
| .await | ||
| ); | ||
|
|
||
| coin.sign_and_send_transaction(0.into(), Call(token_addr), data, gas_limit) |
There was a problem hiding this comment.
This used to use the estimate without the multiplier but it will now use it even if it was meant for other things, is this acceptable?
There was a problem hiding this comment.
I removed this call as redundant, to always use estimate_gas_for_contract_call_if_conf.
Indeed, estimate_gas_for_contract_call_if_conf will apply the multiplier, if it is set. I thought it's okay to use multiplier for all calls, including 'approve'
| if conf["coin"].as_str() != Some(ticker) { | ||
| return ERR!("Failed to activate '{}': ticker does not match coins config", ticker); | ||
| } |
There was a problem hiding this comment.
Just curious why we needed this
There was a problem hiding this comment.
Actually I added this for sanity check in tests (when I used this fn to enable many coins and messed with configs/tickers).
If fact, we can eliminate the ticker param and get it from the conf param in all coin enable functions.
This commit does the following: - Adds SwapGasFeePolicy (Legacy/EIP-1559) stored on the platform coin; tokens inherit it - Supports GasPriceRpcParam in sign_raw_transaction (uses policy or explicit Legacy/EIP-1559 fees) - Adds gas_price_adjust (legacy/base/priority) and estimate_gas_mult; token→platform fallback; same for max_eth_tx_type - enable and enable_eth_with_tokens accept swap_gas_fee_policy (platform coin only) - Renames wei_from_big_decimal → u256_from_big_decimal; removes 1 gwei minimum bump; validates enable ticker vs conf - Replaces global nonce lock with per-network, per-address locks BREAKING CHANGE: renames RPCs get/set_swap_transaction_fee_policy → get/set_swap_gas_fee_policy
This PR adds a few fixes and improvements in handling gas fee policy:
Those params allow to adjust legacy or priority gas price for specific EVM coins or tokens. First the params are searched in the token config, if none, then in the platform coin config (cc @cipig).
swap_gas_fee_policyto theenable(if EVM) andenable_eth_with_tokensRPCs. The swap_gas_fee_policy param can have the values: Legacy, Low, Medium and High. The param is relevant only for the platform coin.wei_from_big_decimalintou256_from_big_decimalbcs this conversion is used not only for wei but for token smallest units as well.More fixes added:
increase_by_percentfn) to allow BNB low feesestimate_gas_multparam to the coins file. If it is set to some floating point value, theneth_estimateGasRPC will be used to get gas limit for swap transactions. The received gas limit value will be also adjusted by multiplying by the estimate_gas_mult value (therefore recommended to set it to value > 1.0).This PR is also preliminary for the upcoming new liquidity routing feature.