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

fix: Inconsistent limit on InfiniteGasMeter and add GasLeft func to GasMeter #9651

Merged

Conversation

likhita-809
Copy link
Contributor

Description

Closes: #9577


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #9651 (1d1bf27) into master (0027111) will increase coverage by 27.91%.
The diff coverage is 63.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9651       +/-   ##
===========================================
+ Coverage   35.48%   63.40%   +27.91%     
===========================================
  Files         332      572      +240     
  Lines       32620    37555     +4935     
===========================================
+ Hits        11575    23810    +12235     
+ Misses      19819    11887     -7932     
- Partials     1226     1858      +632     
Impacted Files Coverage Δ
client/keys/types.go 100.00% <ø> (+100.00%) ⬆️
client/query.go 16.98% <ø> (ø)
client/rpc/block.go 0.00% <ø> (ø)
client/rpc/status.go 67.74% <ø> (ø)
client/rpc/validators.go 0.00% <ø> (ø)
client/test_helpers.go 0.00% <ø> (ø)
client/tx/factory.go 27.00% <ø> (ø)
client/tx/legacy.go 68.42% <ø> (ø)
client/tx/tx.go 40.83% <ø> (ø)
client/utils.go 41.93% <ø> (-41.40%) ⬇️
... and 682 more

…khita/fix-inconsistent-limit-in-InfiniteGasMeter
…khita/fix-inconsistent-limit-in-InfiniteGasMeter
store/types/gas.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ruhatch ruhatch left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but I'm not clear why this is the correct thing to do. Could you add some godocs to the gas meter methods that explain why this makes sense?

Copy link
Contributor

@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

@alexanderbez alexanderbez merged commit 43e0598 into master Jul 9, 2021
@alexanderbez alexanderbez deleted the likhita/fix-inconsistent-limit-in-InfiniteGasMeter branch July 9, 2021 14:38
@likhita-809
Copy link
Contributor Author

likhita-809 commented Jul 9, 2021

This seems reasonable, but I'm not clear why this is the correct thing to do. Could you add some godocs to the gas meter methods that explain why this makes sense?

I created a follow-up PR for this on 9665

@ethanfrey
Copy link
Contributor

Thank you.
@alpe note this so we can eventually remove our workaround (with 0.43?)

mergify bot pushed a commit that referenced this pull request Jul 12, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: #9651 

This is a follow-up PR of adding godocs to GasMeter methods

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
spoo-bar added a commit to archway-network/archway that referenced this pull request Oct 4, 2023
spoo-bar added a commit to archway-network/archway that referenced this pull request Oct 4, 2023
* fixing lint issues

* fixing x/tracking tests

* fixing app_test.go

* replacing gov v1beta1 stuff with gov v1

* fixig app-upgrade-test

txRes.Data is deprecated in favour of txRes.MsgResponses

* removing depracated field usage

* fixing ibctm not setup

* making the flat fees e2e test simpler

* fixing interchaintest chain upgrade

* fixing TestTxFees thanks @fdymylja 🎉

* fixing more tests with initgenesis failure

* fixing testcase to be compatible with this change cosmos/cosmos-sdk#9651

* removing the ante.DeductFeeDecorator as we alreayd have a custom one

* fixing TestRewardsFlatFees

* undo the mintkeeper fix
spoo-bar added a commit to archway-network/archway that referenced this pull request Oct 13, 2023
* chore: replacing tendermint with cometbft (#433)

* replacing tendermint refs with comet bft

* replacing tm-db with cometbft-db

* chore: Bumping sdk, wasmd and ibc-go (#435)

* bumping sdk and wasmd and ibc versions

* bumping ibc-go refs

* chore: protobuf migration (#436)

* protobuf migration

* removing third_party/

* regenerating the proto files

* RecordIDs.Ids is a repeated non-nullable native type, nullable=false has no effect

* chore: fix broken upstream references (#450)

updating upstream references for x/wasm. ibc-go, cosmos-sdk

* chore: remove deprecated appmodule routes (#451)

* remove genmsg deprecated route

* remove rewards module deprecated route

* remove tracking module deprecated route

* chore: fixing all the modules and wasmbindings   (#452)

* fixing x/tracking module

* fixing x/rewards module

* fixing x/genmsg module

* fixing genmsg module.go

* fixing wasmbinding

* chore: updating app wiring (#453)

* fixing the upgrade handlers

* fixing ante.go

* fixing export.go

* updating sim_test

* fixing test_helpers

* fixing some app.go stuff

* custom "add-genesis-account" - removed as its not needed anymore

* fixing root.go

* fixing simulation test

* fixing app.go

* cleanup

* chore: fixing e2e tests (#454)

* fixing some e2e tests

* fix more e2e tests

* fixing ibc e2e stuff

* updating changes from merge

* fixing golang.org/x/exp ref. thx @fdymylja

Co-Authored-By: Frojdi Dymylja <33157909+fdymylja@users.noreply.github.com>

* pr comment fixes

---------

Co-authored-by: Frojdi Dymylja <33157909+fdymylja@users.noreply.github.com>

* chore: apply v46 changes (#455)

* adding posthandler and reflection services

* updating `sdkerrors.Wrap` with `errorsmod`

* every cosmos message protobuf definition must be extended with a cosmos.msg.v1.signer option to signal the signer fields

* better error for when post handler fails

* running go mod tidy

* registering query server after module init

---------

Signed-off-by: Spoorthi <9302666+spoo-bar@users.noreply.github.com>

* chore: apply v47 changes (#458)

* adding crisis module store key

* adding crisis module

* adding consensus module

* adding group module

* adding nft module

* fixing the merge

* fixing gentx decoding

* adding consensusparamtypes.StoreKey storekey && fixing staking keeper

---------

Signed-off-by: Spoorthi <9302666+spoo-bar@users.noreply.github.com>

* feat: bumping archway wasmd to v0.42.0 (#460)

* updating to wasmd fork 0.42.0

* adding capability cosmwasm_1_4

* updating v1.4 libwasmvm checksums

* chore: fixing linter errors and tests (#463)

* fixing lint issues

* fixing x/tracking tests

* fixing app_test.go

* replacing gov v1beta1 stuff with gov v1

* fixig app-upgrade-test

txRes.Data is deprecated in favour of txRes.MsgResponses

* removing depracated field usage

* fixing ibctm not setup

* making the flat fees e2e test simpler

* fixing interchaintest chain upgrade

* fixing TestTxFees thanks @fdymylja 🎉

* fixing more tests with initgenesis failure

* fixing testcase to be compatible with this change cosmos/cosmos-sdk#9651

* removing the ante.DeductFeeDecorator as we alreayd have a custom one

* fixing TestRewardsFlatFees

* undo the mintkeeper fix

* feat: adding upgrade handlers for sdk v47 (#464)

* adding missing consensus keeper

* adding upgrade handler stuff

* oops messed the merge. fixing that

---------

Signed-off-by: Spoorthi <9302666+spoo-bar@users.noreply.github.com>

* feat: enabling rosetta  (#466)

adding rosetta

* fix: updating logic for how we deal with infiniteGasMeter (#467)

* handling behavior change in infinitegasmeter.Limit()

* reflect no mo

* feat: x/rewards param store migration (#468)

* adding new msg proto

* implementing UpdateParams msg

* adding tests

* adding module migrations forx/rewards

* adding x/rewards to upgrade handlers

* adding migratestore test

* fixing lint

* addressing pr review comments

* Update CHANGELOG.md

* feat: bumping archway wasmd to v0.43.0 (#470)

* bumping wasmd fork to to v0.43.0

* updating wasmvm checksum

* removing wasmd deprecated proposals as they arent in the codebase anymore

* Updating changelog

* test: fixing ibc conformance test (#469)

fixing ibc conformance test. the custom genesis values are needed just for upgrade test

---------

Signed-off-by: Spoorthi <9302666+spoo-bar@users.noreply.github.com>
Co-authored-by: Frojdi Dymylja <33157909+fdymylja@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent Limit in InfiniteGasMeter
5 participants