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/authz: Add DelegateAuthorization, UndelegateAuthorization #8472

Merged
merged 47 commits into from
Feb 20, 2021

Conversation

aleem1314
Copy link
Contributor

Description

ref: #8312


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 Jan 29, 2021

Codecov Report

Merging #8472 (012cc59) into master (c6355fd) will increase coverage by 0.04%.
The diff coverage is 70.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8472      +/-   ##
==========================================
+ Coverage   61.43%   61.47%   +0.04%     
==========================================
  Files         658      659       +1     
  Lines       37768    37876     +108     
==========================================
+ Hits        23201    23283      +82     
- Misses      12143    12159      +16     
- Partials     2424     2434      +10     
Impacted Files Coverage Δ
x/authz/types/authorization_grant.go 0.00% <0.00%> (ø)
x/authz/types/codec.go 0.00% <0.00%> (ø)
x/authz/types/generic_authorization.go 0.00% <0.00%> (ø)
x/authz/types/genesis.go 0.00% <0.00%> (ø)
x/authz/types/msgs.go 53.84% <0.00%> (ø)
x/bank/types/send_authorization.go 0.00% <0.00%> (ø)
x/authz/client/cli/tx.go 69.82% <64.00%> (-3.05%) ⬇️
x/authz/simulation/operations.go 81.11% <66.66%> (ø)
x/staking/types/authz.go 87.87% <87.87%> (ø)
x/auth/client/cli/tx_multisign.go 62.03% <100.00%> (+0.35%) ⬆️
... and 5 more

@anilcse anilcse changed the title x/authz authorizations x/authz: Add DelegateAuthorization Jan 29, 2021
proto/cosmos/authz/v1beta1/authz.proto Outdated Show resolved Hide resolved
@aleem1314 aleem1314 marked this pull request as ready for review January 30, 2021 11:45
@aleem1314 aleem1314 requested a review from anilcse January 30, 2021 11:45
@aleem1314 aleem1314 changed the title x/authz: Add DelegateAuthorization x/authz: Add DelegateAuthorization, UndelegateAuthorization Feb 1, 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, just some nits/refactoring requests.

x/authz/exported/authorizations.go Show resolved Hide resolved
x/staking/types/undelegate_authorization.go Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

As @blushi pointed out these are essentially the same message. So we can just include a base message and two types or something like that. Also I imagine we could cover redelegate similarly.

@aleem1314 aleem1314 requested a review from blushi February 10, 2021 17:20
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.

Pre-approving with a few non-blocking comments!

x/authz/client/cli/tx.go Show resolved Hide resolved
x/authz/client/cli/tx.go Outdated Show resolved Hide resolved
x/authz/client/cli/tx.go Outdated Show resolved Hide resolved
x/staking/types/authz_authorization.go Outdated Show resolved Hide resolved
@aleem1314 aleem1314 requested a review from aaronc February 11, 2021 11:50
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

API looks good now. Let's clean up the docs a bit

proto/cosmos/staking/v1beta1/authz.proto Outdated Show resolved Hide resolved
proto/cosmos/staking/v1beta1/authz.proto Outdated Show resolved Hide resolved
proto/cosmos/staking/v1beta1/authz.proto Outdated Show resolved Hide resolved
@aaronc aaronc dismissed their stale review February 11, 2021 19:01

Dismissing as my change requests have been addressed

Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

lgtm

proto/cosmos/bank/v1beta1/authz.proto Outdated Show resolved Hide resolved
@aleem1314 aleem1314 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 20, 2021
@mergify mergify bot merged commit 7df79b5 into master Feb 20, 2021
@mergify mergify bot deleted the aleem/8312-authz-authorizations branch February 20, 2021 06:57
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:x/authz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants