-
Notifications
You must be signed in to change notification settings - Fork 146
proxy: fee-based methods #3613
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
proxy: fee-based methods #3613
Conversation
616c3d1
to
2178422
Compare
getBlobBaseFee(header.excessBlobGas.get, com, com.toEVMFork(header)) * | ||
header.blobGasUsed.get.u256 | ||
if blobBaseFee > high(uint64).u256: | ||
raise newException(ValueError, "blobBaseFee is bigger than uint64.max") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Ethereum terms, it doesn't fit in a Quantity
, which is an official Ethereum spec type. uint64
is, while it's how Quantity
is defined, incidental here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it not? the base fee can take value upto 1ETH per gas as 64bit unsigned integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it to .UInt256
Well nim-web3
defines it as a Quantity
looks like the changes has to be done in nim-web3
https://github.com/status-im/nim-web3/blob/master/web3/eth_api.nim#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely the web3 API defines it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it not? the base fee can take value upto 1ETH per gas as 64bit unsigned integer
My concern is the error message, not the logic, i.e. the user-facing verbiage. uint64
is a programming artifact specific to Nimbus. Here, the idea of a 64-bit Quantity
is something Ethereum defines and is something which is explainable to an Ethereum user.
If, it turns out, that the web3 API seems not to define the actual return value on this, that's fine. But it's also worth checking, yeah, what other ELs do in their actual eth_blobBaseFee
implementations. The Geth link is to an internal function, not what they're willing to return to web3 API users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is the error message, not the logic, i.e. the user-facing verbiage. uint64 is a programming artifact specific to Nimbus. Here, the idea of a 64-bit Quantity is something Ethereum defines and is something which is explainable to an Ethereum user
Ah ok gotcha.
If, it turns out, that the web3 API seems not to define the actual return value on this, that's fine.
the official spec defines the return value as only a hex string
I'll have a look at other ELs and change the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both geth and reth return UInt256
serialized into a hex string. We oughta do the same thing. Opened an issue status-im/nim-web3#222. I think it would be better if we merge this PR first and then I move on to that issue. Resolving that issue might entail more work outside the scope of verified proxy and would block this PR. @tersec what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, yeah.
eth_gasPrice
,eth_maxPriorityFeePerGas
andeth_blobBaseFee
meta: #3101