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

add MsgVote authorization #8462

Closed
wants to merge 3 commits into from
Closed

add MsgVote authorization #8462

wants to merge 3 commits into from

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 28, 2021

Codecov Report

Merging #8462 (65d42cd) into master (580e968) will decrease coverage by 0.01%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8462      +/-   ##
==========================================
- Coverage   61.48%   61.46%   -0.02%     
==========================================
  Files         630      631       +1     
  Lines       36398    36408      +10     
==========================================
  Hits        22379    22379              
- Misses      11670    11679       +9     
- Partials     2349     2350       +1     
Impacted Files Coverage Δ
x/authz/types/authorizations.go 0.00% <ø> (ø)
x/authz/types/codec.go 0.00% <0.00%> (ø)
x/authz/types/generic_authorization.go 0.00% <0.00%> (ø)
x/authz/types/gov_vote_authorization.go 0.00% <0.00%> (ø)
x/authz/types/send_authorization.go 7.69% <0.00%> (ø)
x/authz/client/cli/tx.go 73.28% <100.00%> (+0.41%) ⬆️
x/authz/keeper/keeper.go 71.29% <100.00%> (ø)
x/authz/simulation/operations.go 81.11% <100.00%> (ø)
snapshots/store.go 74.31% <0.00%> (-1.10%) ⬇️

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, Just one query

if !allow {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

@@ -20,7 +20,7 @@ type Authorization interface {

// Accept determines whether this grant permits the provided sdk.ServiceMsg to be performed, and if
// so provides an upgraded authorization instance.
Accept(msg sdk.ServiceMsg, block tmproto.Header) (allow bool, updated Authorization, delete bool)
Accept(msg sdk.ServiceMsg, block tmproto.Header) (allow bool, updated Authorization, delete bool, err error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

We can just replace allow with error if this is needed

proto/cosmos/authz/v1beta1/authz.proto Outdated Show resolved Hide resolved

// VoteAuthorization allows the grantee to vote on a proposal on behalf of the granter's account.
message VoteAuthorization {
option (cosmos_proto.implements_interface) = "Authorization";
Copy link
Collaborator

@anilcse anilcse Jan 28, 2021

Choose a reason for hiding this comment

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

How about authorizing for specific type of proposals? That would be more granular I believe.
ProposalType: Any

It can have one of All, TextProposal, SoftwareUpgradeProposal, ParamChangeProposal etc.

@amaurymartiny any thoughts

Copy link
Member

Choose a reason for hiding this comment

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

I agree 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get how we can achieve this, Accept method doesn't have access to the state/keeper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes, we should find a cleaner way to handle this

Copy link
Member

Choose a reason for hiding this comment

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

So we can use GenericAuthorization if there is nothing special to check here. Adding state access seems out of scope and feels like we should address with the ADR 033 approach if we do...

@aleem1314 aleem1314 marked this pull request as draft January 28, 2021 16:23
@aaronc
Copy link
Member

aaronc commented Jan 28, 2021

So I think that maybe this is superfluous unfortunately. Everything we can do here without state access can be done with GenericAuthorization... So maybe we close this PR and open an issue to discuss state access in authorizations. I think it should basically be the ability to query read only state using ADR033... but there is a dependency on the ADR 033 code which hasn't been upstreamed yet. So we should address in the next release instead.

@anilcse
Copy link
Collaborator

anilcse commented Jan 28, 2021

Closing this for now, follow-up discussion : #8468

@anilcse anilcse closed this Jan 28, 2021
@anilcse anilcse deleted the aleem/8312-msg-vote branch January 28, 2021 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants