Skip to content
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

enhancement: factor out a type from access lists #960

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

richardgreg
Copy link
Contributor

What was wrong?

Because it's repeated in several different transaction types, the Tuple[Address, Tuple[Bytes32, ...]] portion should be promoted to its own type.

Related to Issue #
Fixes #947

How was it fixed?

This PR refactors the access list type used in various transaction classes to reduce redundancy and improve maintainability. Specifically, it introduces a new Access class to represent the access_list structure, which is used in multiple transaction types.

  1. Created a new Access class to encapsulate the account and slots structure.
@slotted_freezable
@dataclass
class Access:
    account: Address
    slots: Tuple[Bytes32, ...]
  1. Updated Transaction Classes

  2. RLP Encoding and Decoding Adjustments:

  • Introduced helper functions encode_access_list and decode_access_list to handle the conversion between the new Access type and the previous tuple structure for RLP encoding and decoding.
  • Updated encode_transaction and decode_transaction functions to use these helpers.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@SamWilsn
Copy link
Collaborator

This looks like pretty much exactly what I had in mind!

@richardgreg richardgreg marked this pull request as ready for review June 27, 2024 14:49
@richardgreg
Copy link
Contributor Author

If this is good, I can do it for Prague if need be

Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been playing around with this PR a bit, and it's looking great!

I think it should be rebased on top of #968. That branch can handle nested dataclasses better than master, and you shouldn't need the encode_access_list and decode_access_list helpers there.

That said, there's going to be a bit more work involved in this refactor. You'll need to use the Access type in tests near, for example, here:

((address1, (hash1, hash2)), (address2, tuple())),

There are a few other required changes, like dealing with unpacking here:

if isinstance(tx, (AccessListTransaction, FeeMarketTransaction)):
for address, keys in tx.access_list:
preaccessed_addresses.add(address)
for key in keys:
preaccessed_storage_keys.add((address, key))

mypy is your best friend when doing refactoring. The easiest way to run it in this project is:

$ tox -e static

y_parity: U256
r: U256
s: U256


# Helper function to handle the RLP encoding of the Access class instances.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Helper function to handle the RLP encoding of the Access class instances.

return tuple((access.account, access.slots) for access in access_list)


# Helper function to handle the RLP decoding of the Access class instances.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Helper function to handle the RLP decoding of the Access class instances.

@richardgreg
Copy link
Contributor Author

I'm back to continue working on this

@richardgreg richardgreg force-pushed the access-list-refactor branch from 8d08d53 to a91b66b Compare August 7, 2024 12:52
@SamWilsn SamWilsn force-pushed the access-list-refactor branch from 6702712 to c2a769b Compare August 13, 2024 16:41
@SamWilsn
Copy link
Collaborator

Took the liberty of rebasing and running isort/black. You'll need to force pull/reset your local branch.

Comment on lines 22 to 27
TX_BASE_COST = 21000
TX_DATA_COST_PER_NON_ZERO = 16
TX_DATA_COST_PER_ZERO = 4
TX_CREATE_COST = 32000
TX_ACCESS_LIST_ADDRESS_COST = 2400
TX_ACCESS_LIST_STORAGE_KEY_COST = 1900
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was removing these intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Fixed it 🙂

@richardgreg
Copy link
Contributor Author

Took the liberty of rebasing and running isort/black. You'll need to force pull/reset your local branch.

Thanks. Running tox -e static breaks due to some issue with rust on my system that I'm still trying to fix, but running the individual linting/formatting commands works. I will be doing that from now on. I also noticed running mypy src tests setup.py --exclude "tests/fixtures/*" --namespace-packages returns many errors. Should that be an issue on its own?

@SamWilsn
Copy link
Collaborator

Running tox -e static breaks due to some issue with rust on my system that I'm still trying to fix

What seems to be the problem?

I also noticed running mypy src tests setup.py --exclude "tests/fixtures/*" --namespace-packages returns many errors. Should that be an issue on its own?

Does it return errors on an unmodified master checkout, or just on your branch? master passes mypy for me without any errors. If both branches are erroring, it's likely that you're missing some packages in your install. Inside your virtual environment, try pip install -e .[lint,test].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Factor out a type from access lists
2 participants