feat(ETH): add gas_limit coins param to override default values

[dimxy]

In PR #2051 there were added several gas limit constants to allocate gas price more precisely for different swap operations and for sending coin and tokens, instead of the old single capped value. However there are also erc20 tokens which are called over proxy contracts, which require more gas.
This PR:

• increases the default consts for erc20 ops (to the old value actually) to ensure proxied erc20 would have enough gas
• adds gas_limit param to coins file to allow setting lower (or higher) gas limits for selected tokens

Fixes #2135

Sample of gas_limit (with all available limit values) in the coins file:

{
    "coin": "ETH",
    "gas_limit": {
        "eth_send_coins": 21000,               -- Gas limit for sending coins 
        "eth_send_erc20": 65000,              -- Gas limit for sending ERC20 tokens
        "eth_payment": 75000,                    -- Gas limit for swap payment tx with coins
        "erc20_payment": 120000,              -- Gas limit for swap payment tx with ERC20 tokens
        "eth_receiver_spend": 55000,         -- Gas limit for swap receiver spend tx with coins 
        "erc20_receiver_spend": 130000,    -- Gas limit for swap receiver spend tx with ERC20 tokens
        "eth_sender_refund": 110000,          -- Gas limit for swap refund tx with coins
        "erc20_sender_refund": 110000,      -- Gas limit for swap refund tx with with ERC20 tokens
        "eth_max_trade_gas": 150000.        -- Gas limit for other operations
    }
}

Both gas_limit and all of its params are optional.

[cipig]

do we need both eth_send_coins and eth_send_erc20? we will need to configure the values for each token in part anyway, so i guess we need only one variant.

[laruh]

Thanks for the fix! I have some suggestions and question

[dimxy]

do we need both eth_send_coins and eth_send_erc20? we will need to configure the values for each token in part anyway, so i guess we need only one variant.

I have a set of the same gas limit constants in MM2 and this param allows to override any of them (just for sake of completeness). You may override only one or few or no one at all

[cipig]

first of all, code works fine, i can set the different limits in coins file
i am already using some lower limits: KomodoPlatform/coins#1074

i collected some gas values for couple different tokens:

======== PLG20 ========

USDT transfer       40,182
USDT erc20Payment   78,553
USDT receiverSpend  55,671
USDT senderRefund   53,641

DAI transfer        40,186
DAI erc20Payment    78,689
DAI receiverSpend   55,675
DAI senderRefund    48,821

PAXG transfer       40,208
PAXG erc20Payment   78,577
PAXG receiverSpend  55,673

USDC transfer       45,059 
USDC erc20Payment   98,788 
USDC receiverSpend  55,724 
USDC senderRefund   53,706 

TBTC transfer       41,696
TBTC erc20Payment   91,965
TBTC receiverSpend  69,461

XIDR erc20Payment   85,751
XIDR receiverSpend  64,606
XIDR senderRefund   62,588

RNDR erc20Payment   80,107 
RNDR senderRefund   50,484

GLM erc20Payment    78,565 
GLM senderRefund    53,643 

EURE transfer       77,410
EURE erc20Payment   134,763
EURE receiverSpend  88,099 

GRT transfer        40,232
GRT erc20Payment    78,613 
GRT receiverSpend   55,709

GMT transfer        34,312
GMT erc20Payment    84,693
GMT receiverSpend   62,101

GNS transfer        34,785
GNS erc20Payment    90,174
GNS receiverSpend   62,550

NEXO transfer       40,220
NEXO erc20Payment   95,701
NEXO receiverSpend  67,997

ETH erc20Payment    73,233
ETH receiverSpend   67,435

TRYB erc20Payment   78,553
TRYB receiverSpend  55,649

AGEUR transfer      41,879
AGEUR erc20Payment  79,989
AGEUR receiverSpend 57,332

CRV transfer        40,232
CRV erc20Payment    95,701
CRV receiverSpend   50,909

======== BEP20 ========

BTC transfer        34,491
BTC erc20Payment    72,794
BTC receiverSpend   49,980
BTC senderRefund    36,750

======== main coins ========

BNB ethPayment      49,406
BNB receiverSpend   40,793
BNB senderRefund    42,887

MATIC ethPayment    48,730
MATIC receiverSpend 40,817
MATIC senderRefund  38,787

i know, some values are missing, but i guess that the following things are true:

