zksync-era icon indicating copy to clipboard operation
zksync-era copied to clipboard

feat(Gas Oracle): Move gas prices to be `U256` to avoid overflows

Open juan518munoz opened this issue 1 year ago • 4 comments

What ❔

Replace variables types from u64 to U256.

Why ❔

Prevent possible overflows.

Checklist

  • [x] PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • [x] Code has been formatted via zk fmt and zk lint.

juan518munoz avatar Mar 01 '24 20:03 juan518munoz

Not sure if this PR is ready to be reviewed, but I think that if we're going to switch to U256 because u64 can now overflow with custom base token, it is certainly in scope to address all the places where an overflow can occur (otherwise there isn't much value in the change).

That makes sense, do you want to add someone else to review this PR or are you checking it internally?

jrchatruc avatar Mar 04 '24 18:03 jrchatruc

@jrchatruc I've asked @perekopskiy to review and assist you with this PR.

popzxc avatar Mar 05 '24 07:03 popzxc

Some general questions/comments

  • Do I get it right that base_fee_per_gas/l2_fair_gas_price may overflow u64 if base token price is low?
  • Do I get it right that l1_gas_price may overflow u64 if L1 is not Ethereum but some other chain with cheap base token?
  • u64 is sufficient for gas_per_pubdata, because all VM gas fits into u32, so even u32 is enough. So please revert all changes you've done to gas_per_pubdata
  • Indeed both base_fee_per_gas and l2_fair_gas_price may overflow 64 bits if the base token price is low.
  • Yes, I believe gas prices in Ethereum are also technically U256, so I think it's a good idea to change it so it matches.
  • Makes sense, we'll revert them

jrchatruc avatar Mar 05 '24 20:03 jrchatruc

The change required for this ticket (related to this discussion) was merged into this PR (here a11e12c) since it requires its changes to do it.

jrchatruc avatar Mar 15 '24 19:03 jrchatruc