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

feat: Add amino support for x/authz and x/feegrant #9457

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Conversation

amaury1093
Copy link
Contributor

Description

closes: #9417


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

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #9457 (148135e) into master (33c045c) will increase coverage by 0.10%.
The diff coverage is 75.54%.

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

@@            Coverage Diff             @@
##           master    #9457      +/-   ##
==========================================
+ Coverage   60.39%   60.50%   +0.10%     
==========================================
  Files         589      590       +1     
  Lines       37099    37342     +243     
==========================================
+ Hits        22405    22592     +187     
- Misses      12715    12769      +54     
- Partials     1979     1981       +2     
Impacted Files Coverage Δ
types/coin.go 93.47% <ø> (ø)
x/authz/msgs.go 51.13% <0.00%> (-5.83%) ⬇️
x/feegrant/msgs.go 63.26% <0.00%> (-8.83%) ⬇️
x/gov/client/cli/query.go 0.00% <0.00%> (ø)
x/gov/client/utils/query.go 23.67% <0.00%> (-4.49%) ⬇️
x/gov/client/testutil/deposits.go 93.75% <93.75%> (ø)
store/types/gas.go 81.48% <100.00%> (+2.31%) ⬆️
x/authz/client/testutil/tx.go 97.78% <100.00%> (+0.15%) ⬆️
x/authz/simulation/operations.go 77.51% <100.00%> (-0.14%) ⬇️
x/feegrant/client/testutil/suite.go 99.68% <100.00%> (+0.01%) ⬆️
... and 3 more

@amaury1093 amaury1093 marked this pull request as ready for review June 3, 2021 12:30
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK, changelog please 😄

Do simulations need to be updated as well?


// Route implements the LegacyMsg.Type method.
func (msg MsgGrant) Route() string {
return sdk.MsgTypeURL(&msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Authz has no legacy msg handler, so this is dead code. I'm actually inclined to panic here with a user friendly message saying so. wdyt?

@amaury1093
Copy link
Contributor Author

I'm actually not sure myself about sims. @aleem1314 @atheeshp do simulations also need updating?

@aleem1314
Copy link
Contributor

I'm actually not sure myself about sims. @aleem1314 @atheeshp do simulations also need updating?

Yes. Small update. protoCdc is not required for legacy messages.

x/authz/msgs.go Outdated Show resolved Hide resolved
x/authz/msgs.go Outdated Show resolved Hide resolved
x/authz/msgs.go Outdated Show resolved Hide resolved
x/feegrant/msgs.go Outdated Show resolved Hide resolved
x/feegrant/msgs.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor Author

Thanks @likhita-809 for the review!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 4, 2021
@mergify mergify bot merged commit ec3e2b4 into master Jun 4, 2021
@mergify mergify bot deleted the am/9417-amino branch June 4, 2021 09:30
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/authz C:x/feegrant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amino support for x/authz & x/feegrant
5 participants