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

Minimal x/gov & x/group Alignment #9438

Closed
aaronc opened this issue Jun 1, 2021 · 32 comments
Closed

Minimal x/gov & x/group Alignment #9438

aaronc opened this issue Jun 1, 2021 · 32 comments
Assignees
Labels
C:x/gov C:x/group T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: never don't do this! T: State Machine Breaking State machine breaking changes (impacts consensus).

Comments

@aaronc
Copy link
Member

aaronc commented Jun 1, 2021

There is a longer conversation happening in #9066 around integration and de-duplication of x/gov and x/group, however, in order to bring some immediate benefit from x/authz and x/group to on-chain governance, I propose the following "minimal" changes to x/gov:

  • add a root account that x/gov controls
  • migrate gov proposal handlers to an array of sdk.Msgs which are only executable by root in their respective modules (i.e. x/param, x/upgrade, etc.)

This will allow x/gov to use x/authz to delegate permissions from root to other "working groups" managed by x/group.

/cc @cmwaters @hxrts @blushi

@aaronc aaronc added this to the Feature Backlog milestone Jun 1, 2021
@aaronc aaronc added Status: Backlog S:needs architecture review To discuss on the next architecture review call to come to alignment C:x/gov C:x/group labels Jun 1, 2021
@aaronc
Copy link
Member Author

aaronc commented Jun 1, 2021

Tagging as "Needs Architecture Review" to remind me to present this at this Friday's architecture call.

@aaronc aaronc added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). Type: Feature T: Proto Breaking Protobuf breaking changes: never don't do this! T: State Machine Breaking State machine breaking changes (impacts consensus). labels Jun 1, 2021
@aaronc aaronc removed the S:needs architecture review To discuss on the next architecture review call to come to alignment label Jun 18, 2021
@aaronc
Copy link
Member Author

aaronc commented Jun 18, 2021

@cmwaters green light to move forward on this. We will need to write an ADR to document the changes. Let's chat to coordinate the on that.

@aaronc
Copy link
Member Author

aaronc commented Jul 16, 2021

In the use cases in #9708 which I believe are mostly covered by this design, the idea that an admin might want to change a test account balance is proposed. I wonder, should we allow root to execute any arbitrary sdk.Msg regardless of the signer, and in turn delegate this through x/authz? I think maybe yes since that's what governance should be able to do, right? It could on the other hand increase attack surface area.

I realize that changing an account balance still wouldn't quite be possible because coins would need to be minted and currently there is no way to do this with sdk.Msgs, we just have MsgSend so we could move coins from one place to another.

If we were going to implement this, my current thought is that maybe x/authz allows the root account to execute anything at all using MsgExec.

Any thoughts @cmwaters @hxrts ?

@hxrts
Copy link
Contributor

hxrts commented Jul 16, 2021

This is definitely a cool approach to achieving the same result in the admin module but I'm wondering how to achieve this while maintaining security best practices. Is there a way to prevent the hub from using this functionality or limit any risk of exploit? Ideally authz would let us leverage our capability model to only allow the most granular permission required.

Until recently I didn't think the admin module would ever run on a production chain, so the security model didn't matter as much. On the last interchain staking call @okwme saw some utility for an admin module running on a baby chain where an interchain account from the hub's gov module could modify baby chain gov params, thereby allowing the baby chain to operate without its own token. I don't love this approach tbh, kind of a backdoor that breaks the sdk's security model.

@aaronc
Copy link
Member Author

aaronc commented Jul 16, 2021

The x/authz approach allows for configurable granularity and I think that's definitely preferable for production use cases.

@cmwaters
Copy link
Contributor

Currently, I've swapped out the gov router for baseapp.MsgServiceRouter and am just passing the array of messages via this handler when the proposal passes. I'm guessing that to reroute these messages via x/authz, I wrap the messages in a MsgExec with the grantee as the root account right? But where do the modules like x/params and x/distribution grant authorization to the gov root account in the first place. Is it possible to set up authorizations when initializing.

