-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor!: Refactor CommunityPoolSpendProposal #12852
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12852 +/- ##
==========================================
- Coverage 55.87% 55.70% -0.18%
==========================================
Files 646 648 +2
Lines 54895 54940 +45
==========================================
- Hits 30675 30606 -69
- Misses 21762 21863 +101
- Partials 2458 2471 +13
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (just missing changelog entry)
@@ -166,3 +166,37 @@ func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParam | |||
|
|||
return &types.MsgUpdateParamsResponse{}, nil | |||
} | |||
|
|||
func (k msgServer) CommunityPoolSpend(goCtx context.Context, req *types.MsgCommunityPoolSpend) (*types.MsgCommunityPoolSpendResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should bump consensus version here (if it wasn't done already since 0.46)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to bump consensus versions even when no state changed, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes when a module introduces a consensus-breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 2 on v0.46 and thus should be 3
here. Not sure why it got changed to 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the technical side. Would still like approval on gov people on the UX side
minttypes.ModuleName, | ||
stakingtypes.BondedPoolName, | ||
stakingtypes.NotBondedPoolName, | ||
govtypes.ModuleName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious about this line, why is it blocked while it is commented below that it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
This PR introduces the ability for v1
x/gov
to execute a message that directly spends tokens from the community pool opposed to having to execute the legacyCommunityPoolSpendProposal
proposal.This provides the ability for governance to spend tokens from the community pool, either to itself (
x/gov
module account) or to another account.Changelog
CommunityPoolSpendProposal
MsgCommunityPoolSpend
withx/gov
as the authority (by default)x/gov
from SimApps'sx/bank
deny list (so it can directly received funds)ref: #12831 (comment)
closes: #12098
cc @hxrts @nooomski
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change