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.

Sign up for GitHub

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

Jump to bottom

feat(forge build): add initcode size check #9116

Merged
merged 24 commits into from
Oct 19, 2024
Merged

feat(forge build): add initcode size check #9116

merged 24 commits into from
Oct 19, 2024

Conversation

mgiagante
Copy link
Contributor

@mgiagante mgiagante commented Oct 15, 2024 •
edited by grandizzy
Loading

Closes #4783
described in the Additional Context of this comment

Motivation

Prior to EIP-3860 there was no size limit on creation code (initcode), and just a 24576 byte size limit on runtime code.
EIP-3860 went live in Shangai so now there is both:

  • a 24576 byte size limit on runtime code
  • a new 24576 * 2 = 49152 byte size limit on initcode.

Currently, forge build --sizes does not account for this new size limit.

Solution

  1. This PR adds a new column pair to the table displayed by forge build --sizes for the contract init code size (bytes) and the margin left counting towards the limit (bytes).
  2. Also, the existing check for the deployed contract size is complemented with a new check for the init code size, making the process exist with status code 1 if the check fails.
  3. Finally, a new option --ignore-eip-3860 allows to ignore the check mentioned in the previous point. This is aimed to users deploying to chains that do not enforce this limit.

Testing

  • Two tests were adjusted to account for the new structure of the table.
  • Two new tests were introduced to verify both failing and ignoring the new init code size limit.

@mgiagante mgiagante mentioned this pull request Oct 15, 2024
Closed
@NelsonGaldeman
Copy link

Finally! Thank you ser 🫡

Tested and worked to me!

grandizzy
grandizzy requested changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

Thank you! looks good, have added minor nits

crates/common/src/compile.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/build.rs Show resolved Hide resolved
@grandizzy grandizzy changed the title Feature/add initcode size check feat(forge build): add initcode size check Oct 15, 2024
@grandizzy grandizzy changed the title feat(forge build): add initcode size check feat(forge build): add initcode size check Oct 15, 2024
@grandizzy grandizzy self-requested a review October 15, 2024 15:25
grandizzy
grandizzy approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

thank you, lgtm! Pending other review

@zerosnacks zerosnacks self-requested a review October 15, 2024 18:37
zerosnacks
zerosnacks approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good 👍

DaniPopes
DaniPopes requested changes Oct 16, 2024
View reviewed changes
crates/common/src/compile.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/build.rs Outdated Show resolved Hide resolved
crates/forge/tests/cli/test_helpers.rs Outdated Show resolved Hide resolved
crates/forge/tests/cli/build.rs Outdated Show resolved Hide resolved
crates/forge/tests/cli/build.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks self-requested a review October 16, 2024 10:44
@mgiagante mgiagante requested a review from DaniPopes October 16, 2024 15:19
@mgiagante
Copy link
Contributor Author

@DaniPopes Please let me know if this needs any further improvements before it can be merged. Thank you!

DaniPopes
DaniPopes approved these changes Oct 19, 2024
View reviewed changes
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks!

@DaniPopes DaniPopes enabled auto-merge (squash) October 19, 2024 11:54
@DaniPopes DaniPopes merged commit 8bdcbfa into foundry-rs:master Oct 19, 2024
21 checks passed
@nbaztec nbaztec mentioned this pull request Nov 5, 2024
Merged
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
@mgiagante @zerosnacks @rplusq
feat(forge build): add initcode size check (foundry-rs#9116)
a87c96f 
* Adds init code size limit check & column to table.

* Adds option to ignore init code size check during --size output.

* Updates tests with new column for --sizes table.

* Adds test helpers for forge CLI.

* Implements test for init code size limit as per EIP-3860

* Adds test for --ignore-eip-3860

* Fixes for Cargo +nightly fmt warnings.

* Refactors both contract size functions into one with a boolean arg.

* Adds alias for --ignore-eip-3860 to --ignore-initcode-size.

* Brings back the original comments.

* Update compile.rs

* Changes --ignore-eip-3860 to be a boolean field.

* Fixes ranges in table display code and comment punctuation.

* Moves testing helper to existing utils module.

* Improve ranges in table display code.

* Adds output assertions to initcode size check tests.

* Minor change to ranges in display logic for sizes table.

---------

Co-authored-by: mgiagante <251503-mgiagante@users.noreply.gitlab.com>
Co-authored-by: zerosnacks <95942363+zerosnacks@users.noreply.github.com>
@grandizzy grandizzy added T-feature Type: feature C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mds1 mds1 mds1 left review comments

@DaniPopes DaniPopes DaniPopes approved these changes

@grandizzy grandizzy grandizzy approved these changes

@klkvr klkvr Awaiting requested review from klkvr klkvr is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@yash-atreya yash-atreya Awaiting requested review from yash-atreya yash-atreya is a code owner

@zerosnacks zerosnacks Awaiting requested review from zerosnacks zerosnacks is a code owner

Assignees
No one assigned
Labels
C-forge Command: forge T-feature Type: feature
Projects
Foundry
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update forge build --sizes to add initcode size column + check for EIP3860 limit
6 participants
@mgiagante @NelsonGaldeman @mds1 @grandizzy @DaniPopes @zerosnacks