Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add eth_multicall , support array of eth_call for simulation across multiple blocks #383
Add eth_multicall , support array of eth_call for simulation across multiple blocks #383
Changes from 11 commits
5556499
95822af
602d2bf
6ce85e2
2fcbf1d
046502a
e4ec926
0fbdf40
ccfc70b
7590b80
bbf34e8
c147f92
23ef5b8
73897a7
152cf09
c8abc82
14e9b35
94786e2
4b63f49
dc38150
9463ba6
51fad35
1802812
6bd8e50
2cf1b8e
ff39b4a
81e6730
4a42aa2
7d159f8
7787ce8
d8ca0a4
90016c0
d596116
0f8426a
6928d24
9528d89
c6a3397
b587f7f
818ef4a
3c7c14e
1814075
c3999c2
1a92538
41213eb
e785349
024958c
385791c
557038a
e2ae78f
d1cce31
dfdab76
2dde369
dd0fe15
6cde1ab
62ab430
bde1830
8d8725a
dc4017b
a3f89ad
28d995f
50c09b8
be26261
0630df2
e861a4a
cba6d4e
2196a29
40daadb
d831c5f
995acb2
88f1f7b
235c40c
e817b74
78b3a92
cb5d419
8d3b4d4
5f09367
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This does not match
uint
definition above it. either both should be decimal or both hex encoded. I understand that this will decrees readability but it has to be done for the sake of consistency.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.
This is only used for error codes, consistent with how other errors are returned (but in general don't have proper error responses/types defined). As an example, eth_call can return:
but eth_call's definition gives no indication about error response/format and it does return a negative decimal
execution-apis/src/eth/execute.yaml
Lines 1 to 16 in 4da6a02
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 understand, but since it is "base-type", shouldn't it follow the same format as other types around it. What if future API calls needed this type outside of the error codes scope? I think defining this should follow convention, and if you want to define another type for decimal int then you probably should.
Also in this case, type should be
number
and notstring
. because afaikstring
type means there is quotations around it. and in the code example shown, it does not have a quotation around it.maybe it should be something like this:
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.
You are correct about it being incorrectly specified as string, that should be changed (and tested thoroughly, only arrays and and strings have ever been defined before).
We could rename to errorCode, although nothing else in that file is prescriptive about how that data type is being used, it's low-level types and it's up to the actual definition of the api to say:
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.
number have been defined before
you can see similar approach here:
execution-apis/src/eth/fee_market.yaml
Lines 38 to 40 in fa0580e
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.
Keep in mind this is only a spec. What matters is the client implementations to be tested and make sure that they match the spec. the more descriptive the spec the better for all clients. One issue with number is it allows not only integers but also numbers with decimal point (float for example) so the description should make sure to outline this validation well to client implementers.
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.
Number has been used before, as applied to a field, but not as a base-type (and i'm not sure it would be clear it is signed, and should not be float). Ideally, this would be defined as an array of possible, explicit values, as here, and just drop the entire notion of signed int, although this has only been done for newer endpoints, it seems:
execution-apis/src/engine/openrpc/methods/forkchoice.yaml
Line 19 in fa0580e
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 with the above approach. lets drop this int base type then and go with the above ^^^
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.
This is even better because it produces a specific list of numbers and defines what each error code means explicitly. much more prefered
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.
Can we mark this as resolved now? I think we agree that there should not be an
int
type, and instead we should just use literals for specific errors?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 guess
blockOverride
andcalls
should be mandatory variables here andstateOverrides
optional?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.
No I'd leave
blockOverride
also optional. For a multi-call single-block simulation users shouldn't need to override block fields.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 personally prefer empty arrays over missing/null arrays. I find it leads to improved developer ergonomics (no need for conditional branching on presence, you can just map/loop over the array). Functionally, I think both are fine though.
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.
if blockoverride is empty, which block is used? latest I guess?
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.
The third parameter is a "block tag" which serves to determine the fallback block fields (as well as base state). Something we have to consider is when there are multiple batches of calls (without block override fields):
I'm in favor of 1. I think we should keep this method dumb rather than do something unexpected for 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.
If possible I'd like us to remove this field. I feel it'll be a random hash that doesn't have a use-case anyway and to produce it we have to merkleize the state as well as receipts and withdrawals.
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 believe blocks still need a hash, because the
BLOCKHASH(number)
opcode has to return something. I think there is value in letting the caller know what value would have been returned by that opcode is useful.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.
Nitpick: I realize the engine API and CL folks call this
feeRecipient
but on the EL side we haveeth_coinbase
oreth_getBlockBy*
returns aminer
field. Nowminer
is outdated but I have a slight preference towardscoinbase
instead of introducing a 3rd name for it.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 agree with this, it seemed like feeRecipient was going to be the one that everyone ran with right after 1559, but it slowly made its way back to coinbase. Any disagreement @MicahZoltu ?
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 pretty strongly favor accurate names rather than names that match internal implementation details that exist for legacy reasons. In this case,
coinbase
dates back to early Bitcoin days and I think it was when your client held your private keys (not sure on that last part).feeRecipient
on the other hand is a term that most accurately reflects what the field means.That being said, if
eth_getBlockBy*
returns thefeeRecpient
address in a field calledcoinbase
, I can see the value in being consistent across calls. It just strikes me as incredibly unfortunate that we seem to be stuck using terrible names forever, making it harder for every new developer in Ethereum to figure out how things work.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.
Do we have standardized error codes for these? We could try to utilize them as much as possible
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 is no example elsewhere for error code standardizataion. We could add but i think it should be a separate initiative applied to all API's and not added to a single API
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 don't think that just because everyone else pees in the pool it means we should also pee in the pool. I agree that having a standard set of error codes would be valuable to have clearly specified here. Is the issue that it isn't clear how exactly to format such a thing in OpenRPC?
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.
inconsistent with BlockOverrides.number where it is uint256 and here uint64, I would change BlockOverrides.number to be uint64.
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.
Fixed (changed to uint64).
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 not to use #/components/schemas/GenericTransaction - it has same fields...
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.
We could use the same, but its missing default values