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

refactor: use BundleBuilder as a state builder #8861

Merged

Conversation

tcoratger
Copy link
Contributor

Closes #4614

Acts as a follow up of #8264 after the most recent changes.

cc @mattsse

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!
a few cosmetic nits.

this is a bit delicate, so I'd appreciate another review by @rkrasiuk and @rakita

Comment on lines +637 to +638
.state_original_account_info(address, present_info.into())
.state_present_account_info(address, present_info.into());
Copy link
Collaborator

@mattsse mattsse Jun 16, 2024

Choose a reason for hiding this comment

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

looks like setter functions that take &mut self would be useful on bundlebuilder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crates/storage/provider/src/providers/database/provider.rs Outdated Show resolved Hide resolved
crates/storage/provider/src/providers/database/provider.rs Outdated Show resolved Hide resolved
crates/storage/provider/src/providers/database/provider.rs Outdated Show resolved Hide resolved
@mattsse mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Jun 16, 2024
@quentinv72
Copy link
Contributor

@tcoratger Just curious why this PR was created? #8264 is still up-to-date.

@mattsse
Copy link
Collaborator

mattsse commented Jun 16, 2024

@quentinv72 I think this was my bad...

thanks for updating, I'll ensure it is properly reviewed this time...

@mattsse
Copy link
Collaborator

mattsse commented Jun 25, 2024

@rkrasiuk idk what to do here

should we proceed with this or #8264

@tcoratger
Copy link
Contributor Author

@rkrasiuk idk what to do here

should we proceed with this or #8264

I think you can even simplify some part with the recent updates of revm, including bluealloy/revm#1527

Let me know if we can merge this or not and I have ideas of a follow up so as not to disorganize things here.

@emhane emhane added the S-blocked This cannot more forward until something else changes label Jun 28, 2024
@emhane
Copy link
Member

emhane commented Jun 28, 2024

blocked by #8264

@tcoratger
Copy link
Contributor Author

tcoratger commented Jun 28, 2024

blocked by #8264

This is an equivalent one no @mattsse? following #8264 (comment) it seems that #8264 will be staled no?

@emhane
Copy link
Member

emhane commented Jun 29, 2024

I'd like to merge the other branch first, since you used it as base, so that the commits go to the author. good open source practice is to use git remote add <repo> to pull the other persons fork, then use git cherry-pick to bring in their changes.

@emhane emhane changed the base branch from main to emhane/integrate-bundle-builder June 29, 2024 09:18
@emhane emhane merged commit 5b7add1 into paradigmxyz:emhane/integrate-bundle-builder Jun 29, 2024
7 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain S-blocked This cannot more forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate BundleBuilder
4 participants