-
Notifications
You must be signed in to change notification settings - Fork 171
feat(tests): Add flexibility to expected absent scenarios for BALs #2124
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
base: main
Are you sure you want to change the base?
Conversation
36f6287
to
7ab6d35
Compare
src/ethereum_test_types/block_access_list/absence_validators.py
Outdated
Show resolved
Hide resolved
src/ethereum_test_types/block_access_list/absence_validators.py
Outdated
Show resolved
Hide resolved
Some ideas for the exclusion APIThoughts on the current designThe separate absence validators introduce some complexity and come with "import" burden in tests. It also loses some expressiveness of expectation API, such as narrowing down a change to a specific transaction index. I think we could leverage the existing API instead. Extending the existing APIThe dictionary based ( BlockAccessListExpectation(
account_expectations={
sender: BalAccountExpectation(
nonce_changes=[BalNonceChange(tx_index=1, post_nonce=1)],
),
contract: BalAccountExpectation(
storage_changes=[
BalStorageSlot(slot=0x01, slot_changes=[BalStorageChange(tx_index=1, post_value=0x01)]),
],
),
}
) Approach 1: Separate exclusion functionsA corresponding set of "exclude" function ( BlockAccessListExpectation(
account_expectations={
sender: BalAccountExpectation(
nonce_changes=[BalNonceChange(tx_index=1, post_nonce=1)], # Inclusion
exclude_nonce_changes=[BalNonceChange(tx_index=2, post_nonce=None)], # Exclusion
),
contract: BalAccountExpectation(
storage_changes=[BalStorageSlot(slot=0x01, slot_changes=[BalStorageChange(tx_index=1, post_value=0x01)])], # Inclusion
exclude_storage_changes=[BalStorageSlot(slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=0x03)])], # Exclusion
),
}
) Not a fan of this approach;
Approach 2: Add a "condition" to expectation [Recommended]We could merge the function pair and introduce a "condition" operator for a much cleaner approach: BlockAccessListExpectation(
account_expectations={
sender: BalAccountExpectation(
nonce_changes=[
BalNonceChange(tx_index=1, post_nonce=1, condition="include"),
BalNonceChange(tx_index=2, post_nonce=5, condition="exclude"),
],
),
contract: BalAccountExpectation(
storage_changes=[
BalStorageSlot(
slot=0x01, slot_changes=[BalStorageChange(tx_index=1, post_value=0x01, condition="include")]
),
BalStorageSlot(
slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=0x03, condition="exclude")]
),
],
),
}
)
Some alternatives for the keyword "condition":
slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=None, criteria="exclude")]
slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=None, intent="exclude")]
slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=None, action="exclude")] |
I appreciate you taking a look and writing this up. I actually wasn't tied to my approach but I'm leaning more and more towards it because I don't think we will have to define very many of these absence validators. I also do like the separation of these checks from the list due to issues with list indexes I'll outline below.
My two cents here is that I've only defined some examples thus far. I think we can have a very minimal set of absence validators that are indeed expressive down to the transaction index (and I think we should). The flexibility here is really limitless but I don't want to have to define too many of these, nor do I think it would be necessary. I think we can come up with a very small set of expressive absence validators to achieve 99% of test case checks and test writers could write their own with this approach if they want to check a very specific thing.
I tried exploring this but if we have complex test cases, one of the "burdens" on the test writer is you can't just define the item you don't want to see. It has to exist inside of the list in the right place. So in order for you to only check that a particular item is not in the list, with a complex testing scenario, you would also have to define all of the "positive" checks that come before it in the list until you get to the list index that points to where the negative check would be, then you explicitly mark that it should be excluded. Does this make sense? I think the above part is fine because we will likely want to define complex cases explicitly... but if you want to check that you have the correct BAL while also checking that some items should be excluded, you end up in a situation where you're defining where an exclusion is within a list, but the next index is then off by one, visually at least if not effectively, because this exclusion takes up room in the list. e.g. BlockAccessListExpectation(
account_expectations={
contract: BalAccountExpectation(
storage_changes=[
BalStorageSlot( # 0th item in the list
slot=0x01, slot_changes=[BalStorageChange(tx_index=1, post_value=0x01)]
),
BalStorageSlot( # we are saying this shouldn't exist as the 1st index in the list
slot=0x02, slot_changes=[BalStorageChange(tx_index=2, post_value=0x02, condition="exclude")]
),
BalStorageSlot( # positive check after slot "0x01" (should now be a positive check on the 1st index in the list but is 2nd)
slot=0x03, slot_changes=[BalStorageChange(tx_index=2, post_value=0x03)]
),
BalStorageSlot( # positive check after slot "0x03" (should now be a positive check on the 2nd index in the list but is 3rd... etc...)
slot=0x04, slot_changes=[BalStorageChange(tx_index=2, post_value=0x04)]
),
],
),
}
) I hope this makes sense ^. Basically, the items you define in the list are not actually where they would exist if you mix positive checks with absence checks. I think this gets quite messy to read and to write, tbh. |
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.
A couple of small suggestions, but it looks mostly good to me :D
from . import AbsenceValidator, BalAccountChange | ||
|
||
|
||
@validate_call |
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.
Add this to the list of things I love about pydantic I didn't even know existed :D
I agree that we'll loose parity w/ the underlying BAL if we include exclusion in the same list, which can be confusing to read. Please go ahead @fselmo ! |
The specs are currently correctly written, but we were not validating all of the ordering according to EIP-7928: - Addresses: lexicographic (bytewise). - Storage keys: lexicographic within each account. - Block access indices: ascending within each change list. This change validates the order of the BAL before we even begin to compare against our expectation. We also now validate that the expectations we define are subsequences within the BAL (expected order). - refactor: Explicit check for the fields we care about up front for `model_fields_set` - refactor: awkward comparison method should just be a validation method (_validate_change_lists)
- This becomes an issue when JSON-serializing the BAL object and then re-filling from the fixture. We should use `HexNumber` for any Number fields as this correctly serializes to JSON as hex representation.
7ab6d35
to
ffb60a3
Compare
- We should have flexibility in defining the absence cases we expect for block-level access lists. This allows us to define absence validators for any case we might want to validate against. I don't expect these to grow very much but this does provide some flexibility.
2bf0435
to
58e92ae
Compare
Rebased on top of #2138, which is more important to get in first and touches the same files. |
🗒️ Description
We should have flexibility in defining the absence cases we expect for block-level access lists. This allows us to define absence validators for any case we might want to validate against. I don't expect these to grow very much but this does provide some flexibility.
I would very much welcome something simpler, without having to define validators, but with all the possible conditions for BALs, it feels that defining quick validators would give us the most flexibility for
should_not_exist
checks. This was also similar to the approach for the modifiers so the devex feels fairly similar.Note: This PR is rebased on top of #2138. Once that is merged, I will do a final cleanup.
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.