Also, the way I understand authz is that authorizations are not transitive i.e. if x/params grants gov's root account the capability to change params, the root account can't then grant another account to be able to change params. Or is something like this possible. Can gov's root account pass on execution of certain messages to elected groups?

On a different note, I'm not sure what the security model is with MsgServiceRouter. It seems like any module is able to send messages to any other module.

@aaronc
Copy link
Member Author

aaronc commented Jul 19, 2021

Also, the way I understand authz is that authorizations are not transitive i.e. if x/params grants gov's root account the capability to change params, the root account can't then grant another account to be able to change params. Or is something like this possible.

I think we covered most of this in our sync, but just for others in this thread, yes root could use x/authz to authorize two different accounts the ability to submit MsgSetParam (possibly with slightly different Authorization permissions - i.e. one account could modify gov params and another could modify staking for instance).

@okwme
Copy link
Contributor

okwme commented Jul 20, 2021

I wanted to follow up with @cmwaters 's question about permissions around other module accounts like distribution. The feature I'm mostly thinking about was outlined in this issue #9689. Essentially for Interchain Accounts to be leveraged by the governance module two things would need to happen: 1) the gov module needs to create outgoing Interchain Account messages (I expected this to be a custom proposal type but could also be taken care of by this work) and 2) the ability for the community pool to execute IBC transfers so that funds can be moved across chains into the interchain account that is controlled by the origin chain. Both of these would be possible with this work but only if the community pool was held by the root account. Currently the community pool funds are held by the distribution module account. Is there a plan in place to ensure that the execution of messages on behalf of this root account by Groups subcommittees can also access the community pool?

Also I'd just like to confirm whether it's possible to create a group that completely replicates (and stays in sync) with the delegated voting capabilities of the gov module?

@okwme
Copy link
Contributor

okwme commented Jul 20, 2021

also, is the upgrade proposal going to become a standard message type as part of this?

@aaronc
Copy link
Member Author

aaronc commented Jul 20, 2021

Is there a plan in place to ensure that the execution of messages on behalf of this root account by Groups subcommittees can also access the community pool?

I think the main thing is to allow x/gov to control multiple addresses, which is straightforward. It's either a list of allowed addresses or x/gov can send any message at all. Maybe this is a configurable param that provides some control over x/gov's authority. x/gov could theoretically execute any message at all and make an authorization grant on behalf of any address.

Also I'd just like to confirm whether it's possible to create a group that completely replicates (and stays in sync) with the delegated voting capabilities of the gov module?

I'm not understanding. Can you explain more?

also, is the upgrade proposal going to become a standard message type as part of this?

yes

@okwme
Copy link
Contributor

okwme commented Jul 21, 2021

ok cool, I discussed this a bit more with @cmwaters today too and i'm getting a better picture of how this covers all the bases (but after writing through the story below I'm afraid there may be issues).


I'm not understanding. Can you explain more?

I had it backwards, please ignore that question.


I'm very wary of the gov module being able to out of the gate execute messages from any account, but I like the idea of a configured list of accounts that could be set at genesis (or potentially voted in). I can see how authz is a solution for this.


My assumption is that even if prop 52 fails there will be more and more opportunities for the community pool to participate in interchain defi and making this non-custodial will become important as the stakes get higher. For the user story of replacing prop 52 with a non-custodial version it might look like:

  • Distribution module contains authz messages in genesis that allow gov root account to execute MsgTransfer on its behalf
    • aside: would allowing gov root to use MsgSend remove the need for the spend proposal type?
  • Gov proposal could come in with the list of messages:
    • MsgRegisterAccount
      • register the Interchain Account on the other chain using params channel and port for Osmosis
    • MsgTransfer
      • fund the account on the other chain with ATOMs from the distribution module account using destination interchain account address and channel and port for Osmosis
    • MsgRunTX
      • execute the Msg that sells 50% of the ATOMs on the other chain for OSMO
    • MsgRunTX
      • execute the Msg that puts the remaining ATOMs and OSMO into the LP

