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

[ADR-3] - Module SubAccounts #4732

Closed
wants to merge 22 commits into from
Closed
Changes from 15 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions docs/architecture/adr-003-subaccounts.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
# ADR 3: Module SubAccounts

## Changelog

## Context

Currently `ModuleAccount`s must be declared upon Supply Keeper initialization. In addition to this they don't allow for separation of fungible coins within an account.

We want to support the ability to define and manage sub-module-accounts.

## Decision

We will use the type `ModuleMultiAccount` to manage sub-accounts of type `ModuleAccount`.
A `ModuleMultiAccount` may have zero or more `ModuleAccount`s.
Each sub-account will utilize the existing permissioned properties of `ModuleAccount`.
`ModuleMultiAccount` and `ModuleAccount` have no pubkeys.
There is no limit on the number of `ModuleAccount`s that a `ModuleMultiAccount` can have.
A `ModuleMultiAccount` has no permissions since it cannot hold any coins.
Its constructor returns a `ModuleMultiAccount` with no `ModuleAccount`s.
A sub-account cannot be removed from a `MultiModuleAccount`.
The account number assigned to sub-accounts will begin at 0 and be monotonically auto incrementing.
Copy link
Contributor

Choose a reason for hiding this comment

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

An idea: each module subaccount could be indexed by a string (aka submodule account name). But thinking out loud maybe that's kinda irrelevant as we could just have named byte variables (aka const FooSubAccount = 0x00)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this thought process led us to the proposed changes posted below


### Implementation Changes

Introduce a new type into `x/supply`:

* `ModuleMultiAccount`
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

```go
// Implements the Account interface.
// SetCoins will return an error to prevent ModuleMultiAccount address from having a balance.
// ModuleAccounts are appended to the SubAccounts array.
// Passively tracks the sum of all ModuleAccount balances.
type ModuleMultiAccount struct {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
SubAccounts []ModuleAccount
Coins sdk.Coins // passively track all sub account balances

CreateSubAccount(name string, permissions ...string) int // returns account number of sub-account
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 probably return an error under the scenario that the subaccount is attempted to be created with non-inherited permissions of the ModuleMultiAccount

GetSubAccount(subAccNumber int64) ModuleAccount
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
```

The `ModuleAccount` implementation will remain unchanged, but we will add the following constructor function:
```go
// NewEmptyModuleSubAccount creates a sub-account ModuleAccount which has an address created from
// the hash of the module's name with the sub-account number appended.
func NewEmptyModuleSubAccount(name string, subAccNumber uint64, permissions ...string) ModuleAccount {
bz := make([]byte, 8)
binary.LittleEndian.PutUint64(bz, subAccNumber)
moduleAddress := append(NewModuleAddress(name), bz...)
baseAcc := authtypes.NewBaseAccountWithAddress(moduleAddress)

if err := validatePermissions(permissions...); err != nil {
panic(err)
}

return &ModuleAccount{
BaseAccount: &baseAcc,
Name: name,
Permissions: permissions,
}
}
```

**Permissions**:

A `ModuleMultiAccount` has no permissions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused can you add a bit more clarity? I thought that ModuleMultiAccounts did have the permissions of the module? Maybe this is just semantics but a more thorough explanation would be appreciated

Copy link
Contributor

Choose a reason for hiding this comment

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

Also these semantics seem to contradict with your later point ModuleMultiAccount can distribute permissions to sub-accounts.


Since `ModuleAccount`s that are sub-accounts have the same name as its parent `ModuleMultiAccount`, a sub-account should only be granted a subset of the permissions registered with the Supply Keeper under its name.

**Other changes**

We will add an invariant check for the `ModuleMultiAccount` `GetCoins()` function, which will iterate over all SubAccounts to see if the sum of the `ModuleAccount` balances equals the passive tracking which is returned in `GetCoins()`
Copy link
Contributor

Choose a reason for hiding this comment

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

++


Bank Keepers `SetCoins()` function will be updated to return an error instead of calling panic on the account's SetCoins error.

## Status

Accepted

## Consequences

### Positive

* ModuleMultiAccount can separate fungible coins.
* ModuleMultiAccount can dynamically add accounts.
* ModuleMultiAccount can distribute permissions to sub-accounts.

### Negative

* sub-accounts cannot be removed from `ModuleMultiAccount`
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Neutral

* Use `ModuleAccount` type as a sub-account for `ModuleMultiAccount`
* Adds a new Account type

## References
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

Specs: [ModuleAccount](https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/supply/01_concepts.md#module-accounts)

Issues: [4657](https://github.com/cosmos/cosmos-sdk/issues/4657)