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

feat: add account balance overrides #137

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

louise-poole
Copy link
Contributor

@louise-poole louise-poole commented Feb 6, 2025

For some protocols the balances used by a component is stored on a separate contract. To best cater for such components we now support using account_balances where the balance for relevant contracts is monitored and overwritten during simulation.

Decisions:

  • it was decided to work towards deprecating the use 'balance_owners' and instead encourage using account balances. For backwards compatibility, support for protocols using balance_owner has been maintained.

@louise-poole louise-poole force-pushed the lp/account-balance-overrides branch from fbe8aa6 to 8017af2 Compare February 6, 2025 13:07
Copy link
Contributor

@zizou0x zizou0x left a comment

Choose a reason for hiding this comment

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

Looks neat! Just one question about a TODO

@louise-poole louise-poole force-pushed the lp/account-balance-overrides branch from 8017af2 to de04d29 Compare February 11, 2025 15:11
@louise-poole louise-poole changed the base branch from main to lp/update-on-account-changes February 11, 2025 15:12
@zizou0x
Copy link
Contributor

zizou0x commented Feb 13, 2025

I noticed a lot of missing/not updated docs, would be nice to double check that we keep everything up to date before merging

@@ -48,23 +48,42 @@ impl TryFromWithBlock<ComponentWithState> for EVMPoolState<PreCachedDB> {
async fn try_from_with_block(
snapshot: ComponentWithState,
block: Header,
account_balances: &HashMap<Bytes, HashMap<Bytes, Bytes>>,
Copy link
Contributor Author

@louise-poole louise-poole Feb 13, 2025

Choose a reason for hiding this comment

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

Idea: We could refactor our try_from_with_block to decode whole Snapshots instead of ComponentWithState (like the python version). Then we'd have the account balances inside the snapshot and don't need to collect and pass it extra like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that'd be the cleaner way. Can we do this in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it means we pass a lot more data in the interface: The full snapshot has the full vm_storage with it which include storage slots, contract code etc.

@louise-poole louise-poole force-pushed the lp/account-balance-overrides branch from 23d5fc9 to 5f28bff Compare February 13, 2025 11:34
@tvinagre tvinagre self-requested a review February 14, 2025 14:32
@louise-poole louise-poole force-pushed the lp/update-on-account-changes branch from a551ec5 to 33284ad Compare February 17, 2025 08:05
Base automatically changed from lp/update-on-account-changes to main February 17, 2025 08:50
@louise-poole louise-poole force-pushed the lp/account-balance-overrides branch from 289fd31 to 6eb24d5 Compare February 17, 2025 09:02
regardless of if there is an associated contract/attribute change.
Copy link
Contributor

@tvinagre tvinagre left a comment

Choose a reason for hiding this comment

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

🔥

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

Successfully merging this pull request may close these issues.

4 participants