Multiple Accounts for ModuleAccount

[colin-axner]

Summary

Creation of a ModuleAccount allows for no distinction between fungible coins. Some mechanism for separating balances could be very useful.

Problem Definition

I will outline this issue specifically in relation to the coinswap #4644 implementation.

The coinswap module needs to use a ModuleAccount to hold liquidity for the exchange. However each trading pair's liquidity needs to be differentiated from the other trading pairs. ModuleAccount does not separate fungible coins so an external mapping using store would need to be added to track the balance of each trading pair. This is the less ideal fix since it causes extra implementation on the application developers side that could increase the likely hood of supply tracking bugs.

Proposal

One solution is to add subaccounts. This would allow for an address to maintain multiple accounts in order to isolate balances. For example, a centralized exchange could have one address and subaccounts for each user. I think this is a very interesting solution and would allow for a lot more features when paired with permissions. The downside is it may take a while to implement and would probably restructure the code a bit.

Another solution is to allow for dynamic creation of module accounts rather than just at the initialization of the supply keeper. I think this is a far less ideal solution and probably undermines the original intent of the supply module.

Thoughts? Would love feedback on this. Please let me know if I should move forward with adding an extra mapping to track balances if no other solution is suitable

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[colin-axner]

@alexanderbez @fedekunze @alessio What do you guys think I should do to get around this?

[alexanderbez]

Thanks for raising these concerns @colin-axner as they're definitely valid. We should certainly go forward with an ideal solution and not a short-term temporary one just to wrap up coinswap. That being said, I think it's OK if the proposed solution involves further restructuring and design/implementation.

I like the idea of sub-module-accounts -- it seems to lend itself nicely to the existing implementation and use of permissions. Also, we can even hook in a governance proposal handler to provide the ability to permission/add new sub-accounts (fungible pairs)!

Although wrt to your centralized exchange example, I don't see why we'd need to use a module account with sub-accounts for each user -- seems overkill. For something like Coinswap, it certainly makes sense to have a single parent/master module account with sub-accounts for each liquid fungible pair.

IMHO I think addressing #4652 first is of higher priority (and much easier). In addition, it'll probably lend itself well to whatever the agreed upon proposal is here.

[colin-axner]

Proposal

Introduce two new types into x/auth
Introduce two new types into x/supply

• MultiModuleAccount
• SubAccount

ModuleMultiAccount maintains an array of SubAccounts

• Implements Account interface
• It returns an error in SetCoins
• Has regular address, no pubkey
  - Initial work is aimed for use by ModuleAccounts, but can be upgraded
  in the future to support user owned MultiAccount (add pubkey usage, etc)
• GetCoins returns sum of all coins owned by subaccounts
• Contains permissions
• Can only append accounts. To invalidate an account, add some func such as SetAccountDisabled which would only allow for withdraws but not increasing account balance

SubAccount

• Implements Account interface
• Address is MultiAccount address with SubAccount index appended
• Contains permissions

Other Changes

• Update BankKeepers SetCoins to return error instead of calling panic on the account's SetCoins error.

[colin-axner]

Alternatively these can go in x/supply though it may be useful to have a SubAccount owned by a user with permissions. Such as DailyAllowance where a user in a group can spend a certain amount daily.

[AdityaSripal]

So my understanding is that each SubAccount has permissions: append(parent.Permissions, self.permissions)?

Also, if this gets implemented in x/auth then auth will need some notion of permission array.

[alessio]

Are sub-account permission somehow restricted by the parent account's permissions set?
I think we need to separate balances in many use cases - I had already stumbled across this particular need long time ago.
I'm concerned about carrying out all these radical changes to such a simple concept like Account is.

[colin-axner]

I think sub-account and parent permissions should not inherit permissions in either direction, i.e. parent and sub-account permissions should be explicitly set for both, they may overlap but I don't think there should be an enforced restriction such as sub-account inheriting parent permissions or parents having all sub-account permissions.

I agree @alessio, this is definitely a discussion that should be well thought out

[fedekunze]

