-
Notifications
You must be signed in to change notification settings - Fork 285
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
test: Add some mising State Test export features #807
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,9 +13,9 @@ evmc_revision to_rev(std::string_view s) | |
return EVMC_FRONTIER; | ||
if (s == "Homestead") | ||
return EVMC_HOMESTEAD; | ||
if (s == "EIP150") | ||
if (s == "Tangerine Whistle" || s == "EIP150") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may not be the best idea because e.g. geth doesn't understand "Tangerine Whistle". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this to export as |
||
return EVMC_TANGERINE_WHISTLE; | ||
if (s == "EIP158") | ||
if (s == "Spurious Dragon" || s == "EIP158") | ||
return EVMC_SPURIOUS_DRAGON; | ||
if (s == "Byzantium") | ||
return EVMC_BYZANTIUM; | ||
|
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 this change? This transaction field in the spec is called
max_priority_fee_per_gas
and is what is added on top of block base fee.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.
Ah I guess because default is legacy transaction? It's confusing that
Transaction
has this field for legacy.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.
Legacy transaction requires these values to be the same. So I made the default agree as well.
Maybe this is not great for clarity of tests, but EIP-1559 calculations work for legacy transactions if these two fields have the same value of legacy gas price.
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't these calculations check transaction type and ignore this field (i.e. consider it equal max_gas_price) for legacy?
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.
Or another idea is that
Transaction
constructor for legacy should enforce thisThere 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.
Yes they can, but there is no time to do this change now.
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.
Then at least add
to state transition.
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.
But it also can be just
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 improved the asserts in
TearDown()
a bit.It looks to me that we should have something like
TestBlock
with every fieldstd::optional
and set default values only after test definition. Otherwise, the test definitions must fight with defaults.