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

Increase and document the MaxGas limit #193

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Conversation

ethanfrey
Copy link
Member

Closes #170
Replaces #188 (which shows why we cannot just remove the field)

We document where the limit comes from and link to the rust code.

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added relevant godoc comments.

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Re-reviewed Files changed in the Github PR explorer


For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@ethanfrey ethanfrey mentioned this pull request Jul 15, 2020
9 tasks
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #193 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   71.58%   71.50%   -0.08%     
==========================================
  Files          27       27              
  Lines        2569     2569              
==========================================
- Hits         1839     1837       -2     
- Misses        620      622       +2     
  Partials      110      110              
Impacted Files Coverage Δ
x/wasm/internal/keeper/keeper.go 93.60% <ø> (ø)
lcd_test/helpers.go 75.00% <0.00%> (-0.72%) ⬇️

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 3dc9bed...7f02f57. Read the comment docs.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Isn't there any way to move this into the testing framework?

I don't think we should silently change the value for the user if unconfigured.

@ethanfrey
Copy link
Member Author

Isn't there any way to move this into the testing framework?
I don't think we should silently change the value for the user if unconfigured.

I don't understand.

In any production code, there will be a gas limit set and it will be below this.
If we use InfiniteGasMeter this will be an issue.

We cannot "patch the testing framework", as there is nothing to patch there. They setup a test context and call the keeper methods. I think any other change would be more invasive.

This was working for quite some time, I just added docs and increased the value (in 0.9->0.10 which allows breaking changes)

@webmaster128
Copy link
Member

I don't understand.

In any production code, there will be a gas limit set and it will be below this.
If we use InfiniteGasMeter this will be an issue.

The existence of MaxGas adds logic to production code, which should not be there for any production use. The variable and its usage is only needed because the testing framework needs it. So the question was if there is a way to get this out of production code. But given your reply this does not seem possible.

I think any other change would be more invasive.

Yeah, if you don't see a way to move this into testing code then we have to live with it.

@ethanfrey ethanfrey merged commit 84bcb39 into master Jul 15, 2020
@ethanfrey ethanfrey deleted the document-max-gas branch July 15, 2020 18:41
zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
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.

Remove keeper.MaxGas or write a comment why it is needed
2 participants