R4R: Bank Denom Metadata Spec #2072

sunnya97 · 2018-08-17T02:05:17Z

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

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 · 2018-08-17T02:17:32Z

Codecov Report

Merging #2072 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2072   +/-   ##
========================================
  Coverage    64.86%   64.86%           
========================================
  Files          115      115           
  Lines         6863     6863           
========================================
  Hits          4452     4452           
  Misses        2127     2127           
  Partials       284      284

alexanderbez

Thanks @sunnya97 -- I left some general feedback.

alexanderbez · 2018-08-17T15:00:59Z

docs/spec/bank/keeper.md

+
+## Denom Metadata
+
+The BankKeeper contains a store that stores the metadata of different token denoms.  Denoms are referred to by their name, same as the `denom` field in sdk.Coin.  The different attributes of a denom are stored in the denom metadata store under the key `[denom name]:[attribute name]`.  The default attributes in the store are explained below.  However, this can be extended by the developer or through SoftwareUpgrade proposals.


I don't think we need to abbreviate denom/denoms in this spec for brevity, unless referring to code snippets.

We have a standard way of writing key formats in specs - see e.g. https://github.com/cosmos/cosmos-sdk/blob/develop/docs/spec/staking/state.md

alexanderbez · 2018-08-17T15:01:11Z

docs/spec/bank/keeper.md

@@ -0,0 +1,22 @@
+# Keeper


A bunch of sentences start with an extra space 👍

alexanderbez · 2018-08-17T15:03:06Z

docs/spec/bank/keeper.md

+
+## Denom Metadata
+
+The BankKeeper contains a store that stores the metadata of different token denoms.  Denoms are referred to by their name, same as the `denom` field in sdk.Coin.  The different attributes of a denom are stored in the denom metadata store under the key `[denom name]:[attribute name]`.  The default attributes in the store are explained below.  However, this can be extended by the developer or through SoftwareUpgrade proposals.