• receiverSpend is always lower then erc20Payment
• senderRefund is always lower then receiverSpend (or similar, for BNB itself it's actually slightly higher)
• transfer is always lower then senderRefund

so maybe we should change the default limits a bit

• ETH_SENDER_REFUND can be set to same as ETH_RECEIVER_SPEND, since those values are nearly the same in reality (38k-42k)
• lower ETH_SEND_ERC20 from 210k... the real values for transfer are 35k-45k and 77k for EURE proxy token
• if ERC20_PAYMENT of 150k works with the EURE proxy token, then we can lower all other ERC20 limits (ERC20_RECEIVER_SPEND, ERC20_SENDER_REFUND, ETH_SEND_ERC20) since the real values are always lower then ERC20_PAYMENT
• or we set ERC20_PAYMENT higher then 150k ... i also observed that when you set the limits to close to the real values, the tx is reverted too... is there a "rule" about this? to me it looks like you can't go past 90% usage of the gas limit
  this is a erc20payment of EURE: https://polygonscan.com/tx/0xd81e5434ee5c9388c4a26f66004903953af963477e892091567edc02f671115c (uses 89% gas, so could be close to being reverted)
• or maybe we should think about using lower values altogether and define higher ones for the tokens that need it... seems that most tokens are not proxy tokens (and EURE will move to a new contract anyway to save fees, see https://github.com/monerium/smart-contracts/blob/feat/v2/docs/version2.md#1-gas-cost-efficiency)... drawback is that some tokens could stop working if we don't find and set the higher limits

[dimxy]

is there a "rule" about this? to me it looks like you can't go past 90% usage of the gas limit

I found in metamask that they multiply calculated gas limit by 1,5 for contract calls.

[mariocynicys]

Have the same nit about the unwrap_ors, LGTM otherwise :)

[shamardy]

or maybe we should think about using lower values altogether and define higher ones for the tokens that need it... seems that most tokens are not proxy tokens (and EURE will move to a new contract anyway to save fees, see https://github.com/monerium/smart-contracts/blob/feat/v2/docs/version2.md#1-gas-cost-efficiency)... drawback is that some tokens could stop working if we don't find and set the higher limits

I am for this approach as I think it's the best one but it will not work for custom tokens. Maybe for custom tokens I can add higher const values when implementing them. If we follow this approach, for any new token added to config, it should be tested with the default low limits for transactions and swaps before adding the gas_limit configs if transactions or swaps need more gas. What do you think @dimxy @cipig , do you envision any drawbacks if we used lower limits as consts?

P.S. for custom tokens they will not be used for cross-chain swaps most probably but will be used to be traded on same chain using the upcoming liquidity routing feature (1inch or uniswap integration) so these will have different gas limits calculations.

[dimxy]

What do you think @dimxy @cipig , do you envision any drawbacks if we used lower limits as consts?

P.S. for custom tokens they will not be used for cross-chain swaps most probably but will be used to be traded on same chain using the upcoming liquidity routing feature (1inch or uniswap integration) so these will have different gas limits calculations.

Maybe yet we could add some token categories in the coins file and create different gas limit defaults for them (like proxied/non-proxied).

[cipig]

Maybe yet we could add some token categories in the coins file and create different gas limit defaults for them (like proxied/non-proxied).

was thinking about that too, but i guess it only makes sense if proxied tokens are detected automatically by mm2 and then higher limits are applied... like default * 1.5 (this would work for EURE-PLG20, but it's the only such token i found so far, so idk if other proxied tokens need other limits... and if the detection is not done automatically, we can simply define higher limits for each of such tokens manually in the coins file with the existing "gas_limit" structure

some differences in gas consumption are still a bit of a mystery to me, eg USDT-BEP20 receiverSpend 67,104 49,968... same call (receiverSpend) used a different amount of gas, one time 67k, the other time 50k ... first i thought the difference depends on if the call was done by maker or taker, but it's not that, so idk where such differences come from... will continue to collect data

from the tokens i tested so far (30 or so), this values would work (incl. EURE-PLG20, the proxied token)

    pub const ETH_PAYMENT: u64 = 65_000; // same value atm
    pub const ETH_RECEIVER_SPEND: u64 = 65_000; // same value atm
    pub const ETH_SENDER_REFUND: u64 = 65_000; // is 100_000 atm

    pub const ETH_SEND_ERC20: u64 = 90_000; // is 210_000 atm
    pub const ERC20_PAYMENT: u64 = 150_000; // same value atm
    pub const ERC20_RECEIVER_SPEND: u64 = 120_000; // is 150_000 atm
    pub const ERC20_SENDER_REFUND: u64 = 120_000; // is 150_000 atm

and excluding EURE-PLG20 this values are fine

    pub const ETH_SEND_ERC20: u64 = 60_000; // is 210_000 atm
    pub const ERC20_PAYMENT: u64 = 120_000; // same value atm
    pub const ERC20_RECEIVER_SPEND: u64 = 90_000; // is 150_000 atm
    pub const ERC20_SENDER_REFUND: u64 = 90_000; // is 150_000 atm

but i have only tested PLG20, AVX20 and BEP20 tokens... from the main coins i also tested FTM and KCS, but not the tokens on those chains

[onur-ozkan]

This is already handled by serde(default)

[dimxy]

if "gas_limit" is null serde returns invalid type: null error. I wanted to handle this differently (to return the default) than any bad values (to fail)

[shamardy]

🔥

[shamardy]

@KomodoPlatform/qa I guess @cipig already added some gas limits to some coins here KomodoPlatform/coins#1074 , KomodoPlatform/coins#1075 but the new gas_limit parameter described here #2137 (comment) needs to be documented.

[shamardy]

I guess I can merge this since docs PR is already open and under review plus @cipig already uses it and it works.