Add EXTCODEHASH tests #544
Conversation
| }, | ||
| "transaction": { | ||
| "data": [ | ||
| "0x000000000000000000000000deadbeef00000000000000000000000000000001" |
There was a problem hiding this comment.
this could be extended (just by adding more data elements) to dynamic argument to be
- non existent account
- precompiled account
- empty account / no code
There was a problem hiding this comment.
True. I've added these cases - actually 2 cases for precompiles, one where it exists as an account and one where it doesn't. I believe we have PRs to add tests specifically for these cases as well, do we want both dynamic and static versions or is the dynamic case enough?
There was a problem hiding this comment.
I think dynamic case is enough. also try not to use <=Byzantium expect section as it multiplies the amount of tests. and extcodehash not even supported on <=Byzantium.
| "value": -1 | ||
| }, | ||
| "network": [ | ||
| "<=Byzantium" |
There was a problem hiding this comment.
if this checks that extcodehash is failing it might be just enough to have one test checking that extcodehash failing on all forks <= Byzantium. and in this and other tests it is not required to check its failing on <=Byzantium.
cause otherwise it will produce many tests that all do the same thing - checking that extcodehash is failing on <= Byzantium.
There was a problem hiding this comment.
Agreed, I copied this from another test. I suspect the best approach is to have a single test that is specifically checking EXTCODEHASH is unsupported <= Byzantium. The purpose is fairly ambiguous with it mixed in with other tests.
For now I've removed the <=Byzantium cases from the tests I'm adding and also change >Byzantium to >=Constantinople to be clearer that these are added in Constantinople (my little brain struggles to remember the ordering of hard forks).
| "value": -1 | ||
| }, | ||
| "network": [ | ||
| "<=Byzantium" |
There was a problem hiding this comment.
same tests overload (see above comments)
|
dont forget to run --filltests --fillchain once done. to enable this tests for hive. |
…precompiles (existing and non-existing) and empty accounts.
…ble that this feature is new in Constantinople. Remove <=Byzantium cases that are only checking that EXTCODEHASH is not supported.
|
Added the blockchain variants. |
I figured small batches was a good balance between review size and amount of time spent wrangle git branches but I'm happy to create separate PRs for each of these if that's easier.