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

cast: s/build_tx/TxBuilder/ #1082

Merged
merged 3 commits into from
Apr 2, 2022
Merged

cast: s/build_tx/TxBuilder/ #1082

merged 3 commits into from
Apr 2, 2022

Conversation

sblOWPCKCR
Copy link
Contributor

@sblOWPCKCR sblOWPCKCR commented Mar 28, 2022

Refactoring to move away from function-with-hundred-arguments to a Builder pattern

Fixes #937

TxBuilder constructs the tx on the fly, that's why the interface is not super consistent (args() returns a future result, while other setters return &mut self), and provider needs to be passed to some of the setters.

Alternatively, I can change TxBuilder to be just a glorified bag-of-arguments, and leave the actual transaction building in lib.tx. The downsides will be: 1) we'll still have 1 gigantic function that need to handle every field in TxBuilder; 2) it'll use a bit more memory

I'm not 100% confident in my Rust, so plz check for dumb things

Fixes foundry-rs#937

Refactoring to move away from function-with-hundred-arguments to a Builder pattern
cast/src/tx.rs Outdated Show resolved Hide resolved
@onbjerg onbjerg added the L-ignore Log: ignore PR in changelog label Mar 28, 2022
@sblOWPCKCR sblOWPCKCR marked this pull request as draft March 31, 2022 05:36
@sblOWPCKCR sblOWPCKCR force-pushed the tx_build branch 3 times, most recently from 868a9d3 to 6c76711 Compare March 31, 2022 06:08
@sblOWPCKCR sblOWPCKCR marked this pull request as ready for review March 31, 2022 06:20
@sblOWPCKCR
Copy link
Contributor Author

ready for review

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

I just have some nits, mostly that you start docblocks with an empty /// line, otherwise LGTM

cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
utils/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

really nice work - agree with Bjerg's nits, plus some more changes requested

cast/src/lib.rs Outdated Show resolved Hide resolved
cast/src/tx.rs Outdated Show resolved Hide resolved
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Nice work - thanks!

@gakonst gakonst merged commit a0db055 into foundry-rs:master Apr 2, 2022
@sblOWPCKCR sblOWPCKCR deleted the tx_build branch April 3, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L-ignore Log: ignore PR in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Cast::build_tx with Builder pattern
3 participants