Actually I think this won't work. First off, IBC messages would need to wait until the entire sequence is complete before executing follow up messages. Secondly, it actually requires the results of some initial transactions to be used as parameters of the follow up transactions. I'd imagine that would get really messy to try to do programatically @seantking can you confirm whether the MsgRegisterAccount results in the creation of the interchain account address or whether there's any chance of pre-calculating it? Also the results of the ATOM sell would be different depending on the delay of execution so it might not be possible to pre-calculate it. You might be able to add liquidity with just one token as a parameter and the tx will fail if you don't have enough of the pair token.

What are the other use stories that these changes are prioritizing? I can think of a couple now but I'd be curious as to the others i'm missing:

  • enabling cross chain DAO activities
    • Interchain accounts doing defi on other chains
    • Interchain accounts doing governance on other chains (assuming community pool starts owning other tokens)
  • enabling local chain DAO activities
    • gov root account can do the same things as prop 52 except targeting the gravity dex instead
  • updating more than one IBC client at a time
  • convert current governance action committees from multisig holders to groups module participants
    • essentially the same features as a multisig except without as much signing coordination (in this scenario would the composition of the group be re-configurable after creation?)
  • an upgrade proposal can include a spend proposal to go to the team who is doing the work

@cmwaters
Copy link
Contributor

Some cool ideas.

can you confirm whether the MsgRegisterAccount results in the creation of the interchain account address or whether there's any chance of pre-calculating it

I remember Sean telling me that anyone can create an interchain account but only gov can register it. So I would assume it would be known in advance.

First off, IBC messages would need to wait until the entire sequence is complete before executing follow up messages.

Are you not able to bundle a set of transactions into a single IBC packet and make it atomic since it's all going to the same chain.

aside: would allowing gov root to use MsgSend remove the need for the spend proposal type?

basically. Although the distribution module has this blocklist that prevents money going to certain accounts

@aaronc
Copy link
Member Author

aaronc commented Jul 21, 2021

If the interchain account address were deterministic, that could work but I have no idea. An IBC bundle packet also sounds like it could help.

It almost sounds like this use case needs some lightweight scripting support where you can get the return value of MsgRegisterAccount and use that as a var to fill in the address in other messages. I wonder how often those types of scripts would be useful. I have thought of ways to have lightweight non-VM based scripts (for instance in #9668). But that sounds like a whole other conversation.

It may be though that you need to split it up into two proposals - one to create the account and the other to perform actions. Maybe the second at least can be atomic.

convert current governance action committees from multisig holders to groups module participants

we have explored ways to convert existing accounts to group accounts via ADR 032 (account rekeying) and it's in the roadmap, just not right away

@tac0turtle tac0turtle moved this to In Progress in Cosmos SDK: Gov & Group WG Jan 20, 2022
mergify bot pushed a commit that referenced this issue Jan 21, 2022
## Description

Ref: #9438

This PR performs the major work of swapping out the v1beta1 msg server and query server for the new one which can process a proposal as an array of messages. This PR still retains the legacy servers which simply wrap around the new ones, providing the same interface as before.

In order to keep backwards compatibility, a new msg, `MsgExecLegacyContent` has been created which allows `Content` to become a `Msg` type and still be used as part of the new implementation. 


---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
@tac0turtle
Copy link
Member

@blushi could this be closed?

@amaury1093
Copy link
Contributor

We just finished audit and QA tests on both gov and group, so let's close this! 🎉

@amaury1093 amaury1093 moved this to 👏 Done in Cosmos-SDK Jun 2, 2022
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this issue May 22, 2023
## Description

Ref: cosmos#9438

This PR performs the major work of swapping out the v1beta1 msg server and query server for the new one which can process a proposal as an array of messages. This PR still retains the legacy servers which simply wrap around the new ones, providing the same interface as before.

In order to keep backwards compatibility, a new msg, `MsgExecLegacyContent` has been created which allows `Content` to become a `Msg` type and still be used as part of the new implementation. 


---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gov C:x/group T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: never don't do this! T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
Archived in project
9 participants