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.

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

Fix the calculation for estimated gas price #20

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

piersy
Copy link

@piersy piersy commented Oct 14, 2024

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.

Copy link

github-actions bot commented Oct 14, 2024

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

@piersy piersy force-pushed the piersy/fix_celo_gas_price_estimation branch from bab6984 to f4b6e8d Compare October 14, 2024 17:41
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.
@piersy piersy force-pushed the piersy/fix_celo_gas_price_estimation branch from 28006e8 to 3ed6e3d Compare October 14, 2024 17:42
@piersy piersy requested a review from shazarre October 14, 2024 17:43
@piersy piersy requested a review from aaronmgdr October 15, 2024 16:05
@aaronmgdr
Copy link
Member

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
Copy link
Author

piersy commented Oct 15, 2024

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 😃

  • Celo L1 - eth_gasPrice returns baseFee * celoMultiplier where the default celoMultiplier is 2! And it looks like we use the default in forno.
  • Op L2 stack eth_gasPrice returns baseFee + suggestedTip

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:

  • Celo L1 - ((baseFee * celoMultiplier) * viemMultiplier) + suggestedTip
  • Op L2 stack - ((baseFee+suggestedTip) * viemMultiplier) + suggestedTip

With this PR we get:

  • Celo L1 - (((baseFee * celoMultiplier)-suggestedTip) * viemMultiplier) + suggestedTip
  • Op L2 stack - (baseFee * viemMultiplier) + suggestedTip

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.

src/celo/fees.ts Outdated Show resolved Hide resolved
src/celo/fees.ts Outdated Show resolved Hide resolved
@piersy
Copy link
Author

piersy commented Oct 16, 2024

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?

@piersy piersy requested a review from shazarre October 16, 2024 21:42
@aaronmgdr
Copy link
Member

@piersy thanks so much for your help on this

aaronmgdr added a commit to celo-org/developer-tooling that referenced this pull request Oct 17, 2024
@piersy
Copy link
Author

piersy commented Oct 22, 2024

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

@piersy piersy closed this Oct 22, 2024
@piersy piersy reopened this Oct 22, 2024
@aaronmgdr
Copy link
Member

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

open against the wevm/viem repo

@aaronmgdr aaronmgdr self-assigned this Nov 21, 2024
@aaronmgdr aaronmgdr added this to the 5 - Celo MVP Mainnet milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants