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

Increasing allowed wasm bytecode size #4174

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Conversation

nicolaslara
Copy link
Contributor

Closes: #4171

What is the purpose of the change

Increase the max allowed wasm size to 2MB

Brief Changelog

  • Increased max allowed wasm size to 2MB

Testing and Verifying

Added an E2E test uploading a large file. The test fails without the change.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jan 31, 2023
@nicolaslara nicolaslara added the V:state/breaking State machine breaking PR label Jan 31, 2023
@Magicloud
Copy link

I tested with our contract. It works.

Comment on lines +165 to +169
func overrideWasmVariables() {
// Override Wasm size limitation from WASMD.
wasmtypes.MaxWasmSize = 2 * 1024 * 1024
wasmtypes.MaxProposalWasmSize = wasmtypes.MaxWasmSize
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we later make this a wasm option, and move this function elsewhere?

https://github.com/CosmWasm/wasmd/blob/88e01a98ab8a87b98dc26c03715e6aef5c92781b/x/wasm/keeper/keeper.go#L43-L45

If so we can make a followup issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I feel like wasmd configs are all over the place, but I don't know if there's a reason for that. For example, I often use a local fork to override defaultContractDebugMode (using --trace also adds that, but I think that doesn't work on tests and I don't want other side effects of adding trace).

But maybe there's something I'm missing. Do you know the right way to do this with apply()? Is that something we can define on osmosis or would it need to change in wasmd?

Copy link
Member

Choose a reason for hiding this comment

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

OH its a lowercase apply not uppercase. I dont think we can implement the interface outside the repo, RIP.

Lets at least bundle all our wasm options, into perhaps an app/wasm.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added #4206 to reorganize this in the future.

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM, but needs state breaking changelog. (And can you update #4170 )

@nicolaslara nicolaslara merged commit 8550c87 into main Feb 2, 2023
@nicolaslara nicolaslara deleted the nicolas/max_wasm_size branch February 2, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder V:state/breaking State machine breaking PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Chore: Increase wasm size limit
4 participants