[ADR-3] - Module SubAccounts

[colin-axner]

ADR for #4657

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

• Wrote tests

• Updated relevant documentation (docs/)

• Added a relevant changelog entry: clog add [section] [stanza] [message]

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #4732 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4732      +/-   ##
==========================================
- Coverage   53.73%   53.72%   -0.02%     
==========================================
  Files         273      273              
  Lines       17279    17279              
==========================================
- Hits         9285     9283       -2     
- Misses       7310     7312       +2     
  Partials      684      684

[colin-axner]

After looking over current work of subkeys and discussing with @AdityaSripal. It seems SubAccounts for regular accounts are only useful with permissions, but for ModuleAccount they are useful for separation of fungible coins and permissions.

[AdityaSripal]

Yea so MultiAccount if it is only enabling isolation of account balances is of no use to users to own (since you may as well just own multiple BaseAccounts). But using MultiAccount to delegate specific permissions to their SubAccounts would be useful both for modules and for users.

Thus, I think permissions should not be restricted to just ModuleAccounts but should be something that all accounts can be defined.

Here is a very rough idea of what I'm thinking:

Account interface {
     Permissioned()   bool // if false, then treat account as if it has all permissions
     Permissions()     map[string]{map[string]{interface{}}} // very ugly type declaration but should have some way of defining parameters for each permission.
}

Router changes:

for _, acc := range msg.GetSigners {
    if acc.Permissioned() && "module_name" not int acc.Permissions() {
        return err
    }
}

If any keeper wanted to check against specific permissions they could do it like so:

if account.Permissioned() {
    permissions := account.Permissions()["module_name"]
    // validate that user can perform this specific action given the permission parameters
    // if not return error
}

The implementation above is pretty ugly but its a quick sketch of how something like this could be possible. Probably the more important thing to consider is whether we want users to be able to create permissioned accounts or not.

I think it makes sense for users to create restrictions on their own accounts for security reasons. So that I can travel with the privatekeys for accounts that have restrictive permissions while holding a larger permissionless account safe in a vault or something.

I bring this up because now would be a good time to finalize a design on this so a multi-account feature could be created with that functionality in mind

[fedekunze]

I think the idea of permissioned accounts makes total sense. I think it's out of the scope of this ADR tho

[AdityaSripal]

Agreed, MultiAccount should probably just be a nice way to isolate account balances

Have to think more about how permissioned accounts can dovetail with the current work on subkeys

[fedekunze]

LGTM in general, only minor comments. I'd be great if you could write something about 1. 3. and 4. from your comments here #4657 (comment) on this ADR

[alexanderbez]

ACK

significant changes to adr

[AdityaSripal]

I think this should be TreeCoins/TotalCoins or something similar and passively track sum of child + self balances

[colin-axner]

I am in favor of only passively tracking child balances and just calling GetChildCoins + GetCoins to get total. What do other people think?

[AdityaSripal]

Is it possible to add/delete attributes. Should parents be able to modify attributes of their children?

[colin-axner]

Discussion for this can be seen here #4762. Outside of the scope of this ADR in my opinion.

[rigelrozanski]

