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

Move constants to params #235

Closed
wants to merge 2 commits into from
Closed

Move constants to params #235

wants to merge 2 commits into from

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Jul 28, 2020

🚧 WIP

Resolves #202

In Params now:

  • GasMultiplier
  • InstanceCost
  • CompileCost

NOTE: the changes come with a significant increase in gas costs (in our test setup). The work is on hold for >0.10

TODO:

  • move other constants
  • proper tests

@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #235 into master will decrease coverage by 0.05%.
The diff coverage is 85.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   72.34%   72.28%   -0.06%     
==========================================
  Files          27       27              
  Lines        2632     2659      +27     
==========================================
+ Hits         1904     1922      +18     
- Misses        616      623       +7     
- Partials      112      114       +2     
Impacted Files Coverage Δ
x/wasm/internal/types/params.go 54.54% <30.00%> (-3.15%) ⬇️
x/wasm/internal/keeper/api.go 85.71% <91.66%> (+14.28%) ⬆️
x/wasm/internal/keeper/keeper.go 90.86% <97.43%> (-0.83%) ⬇️
lcd_test/helpers.go 75.71% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23d1fd8...55153e8. Read the comment docs.

@ethanfrey
Copy link
Member

I would leave this on hold for a bit to think if we want to. The gas costs are an interesting factor (5x reads?)

It would need to be for 0.11 in any case.

@ethanfrey ethanfrey added the on hold Let's pause this and come back in a week... label Aug 6, 2020
@ethanfrey
Copy link
Member

Nice idea. Let's not do this for 1.0. Maybe 2.0

@ethanfrey ethanfrey closed this Aug 18, 2020
@ethanfrey
Copy link
Member

Do not delete branch

@alpe alpe deleted the more_params_202 branch September 15, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Let's pause this and come back in a week...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move constants to params
2 participants