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

Consolidate MaxWasmSize constraints into a single var #826

Merged
merged 5 commits into from
Apr 29, 2022
Merged

Consolidate MaxWasmSize constraints into a single var #826

merged 5 commits into from
Apr 29, 2022

Conversation

alpe
Copy link
Member

@alpe alpe commented Apr 29, 2022

Resolves #813

This PR contains all work started in #809 by @bigs rebased on top of current main.
Big thanks for the contribution and driving this forward! 💐

Note: with this PR the max code size was changed from 500kb to 800kb

@alpe alpe changed the title 809 fix Consolidate MaxWasmSize constraints into a single var Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #826 (3b573eb) into main (8ca55b7) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   59.37%   59.32%   -0.06%     
==========================================
  Files          51       51              
  Lines        5898     5883      -15     
==========================================
- Hits         3502     3490      -12     
+ Misses       2144     2142       -2     
+ Partials      252      251       -1     
Impacted Files Coverage Δ
x/wasm/types/params.go 60.21% <ø> (-1.33%) ⬇️
x/wasm/types/validation.go 100.00% <ø> (ø)
x/wasm/keeper/keeper.go 87.74% <100.00%> (-0.09%) ⬇️

Copy link
Contributor

@bigs bigs left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for nudging it over the line while I was indisposed!

Copy link
Contributor

@pinosu pinosu left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@alpe alpe merged commit bfb4d31 into main Apr 29, 2022
@alpe alpe deleted the 809_fix branch April 29, 2022 16:54
@jhernandezb
Copy link
Contributor

jhernandezb commented Apr 29, 2022

I'm worried that this could break existing chains without a state migration: remove an existing param from ParamSetPairs table, removing field from protobuf.

  • What will happen when chains upgrade and params are retrieved? will ignore the previous existence of the param or fail?
  • What will happen to a chain data export ?

See an example of adding a new param key https://github.com/cosmos/cosmos-sdk/blob/main/x/staking/migrations/v046/store.go#L21

Do we need to add such migration to remove it ?

@bigs
Copy link
Contributor

bigs commented Apr 29, 2022

@jhernandezb Good questions! As far as breaking changes go, I think the only time you'd experience breaking behavior is if your param was set to a value lower than the 500KB that was defined as a constant in the validation section.

The old param not being present shouldn't break anything, but I do agree it would be tidier to write a migration to remove it from the store, as it certainly won't be used anymore.

@jhernandezb
Copy link
Contributor

I'll add an upgrade test to our chain code targeting this branch and see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace many const with var
4 participants