Do we have a convention in the rest of the SDK for composite keys using */*?

alexanderbez · 2018-08-17T15:07:46Z

docs/spec/bank/keeper.md

@@ -0,0 +1,22 @@
+# Keeper
+


I also think it would help to add an example here for a token with the default metadata.

cwgoes

See comments.

cwgoes · 2018-08-20T15:19:07Z

docs/spec/bank/keeper.md

+
+## Denom Metadata
+
+The BankKeeper contains a store that stores the metadata of different token denoms.  Denoms are referred to by their name, same as the `denom` field in sdk.Coin.  The different attributes of a denom are stored in the denom metadata store under the key `[denom name]:[attribute name]`.  The default attributes in the store are explained below.  However, this can be extended by the developer or through SoftwareUpgrade proposals.


We have a standard way of writing key formats in specs - see e.g. https://github.com/cosmos/cosmos-sdk/blob/develop/docs/spec/staking/state.md

cwgoes · 2018-08-20T15:19:43Z

docs/spec/bank/keeper.md

+
+## Denom Metadata
+
+The BankKeeper contains a store that stores the metadata of different token denoms.  Denoms are referred to by their name, same as the `denom` field in sdk.Coin.  The different attributes of a denom are stored in the denom metadata store under the key `[denom name]:[attribute name]`.  The default attributes in the store are explained below.  However, this can be extended by the developer or through SoftwareUpgrade proposals.


Anything can be extended through software upgrade proposals, not sure it's necessary to mention unless you had particular upgrades in mind.

cwgoes · 2018-08-20T15:22:00Z

docs/spec/bank/keeper.md

+
+`1 [Base Unit] = 10^(N) [Smallest Unit]`
+
+### TotalSupply `sdk.Integer`


Is this intended to be a duplicate of the data kept in the stake Pool structure (total supply, including unclaimed inflation) or a different number (circulating supply, in accounts but not bonded)?

In either case, it seems a little expensive to track this explicitly, do we really need to?

No, we stop tracking it in the Pool.

cwgoes · 2018-08-20T15:22:25Z

docs/spec/bank/keeper.md

+
+### Aliases `[]string`
+
+Aliases is an array of strings that are "alternative names" for a token. As an example, while the Ether's denom name might be `ether`, a possible alias could be `ETH`.  This field can be useful for UIs and clients.  It is intended that this field can be modified by a governance mechanism.


So this is read-only in the state machine? What's the advantage of doing this on-chain?

It can be changed by governance. Also, it provides a standard source of truth for wallets to use.

cwgoes · 2018-08-20T15:23:50Z

docs/spec/bank/keeper.md

+
+### Decimals `int8`
+
+- `Base Unit` = The common standard for the default "standard" size of a token. Examples: 1 Bitcoin or 1 Ether.


These are amounts, denominated in integers (of the smallest unit), right?

e.g. for Ether:

Base Unit: 1e18

Smallest Unit: 1

Or are you suggesting we store the names of the sizes?

ValarDragon · 2018-08-18T14:17:59Z

docs/spec/bank/keeper.md

+- `Base Unit` = The common standard for the default "standard" size of a token. Examples: 1 Bitcoin or 1 Ether.
+- `Smallest Unit` = The smallest possible denomination of a token. A fraction of the base unit.  Examples: 1 satoshi or 1 wei.
+
+All amounts throughout the SDK are denominated in the smallest unit of a token, so that all amounts can be expressed as integers.  However, UIs typically want to display token values in the base unit, so the Decimals metadata field standardizes the number of digits that come after the decimal place in the base unit.


Not entirely true. Inflation stores amounts as decimal not integer. Perhaps rephrase "all amounts" to "all account holdings"

ValarDragon · 2018-08-18T14:22:36Z

docs/spec/bank/keeper.md

+
+### Aliases `[]string`
+
+Aliases is an array of strings that are "alternative names" for a token. As an example, while the Ether's denom name might be `ether`, a possible alias could be `ETH`.  This field can be useful for UIs and clients.  It is intended that this field can be modified by a governance mechanism.


Should we standardize on having uis accept case insensitive aliases, and therefore two aliases shouldn't be equal in a case insensitive manner

ValarDragon · 2018-08-18T14:24:14Z

docs/spec/bank/keeper.md

+
+### TotalSupply `sdk.Integer`
+
+The TotalSupply of a denom is the total amount of a token that exists (known to the chain) across all accounts and modules.  It is denominated in the `smallest unit` of a denom.  It can be changed by the Keeper functions `MintCoins` and `BurnCoins`.  `AddCoins` and `SubtractCoins` are used when adding or subtracting coins for an account, but not removing them from total supply (for example, when moving the coins to the control of the staking module).


Are chains then expected to update this on every IBC tx? (I.e. is sending tokens from chain 1 to 2 a mint on chain 2 since they now know about this token for the first time)

cwgoes · 2018-08-24T17:26:43Z

Should #1277 be in scope for this PR?

rigelrozanski · 2018-09-11T17:39:59Z

Should be merged as WIP in filename

rigelrozanski

approved for merge as WIP (as per recent team discussions)

approved as WIP merge

docs/spec/bank/WIP_keeper.md

Bank Denom Metadata Spec

766738b

sunnya97 requested review from jaekwon, mossid, rigelrozanski and cwgoes August 17, 2018 02:05

sunnya97 requested a review from zramsay as a code owner August 17, 2018 02:05

sunnya97 added spec ready-for-review labels Aug 17, 2018

alexanderbez previously requested changes Aug 17, 2018

View reviewed changes

cwgoes previously requested changes Aug 20, 2018

View reviewed changes

ValarDragon reviewed Aug 20, 2018

View reviewed changes

ValarDragon mentioned this pull request Sep 10, 2018

Ticker for coins created in local testnet #2294

Closed

rigelrozanski self-assigned this Sep 11, 2018

moved to WIP filename

24491de

rigelrozanski reviewed Sep 25, 2018

View reviewed changes

rigelrozanski approved these changes Sep 25, 2018

View reviewed changes

ValarDragon reviewed Sep 25, 2018

View reviewed changes

docs/spec/bank/WIP_keeper.md Show resolved Hide resolved

val comment

d794af7

rigelrozanski merged commit a04d5cf into develop Sep 25, 2018

rigelrozanski deleted the sunny/bank-denoms-spec branch September 25, 2018 18:18

jackzampolin mentioned this pull request Oct 12, 2018

Write bank module specification #1277

Closed

alexanderbez mentioned this pull request Aug 15, 2019

ADR: Bank MetaData #4911

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Bank Denom Metadata Spec #2072

R4R: Bank Denom Metadata Spec #2072

sunnya97 commented Aug 17, 2018

codecov bot commented Aug 17, 2018

alexanderbez left a comment

alexanderbez Aug 17, 2018

cwgoes Aug 20, 2018

alexanderbez Aug 17, 2018

alexanderbez Aug 17, 2018

alexanderbez Aug 17, 2018

cwgoes left a comment

cwgoes Aug 20, 2018

cwgoes Aug 20, 2018

cwgoes Aug 20, 2018

sunnya97 Jan 2, 2019

cwgoes Aug 20, 2018

sunnya97 Aug 22, 2018

cwgoes Aug 20, 2018

ValarDragon Aug 18, 2018

ValarDragon Aug 18, 2018

ValarDragon Aug 18, 2018

sunnya97 Aug 22, 2018

cwgoes commented Aug 24, 2018

rigelrozanski commented Sep 11, 2018

rigelrozanski left a comment


		## Denom Metadata

		The BankKeeper contains a store that stores the metadata of different token denoms. Denoms are referred to by their name, same as the `denom` field in sdk.Coin. The different attributes of a denom are stored in the denom metadata store under the key `[denom name]:[attribute name]`. The default attributes in the store are explained below. However, this can be extended by the developer or through SoftwareUpgrade proposals.


		`1 [Base Unit] = 10^(N) [Smallest Unit]`

		### TotalSupply `sdk.Integer`


		### Aliases `[]string`

		Aliases is an array of strings that are "alternative names" for a token. As an example, while the Ether's denom name might be `ether`, a possible alias could be `ETH`. This field can be useful for UIs and clients. It is intended that this field can be modified by a governance mechanism.


		### Decimals `int8`

		- `Base Unit` = The common standard for the default "standard" size of a token. Examples: 1 Bitcoin or 1 Ether.


		### TotalSupply `sdk.Integer`

		The TotalSupply of a denom is the total amount of a token that exists (known to the chain) across all accounts and modules. It is denominated in the `smallest unit` of a denom. It can be changed by the Keeper functions `MintCoins` and `BurnCoins`. `AddCoins` and `SubtractCoins` are used when adding or subtracting coins for an account, but not removing them from total supply (for example, when moving the coins to the control of the staking module).

R4R: Bank Denom Metadata Spec #2072

R4R: Bank Denom Metadata Spec #2072

Conversation

sunnya97 commented Aug 17, 2018

codecov bot commented Aug 17, 2018

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Aug 24, 2018

rigelrozanski commented Sep 11, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment