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

x/feegrant remove height base expiration #9206

Merged
merged 44 commits into from
May 5, 2021

Conversation

aleem1314
Copy link
Contributor

@aleem1314 aleem1314 commented Apr 27, 2021

Description

ref: #9115

Get rid of block height in ExportGenesis and remove height support in general and get rid of PrepareForExport method from FeeAllowanceI interface.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • 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
  • Review Codecov Report in the comment section below once CI passes

@blushi blushi mentioned this pull request Apr 27, 2021
7 tasks
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #9206 (18ce215) into master (42a4e4b) will increase coverage by 0.03%.
The diff coverage is 88.23%.

❗ Current head 18ce215 differs from pull request most recent head cc2f1e8. Consider uploading reports for the commit cc2f1e8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9206      +/-   ##
==========================================
+ Coverage   60.13%   60.16%   +0.03%     
==========================================
  Files         595      594       -1     
  Lines       37185    37106      -79     
==========================================
- Hits        22360    22325      -35     
+ Misses      12846    12805      -41     
+ Partials     1979     1976       -3     
Impacted Files Coverage Δ
x/feegrant/types/filtered_fee.go 0.00% <ø> (ø)
x/feegrant/types/grant.go 70.37% <ø> (+26.18%) ⬆️
x/feegrant/types/periodic_fee.go 68.42% <77.77%> (+12.17%) ⬆️
x/feegrant/genesis.go 90.47% <100.00%> (-0.44%) ⬇️
x/feegrant/simulation/genesis.go 79.48% <100.00%> (ø)
x/feegrant/simulation/operations.go 77.19% <100.00%> (+0.20%) ⬆️
x/feegrant/types/basic_fee.go 88.23% <100.00%> (+13.23%) ⬆️
x/feegrant/keeper/keeper.go 85.05% <0.00%> (ø)
x/bank/types/balance.go 89.18% <0.00%> (+2.52%) ⬆️

@github-actions github-actions bot added C:CLI C:Simulations C:x/feegrant T:Docs Changes and features related to documentation. labels Apr 28, 2021
@aleem1314 aleem1314 marked this pull request as ready for review April 28, 2021 07:58
x/feegrant/client/cli/tx.go Outdated Show resolved Hide resolved
x/feegrant/genesis_test.go Outdated Show resolved Hide resolved
x/feegrant/types/basic_fee.go Outdated Show resolved Hide resolved
x/feegrant/types/basic_fee.go Outdated Show resolved Hide resolved
x/feegrant/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/feegrant/types/grant_test.go Show resolved Hide resolved
atheeshp and others added 2 commits May 4, 2021 15:45
@aleem1314 aleem1314 removed C:Simulations T:Docs Changes and features related to documentation. labels May 4, 2021
Copy link
Contributor

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Looks good overall but could you update the ADR as well?

@github-actions github-actions bot added C:Simulations T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation. labels May 5, 2021
@aleem1314 aleem1314 removed C:Simulations T: ADR An issue or PR relating to an architectural decision record labels May 5, 2021
@aleem1314 aleem1314 requested a review from blushi May 5, 2021 06:38
docs/architecture/adr-029-fee-grant-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-029-fee-grant-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-029-fee-grant-module.md Show resolved Hide resolved
@github-actions github-actions bot added C:Simulations T: ADR An issue or PR relating to an architectural decision record labels May 5, 2021
@aleem1314 aleem1314 requested a review from blushi May 5, 2021 09:01
Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@aleem1314 aleem1314 added the A:automerge Automatically merge PR once all prerequisites pass. label May 5, 2021
@mergify mergify bot merged commit 1e1c812 into master May 5, 2021
@mergify mergify bot deleted the aleem/9115-feegrant-remove-heght branch May 5, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Simulations C:x/feegrant T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants