From ca2ca2224131b8d0d36a8fadd49cd18e76b393f6 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 14 Jan 2020 16:51:20 +0100 Subject: [PATCH] Merge PR #5494: ADR 004: Split Denomination Keys --- docs/architecture/README.md | 1 + .../adr-004-split-denomination-keys.md | 73 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 docs/architecture/adr-004-split-denomination-keys.md diff --git a/docs/architecture/README.md b/docs/architecture/README.md index be9cf3564..e882cb90c 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -33,6 +33,7 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 002: SDK Documentation Structure](./adr-002-docs-structure.md) - [ADR 003: Dynamic Capability Store](./adr-003-dynamic-capability-store.md) +- [ADR 004: Split Denomination Keys](./adr-004-split-denomination-keys.md) - [ADR 006: Secret Store Replacement](./adr-006-secret-store-replacement.md) - [ADR 009: Evidence Module](./adr-009-evidence-module.md) - [ADR 010: Modular AnteHandler](./adr-010-modular-antehandler.md) diff --git a/docs/architecture/adr-004-split-denomination-keys.md b/docs/architecture/adr-004-split-denomination-keys.md new file mode 100644 index 000000000..5e91f7da3 --- /dev/null +++ b/docs/architecture/adr-004-split-denomination-keys.md @@ -0,0 +1,73 @@ +# ADR 004: Split Denomination Keys + +## Changelog + +- 2020-01-08: Initial version +- 2020-01-09: Alterations to handle vesting accounts +- 2020-01-14: Updates from review feedback + +## Context + +With permissionless IBC, anyone will be able to send arbitrary denominations to any other account. Currently, all non-zero balances are stored along with the account in an `sdk.Coins` struct, which creates a potential denial-of-service concern, as too many denominations will become expensive to load & store each time the account is modified. See issues [5467](https://github.com/cosmos/cosmos-sdk/issues/5467) and [4982](https://github.com/cosmos/cosmos-sdk/issues/4982) for additional context. + +Simply rejecting incoming deposits after a denomination count limit doesn't work, since it opens up a griefing vector: someone could send a user lots of nonsensical coins over IBC, and then prevent the user from receiving real denominations (such as staking rewards). + +## Decision + +Balances shall be stored per-account & per-denomination under a denomination- and account-unique key, thus enabling O(1) read & write access to the balance of a particular account in a particular denomination. + +### Account interface (x/auth) + +`GetCoins()` and `SetCoins()` will be removed from the account interface, since coin balances will now be stored in & managed by the bank module. + +`SpendableCoinsVestingAccount()` and `TrackDelegation()` will be altered to take a bank keeper and a denomination as two additional arguments, which will be used to lookup the balances from the base account as necessary. + +Vesting accounts will continue to store original vesting, delegated free, and delegated vesting coins (which is safe since these cannot contain arbitrary denominations). + +### Bank keeper (x/bank) + +`GetBalance(addr AccAddress, denom string) sdk.Coin` and `SetBalance(addr AccAddress, coin sdk.Coin)` methods will be added to the bank keeper to retrieve & set balances, respectively. + +Balances will be stored first by the address, then by the denomination (the reverse is also possible, but retrieval of all balances for a single account is presumed to be more frequent): + +```golang +func BalanceKey(addr sdk.AccAddress, denom string) []byte { + return append(append(BalanceKeyPrefix, addr.Bytes()...), []byte(denom)...) +} +``` + +`DelegateCoins()` and `UndelegateCoins()` will be altered to take a single `sdk.Coin` (one denomination & amount) instead of `sdk.Coins`, since they should only operate on one denomination. They will read balances directly instead of calling `GetCoins()` (which no longer exists). + +`SubtractCoins()` and `AddCoins()` will be altered to read & write the balances directly instead of calling `GetCoins()` / `SetCoins()` (which no longer exist). + +`trackDelegation()` and `trackUndelegation()` will be altered to read & write the balances directly instead of calling `GetCoins()` / `SetCoins()` (which no longer exist). + +External APIs will need to scan all balances under an account to retain backwards-compatibility - additional methods should be added to fetch a balance for a single denomination only. + +### Supply module + +The supply module, in order to implement the total supply invariant, will now need to scan all accounts & call `GetBalance` using the `x/bank` Keeper for the denomination in question, then sum the balances and check that they match the expected total supply. + +## Status + +Proposed. + +## Consequences + +### Positive + +- O(1) reads & writes of balances (with respect to the number of denominations for which an account has non-zero balances) + +### Negative + +- Slighly less efficient reads/writes when reading & writing all balances of a single account in a transaction. + +### Neutral + +None in particular. + +## References + +Ref https://github.com/cosmos/cosmos-sdk/issues/4982 +Ref https://github.com/cosmos/cosmos-sdk/issues/5467 +Ref https://github.com/cosmos/cosmos-sdk/issues/5492