Re: The link immediately above here. Turns out it seems as though subaccounts is high level abstraction of a generalized staking mechanism (meaning we shouldn't implement the staking mechanism) - very cool - worth thinking about this ADR with that potential use case in mind (CC @colin-axner @AdityaSripal )

[ethanfrey]

I saw this with a request to review in slack.

I don't really understand the motivation, making it very hard to evaluate. Can you please add a short section to the ADR explaining what features this provides to users and app developers. Context just says modules are limited to one account. Please show me an example or two where multiple accounts would be important.

Happy to review when I understand this

[colin-axner]

@ethanfrey I updated the context with a short example of how this feature will be used. Let me know if you still have any questions. Looking forward to your feedback!

[ethanfrey]

Thank you @colin-axner I know understand the why.

A module contains multiple independent objects, each of which needs their own account. Like multiple orderbooks in a uniswap module, or multiple validators in the distribution module (rather than store all tokens in one global), or even the multiple group accounts proposed in Regen's Key Managment proposal.

However, I am now confused by the difference between Accounts and ModuleAccounts.

In @aaronc 's proposal for key management, there are groups which have accounts (a mix between multisig and small governance votes). These accounts can hold tokens, stake, make proposals, etc. In the parlance of this PR, they would be sub-module accounts. In the parlance of his proposal, they simply have sdk.AccAddress, and he discussed them as normal accounts.

Why are there two kinds of accounts? Why not unify this all? I have done prior art on such a design and it seemed much simpler... any object can have an Address. Anything that has an Address can authorize a message.

That means, the auth.BaseAccount and auth.Account types contain both generic info like GetAddress() sdk.AccAddress, token-specific actions that belong to any account (also modules) like GetCoins() sdk.Coins and SetCoins(sdk.Coins) error, and public key signature specific actions like GetPubKey() crypto.PubKey and GetAccountNumber() uint64.

Is there a proper ADR explaining the general Account design in the SDK, before making these changes? If this is planned for v0.37 when there can be breaking API changes, I suggest revisting the entire Account design and simplifying it.

I have it on good authority from many experienced go dev's that when interfaces have more than a couple methods it is a smell of bad design. I don't agree fully with the max 2 method, but I do see this as multiple, independent interfaces thrown together, which can be solved by designing an orthogonal set of interfaces that can be combined to support multiple use cases, much as all particles are just comprised of different combinations of fermions.

[alexanderbez]

@ethanfrey I believe it's important to make the distinction that module(-sub)-accounts ≠ sub-keys (which I assume you're referring to as @aaronc's proposal).

afaiu, ModuleAccount are internal to the protocol. They represent what we typically used to call "pools" internally (e.g. burned pool, unbonded pool, distribution rewards pool, etc...). A ModuleAccount is not user managed. There is no entity that controls them or uses them. You can't send funds to them and you can withdraw funds from them (for the most part). It is to be an abstraction that most people won't even have to bother with. In fact, there is even a proposal by @AdityaSripal to even store ModuleAccounts in a separate store!

[ethanfrey]

Thanks for that explanation @alexanderbez

Is there a good write-up of the account / module account design? I feel like I ask basic questions on design, yet cannot find good docs. Not just the how, which I can figure out much of by reading code, but the why and the best practices.

specs like https://github.com/cosmos/cosmos-sdk/tree/master/docs/spec/auth do have useful info, but more on how the implementation works (but definitely nicer than the code)

Many other places like https://github.com/cosmos/cosmos-sdk/tree/master/docs/interfaces are missing and https://github.com/cosmos/cosmos-sdk/blob/master/docs/concepts/tx-lifecycle.md is a merge conflict....

Maybe what I want is in docs somewhere, but there is so much half-finished, it is hard to find the important pieces. (I guess this is what https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-002-docs-structure.md attempts to address 🙈 )

[colin-axner]

@ethanfrey to the best of my knowledge this is the only docs on ModuleAccounts

I agree with you though, there doesn't seem to be a clear reason why a ModuleAccount can't just be a normal account. This ADR was mostly aimed at working around the current implementation to support more flexibility for ModuleAccounts, but it is starting to seem like it will be retired in favor of reworking the current design in 0.37. @sunnya97 is going to be working on a proposal for this, so maybe if you have any ideas you could direct them there.

I think future usage of ADR's will solve some of the issues regarding understanding previous design decisions.

Many other places like https://github.com/cosmos/cosmos-sdk/tree/master/docs/interfaces are missing and https://github.com/cosmos/cosmos-sdk/blob/master/docs/concepts/tx-lifecycle.md is a merge conflict....

Thanks for pointing this out, it has been fixed now..

[colin-axner]

blocking until more discussion is had and consensus is reached over what direction to take with ModuleAccounts

[jackzampolin]

Is there any update on this work?

[colin-axner]

Is there any update on this work?

not on my end. I was under the impression @sunnya97 was working on a proposal that would solve this ADR's issue and therefore make it irrelevant

[jackzampolin]

@sunnya97 any update here?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.