search

celo-org / viem

TypeScript Interface for Ethereum
https://viem.sh
Other
0 stars 0 forks source link

Fix the calculation for estimated gas price #20

Open piersy opened 1 month ago

piersy commented 1 month ago

The gas price returned by the rpc is base_fee + max_priority_fee. So we shouldn't add the max_priority_fee onto that value since it's already included. Also the logic used by viem for applying the multiplier is to only apply the multiplier to the base fee and not the priority fee, so we match that approach here.

github-actions[bot] commented 1 month ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
import * from 'viem' (esm) 60.99 KB (0%) 1.3 s (0%) 21.3 s (+47.16% 🔺) 22.5 s
const viem = require('viem') (cjs) 71.26 KB (0%) 1.5 s (0%) 33.3 s (+27.64% 🔺) 34.7 s
import { createClient, http } from 'viem' 6.26 KB (0%) 126 ms (0%) 1.9 s (+56.03% 🔺) 2 s
import * from 'viem/account-abstraction' 43.89 KB (0%) 878 ms (0%) 8 s (-31.94% 🔽) 8.9 s
import { toCoinbaseSmartAccount } from 'viem/account-abstraction' 34.08 KB (0%) 682 ms (0%) 8.1 s (+41.93% 🔺) 8.8 s
import * from 'viem/accounts' 80.77 KB (0%) 1.7 s (0%) 9.4 s (+37.89% 🔺) 11 s
import { privateKeyToAccount } from 'viem/accounts' 19.43 KB (0%) 389 ms (0%) 1.6 s (-41.69% 🔽) 2 s
import { mnemonicToAccount } from 'viem/accounts' 25.69 KB (0%) 514 ms (0%) 9.4 s (+19.26% 🔺) 9.9 s
import * from 'viem/actions' 46.05 KB (0%) 921 ms (0%) 13.3 s (+56.67% 🔺) 14.3 s
import { getBlockNumber } from 'viem/actions' 318 B (0%) 10 ms (0%) 55 ms (+8.83% 🔺) 65 ms
import * from 'viem/chains' 38.75 KB (+0.16% 🔺) 775 ms (+0.16% 🔺) 8.1 s (-16.55% 🔽) 8.9 s
import { mainnet } from 'viem/chains' 324 B (0%) 10 ms (0%) 99 ms (+92.76% 🔺) 109 ms
import * from 'viem/chains/utils' 1.08 KB (0%) 22 ms (0%) 64 ms (-57.2% 🔽) 86 ms
import * from 'viem/ens' 45.48 KB (0%) 910 ms (0%) 8.3 s (+45.71% 🔺) 9.2 s
import { getEnsAvatar } from 'viem/ens' 22.2 KB (0%) 445 ms (0%) 3.1 s (-61.58% 🔽) 3.6 s
import * from 'viem/siwe' 30.23 KB (0%) 605 ms (0%) 7.4 s (-41.09% 🔽) 8 s
import { verifySiweMessage } from 'viem/siwe' 29.2 KB (0%) 585 ms (0%) 8.6 s (-11.15% 🔽) 9.2 s
aaronmgdr commented 1 month ago

ok. learning more.

so eth_gasPrice is it always returned as vaue of base_fee + max_priority_fee? is this true for when we are on op l2 stack too? also true for when the fee token is passed as a param vs not?

thanks for the find @piersy

piersy commented 1 month ago

ok. learning more.

so eth_gasPrice is it always returned as vaue of base_fee + max_priority_fee? is this true for when we are on op l2 stack too? also true for when the fee token is passed as a param vs not?

thanks for the find @piersy

Hey @aaronmgdr me too 😃

Note the default viemMultiplier is 1.2 and the default celoMultiplier is 2.

So the current gas maxFeePerGas suggested by viem (before this PR) is this:

With this PR we get:

So this change is fixing the gas price suggestion for the Op L2 stack but not for the Celo L1. Although it is arguably improving the situation since the estimate will be lower and given that the baseFee would already be doubled by the server we probably want to minimize any further increases, I suspect that could be why our viem modifications were not using the viemMultiplier originally.

So I'm not sure what to do here, do we want to support different logic for the different networks or do we want just a single approach? If we did want to support different logic for mainnet then we might also want to have that logic switch base on when the cel2 fork occurs, however I don't currently have a clear idea about how best achieve that.

piersy commented 1 month ago

Hi @shazarre, I've made the changes we discussed to switch based on whether the chain is L1 or L2.

BTW when I try to commit I get a bunch of errors from the linter for code that I didn't touch, and I'm wondering how I can configure the project to avoid that?

aaronmgdr commented 1 month ago

@piersy thanks so much for your help on this

piersy commented 1 month ago

Hi @aaronmgdr I've fixed the linter errors here, what's the next stage for this PR?

aaronmgdr commented 1 month ago

Hi @aaronmgdr I've fixed the linter errors here, what's the next stage for this PR?

open against the wevm/viem repo