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.

Sign up for GitHub

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

Jump to bottom

x/gov: allow arbitrary messages in proposal #9726

Closed
wants to merge 1 commit into from
Closed

x/gov: allow arbitrary messages in proposal #9726

wants to merge 1 commit into from

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Jul 19, 2021 •
edited

Description

Closes: #9438

NOTE: I will most likely break this up into a few smaller PR's given the amount of files it touches

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@orijbot
Copy link

orijbot commented Jul 19, 2021

Visit https://dashboard.github.orijtech.com?pr=9726&repo=cosmos%2Fcosmos-sdk to see benchmark details.

aaronc
aaronc reviewed Jul 19, 2021
View reviewed changes
proto/cosmos/gov/v1beta1/tx.proto
Comment on lines +98 to +106
message MsgSignal {
option (gogoproto.equal) = true;
option (gogoproto.goproto_stringer) = false;
option (gogoproto.stringer) = false;
option (gogoproto.goproto_getters) = false;

string title = 1;
string description = 2;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we just add title & description fields to MsgSubmitProposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the alternative to adding a signal message in the proposal. I wasn't sure if all proposals necessary needed a title and a description. In some cases a link/address that points to the forum / proposal write up might suffice

Copy link
Member

Choose a reason for hiding this comment

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

I think a link is more common and maybe a better idea. We should discuss. I don't think the title and description metadata should extend to group proposals for a number of reasons. Some groups may be want to be more anonymous and including title and description could confuse users into sharing private info on a public blockchain.

blushi
blushi reviewed Jul 20, 2021
View reviewed changes
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.

Thanks @cmwaters! I left a few small comments.
I think it would make sense indeed to split it up into smaller PRs: one for each new Msg and one for the gov changes, as suggested by @aaronc
This will also make it easier to make sure that new code has good test coverage.

proto/cosmos/distribution/v1beta1/tx.proto
@@ -24,6 +24,10 @@ service Msg {
// FundCommunityPool defines a method to allow an account to directly
// fund the community pool.
rpc FundCommunityPool(MsgFundCommunityPool) returns (MsgFundCommunityPoolResponse);

// SpendCommunityPool defined a method to transfer tokens from the community
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// SpendCommunityPool defined a method to transfer tokens from the community
// SpendCommunityPool defines a method to transfer tokens from the community

simapp/app.go
@@ -272,7 +272,7 @@ func NewSimApp(
AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper))
govKeeper := govkeeper.NewKeeper(
appCodec, keys[govtypes.StoreKey], app.GetSubspace(govtypes.ModuleName), app.AccountKeeper, app.BankKeeper,
&stakingKeeper, govRouter,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just get rid of the govRouter declaration above and delete x/gov/types/router.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is my intention.

NOTE: I just published the PR so aaron and I could discuss a few points. It isn't quite ready

x/distribution/keeper/keeper.go
@@ -181,3 +184,26 @@ func (k Keeper) FundCommunityPool(ctx sdk.Context, amount sdk.Coins, sender sdk.

return nil
}

// SpendCommunityPool allows an authority to send coins to a recipient account.
// An error is returned if not sufficient funds exist all the caller of the msg
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// An error is returned if not sufficient funds exist all the caller of the msg
// An error is returned if not sufficient funds exist or the caller of the msg

x/distribution/keeper/msg_server.go
ctx.EventManager().EmitEvent(
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we should emit this event here

x/gov/client/cli/tx.go

Example:
$ %s tx gov submit-proposal --proposal="path/to/proposal.json" --from mykey
$ %s tx gov submit-proposal path/to/msgs.json 10stake --from mykey
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ %s tx gov submit-proposal path/to/msgs.json 10stake --from mykey
$ %s tx gov submit-proposal path/to/msgs.json --from mykey --deposit 10stake

x/gov/types/proposal.go

return msgs, nil
}

// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
func (p Proposal) UnpackInterfaces(unpacker types.AnyUnpacker) error {
var content Content
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be updated to unpack messages, ditto for MsgSubmitProposal

@cmwaters
Copy link
Contributor Author

Thanks @cmwaters! I left a few small comments.
I think it would make sense indeed to split it up into smaller PRs: one for each new Msg and one for the gov changes, as suggested by @aaronc
This will also make it easier to make sure that new code has good test coverage.

Okay yeah I will try split it up 👍

@hxrts hxrts mentioned this pull request Jul 23, 2021
Closed
@cmwaters
Copy link
Contributor Author

Closing in favour of #9809

@cmwaters cmwaters closed this Jul 28, 2021
@alexanderbez alexanderbez deleted the callum/gov branch April 22, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronc aaronc aaronc left review comments

@blushi blushi blushi left review comments

Assignees
No one assigned
Labels
C:CLI C:Simulations C:x/auth C:x/bank C:x/capability C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/gov C:x/params
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minimal x/gov & x/group Alignment
4 participants
@cmwaters @orijbot @aaronc @blushi