test: Add some mising State Test export features #807
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
==========================================
+ Coverage 97.93% 97.98% +0.04%
==========================================
Files 116 116
Lines 11309 11370 +61
==========================================
+ Hits 11076 11141 +65
+ Misses 233 229 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chfast
force-pushed
the
test/export
branch
2 times, most recently
from
2e07baf to
2646cc4
Compare
chfast
force-pushed
the
test/export
branch
2 times, most recently
from
2680d79 to
bae296a
Compare
| @@ -79,7 +79,7 @@ class state_transition : public ExportableFixture | |||
| Transaction tx{ | |||
| .gas_limit = block.gas_limit, | |||
| .max_gas_price = block.base_fee + 1, | |||
| .max_priority_gas_price = 1, | |||
| .max_priority_gas_price = block.base_fee + 1, | |||
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Or another idea is that Transaction constructor for legacy should enforce this
There was a problem hiding this comment.
Yes they can, but there is no time to do this change now.
There was a problem hiding this comment.
Then at least add
assert(tx.type != Legacy || tx.max_priority_gas_price == tx.max_gas_price);
to state transition.
There was a problem hiding this comment.
But it also can be just
if (tx.type == Legacy)
tx.max_priority_gas_price = tx.max_gas_price;
There was a problem hiding this comment.
I improved the asserts in TearDown() a bit.
It looks to me that we should have something like TestBlock with every field std::optional and set default values only after test definition. Otherwise, the test definitions must fight with defaults.
| @@ -79,7 +79,7 @@ class state_transition : public ExportableFixture | |||
| Transaction tx{ | |||
| .gas_limit = block.gas_limit, | |||
| .max_gas_price = block.base_fee + 1, | |||
| .max_priority_gas_price = 1, | |||
| .max_priority_gas_price = block.base_fee + 1, | |||
There was a problem hiding this comment.
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.
| @@ -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.
This may not be the best idea because e.g. geth doesn't understand "Tangerine Whistle".
There was a problem hiding this comment.
I changed this to export as EIP150 and EIP158... We still accept full revision names.
| rev = EVMC_SPURIOUS_DRAGON; | ||
|
|
||
| block_reward = 0; | ||
| block.base_fee = 0; |
test/unittests/state_transition.cpp
Outdated
| auto& jstorage_keys = je["storageKeys"] = json::json::array(); | ||
| for (const auto& k : storage_keys) | ||
| jstorage_keys.push_back(hex0x(k)); | ||
| ja.push_back(je); |
There was a problem hiding this comment.
Maybe makes sense to std::move(je)
| expect.tx_error = INSUFFICIENT_FUNDS; | ||
| } | ||
|
|
||
| TEST_F(state_transition, blob_tx_insuficient_funds) | ||
| { | ||
| rev = EVMC_CANCUN; | ||
| block.base_fee = 1; |
There was a problem hiding this comment.
@winsvega usually says it should be at least 7 in tests, but I'm not sure exactly why
There was a problem hiding this comment.
It is the minimal value by the spec.
Add support for exporting state transition tests: