-
Notifications
You must be signed in to change notification settings - Fork 710
Increase Stylus smart contract size limit via merge-on-activate #4193
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
Conversation
53db006 to
e4b1efe
Compare
e12128c to
4446026
Compare
4446026 to
1f80b39
Compare
❌ 4 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
1f80b39 to
9c6a7fc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4193 +/- ##
==========================================
- Coverage 32.94% 32.62% -0.32%
==========================================
Files 471 471
Lines 56610 56670 +60
==========================================
- Hits 18648 18487 -161
- Misses 34713 34948 +235
+ Partials 3249 3235 -14 |
607f160 to
7c85084
Compare
6658c10 to
8bbc36d
Compare
|
@pmikolajczyk41 ready for another look 🫡 |
8bbc36d to
a1c8594
Compare
tsahee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looking good and very close. Tiny comment, since you also need to solve a merge conflict
|
@tsahee ready for another look |
tsahee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly small comments
| stylusParams.MaxWasmSize = initialMaxWasmSize | ||
| } | ||
| if p.ArbosVersion >= params.ArbosVersion_StylusContractLimit { | ||
| stylusParams.MaxFragmentCount = arbmath.BytesToUint16(take(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use uint8 and only take 1.
255 fragments should be enough, and we're very close to the 32byte limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok here is the PR for that #4285
| return nil, ProgramNotWasmError() | ||
| } | ||
|
|
||
| func handleClassicStylus(data []byte, maxSize uint32) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a style question, but I don't like this function name. It's visible/theoretically callable from the entire module and "handle" doesn't mean anything for that context. Suggest something like "getWasmFromClassicStylus" or something similar.
| return arbcompress.DecompressWithDictionary(wasm, int(maxSize), dict) | ||
| } | ||
|
|
||
| func handleRootStylus(statedb vm.StateDB, data []byte, maxSize uint32, maxFragments uint16, isActivation bool) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Fixes NIT-4374
https://linear.app/offchain-labs/project/increase-stylus-smart-contract-size-limit-via-merge-on-activate-e809a493a62b/overview
Pulls in OffchainLabs/go-ethereum#605
Pulls in OffchainLabs/nitro-precompile-interfaces#27