Questions (this will have to be spec'ed out before implementation):

• Do MultiAccounts have a restriction on the number of SubAccounts that they can create
• Can a MultiAccount with no subaccounts be created ?
• With permissions for the multi acc you mean that every subacc contains a set of permission or the multiacc also contains additional permissions?

Address is MultiAccount address with SubAccount index appended

• I assume that the address is generated from the hash of a root string ? How will that impact when you extend the functionality to not ModuleAccounts? I'd suggest implementing this at once instead of extending it later on.
• Will the total coins of the MultiAccount be the sum of the individual sub accs (passively tracking the coins)? If so, should we have invariant checks as well?

[colin-axner]

1. Probably add the ability to set a restriction upon initialization, but we probably also want the ability to allow for unbounded subaccs (in coinswap we don't want to limit number of trading pairs, except maybe through governance?)
   Since SubAccounts are managed by a module, we will let the application developer enforce restrictions and not add any restrictions into the notion of a MultiModuleAccount

2. Yes, this is what a MultiAcc constructor will return.

3. subacc and multiacc each contain set of permissions. subacc could have perm A,B and mulitacc could have B,C or maybe just C. All perms possible perms for multiacc and subacc would need to be added to PermissionsForAddress mapping that is set for the multi acc.

4. This relates to how we want to manage pubkeys for subaccs. Will look through current state of subkeys and see if I can circle back on this question as it is subkeys is probably related to how we want to handle this.
   SubAccounts probably should never be used for non ModuleAccounts. Instead PermissionedAccounts are introduced later

5. Yes, invariant can loop over all subaccs and see if sum == multiacc.GetCoins()

[colin-axner]

Maybe it makes more sense to just give MultiAcc full permissions, since it would be the account that creates the subaccs? Then the subacc perms just need to be a subset of parent permissions.

Will try to open ADR/Spec for this soon, so comments can be moved to a draft pr.

[alexanderbez]

@colin-axner I think a big point of confusion will come from implementation of what is outlined in this issue and that of the subkey implement bharvest and regen are working on. Would you mind updating the spec/details here please?

Namely, I'd want to be assured that x/auth wont become terribly confusing to grok when understanding both implementations.

[colin-axner]

@alexanderbez sorry for the confusion. Updated this issue and #4732 . Proposed changes should not touch auth at all. All changes would be limited to within the supply module

[alexanderbez]

Perfect! I reviewed it; exactly what I had in mind 🌮

[iramiller]

This proposal would be perfect for a use case I have that is currently solved with a custom account type handled by a module. That approach isn’t as nice as these sub account though with respect to permissions, iterators/grouping, etc. Hopefully this approach can also allow the subaccounts to be selectively blacklisted (see #5371).

[alexanderbez]

We can slate this for 0.41

[aaronc]

Module sub-account addresses are being addressed in ADR 028 and their usage in ADR 033.

Some of this will be tackled as part of the larger refactor outlined in #7091 and #7093 .

[colin-axner]

Once this feature is completed, IBC transfer module will likely need to integrate these changes. It will help us fully solve #7737

[aaronc]

How soon is this needed @colin-axner ? For now can you just generate addresses based on ADR 028? I have a proof of concept of #7459 but it will take a bit of time to get it fully ready.

[colin-axner]

@aaronc Sorry, didn't mean to imply we needed this immediately. Just once this feature is included, we will want to migrate to it. Just read ADR 028, that will definitely work for now, thanks!

[robert-zaremba]

This has been implemented in ADR-28.
Look at the address.Module function.

[georgelombardi97]

Yeap, and an example of its usage is in osmosis pools.

Each pool should keep funds in a separate account/address. The address is created here:
https://github.com/osmosis-labs/osmosis/blob/c7f0a52af4540bf4c77d17953341d0a7a752cb00/x/gamm/types/pool.go#L138

It also creates a module account for the pool:
https://github.com/osmosis-labs/osmosis/blob/f8eceacc9cb762a147a80e0e0715cae57d4757bd/x/gamm/keeper/pool_service.go#L142

[georgelombardi97]

Wondering why they create an account for it expelicitly. The bank module creates the account if it doesn't exist.

[robert-zaremba]

@georgelombardi97 bank doesn't create new addresses.

[georgelombardi97]

Yea. My question was why explicitly create an account while the bank module creates the account for the given addresses. (not to generate new address. yea bank doesnt do that, it just gets addr as input, but nevertheless creates the account for the address)

cosmos-sdk/x/bank/keeper/send.go

Line 210 in f08ba9e

k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr))