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

Problem: base denomination can't be parsed with decimal point #16

Closed
calvinaco opened this issue Sep 3, 2020 · 14 comments
Closed

Problem: base denomination can't be parsed with decimal point #16

calvinaco opened this issue Sep 3, 2020 · 14 comments
Assignees

Comments

@calvinaco
Copy link
Contributor

Right now we use Carson for the name 😂 but ideally we should have a official name for the base unit. This will help to facilitate the communication.

@tomtau tomtau changed the title Problem: CRO basic unit name Problem: unofficial CRO basic unit name Sep 4, 2020
@tomtau tomtau changed the title Problem: unofficial CRO basic unit name Problem: unofficial CRO base unit name Sep 4, 2020
@tomtau
Copy link
Contributor

tomtau commented Sep 4, 2020

@calvinaco is that an issue here? in SDK's codebase, base units aren't really exposed / are rarely used (it seems all "end-user" RPC, CLI, configs etc. specify values in full coins)?
one other thing is that SDK has a fixed precision that's larger than the original erc20's 8 decimal points -- and it's used everywhere... "carson" referred to the base unit in the old code (and erc20), but here one can have "sub-carson" units

@calvinaco
Copy link
Contributor Author

calvinaco commented Sep 4, 2020

@tomtau Oh actually this is something I want to clarify first as well. In the SDK the Coin should be CRO or the base unit? Because it seems coin is the base unit and the proposal to further divide the "coin" to smaller usb unit is still in draft. But perhaps I overlook the concept.

If we look into the gaia genesis, it is also specifying the uatom but not atom
https://raw.githubusercontent.com/cosmos/launch/master/genesis.json

@tomtau
Copy link
Contributor

tomtau commented Sep 4, 2020

@calvinaco SDK's Coin is flexible -- it will always operate on that highest/fixed precision.
For application denominations, one can configure/register them and then convert between them: https://github.com/cosmos/cosmos-sdk/blob/master/types/denom.go#L44

@tomtau
Copy link
Contributor

tomtau commented Sep 4, 2020

so I suggest just using "base denomination" as CRO (inputted with a decimal point if needed).
if there's ever a need to have that weird 10^-8 denomination (e.g. if it'll make easier some conversions from Eth), then just call RegisterDenom("carson", NewDecWithPrec(1, 8)), as it's developer-only and it's already used

@calvinaco
Copy link
Contributor Author

Oh, but do coin support decimal places? Because I saw the parse coin function in #15 , and the implementation seems to assume integer only.

Reference: https://github.com/cosmos/cosmos-sdk/blob/master/types/coin.go#L622

@tomtau tomtau changed the title Problem: unofficial CRO base unit name Problem: base denomination can't be parsed with decimal point Sep 4, 2020
@tomtau
Copy link
Contributor

tomtau commented Sep 4, 2020

@calvinaco it seems it's always an integer to prevent confusion -- in that case, it's perhaps the easiest just to use SI units (milliCRO, microCRO, nanoCRO). "nanoCRO" won't have a correspondence on Eth, so one visual check is that, in genesis, it should always end with "0" (1 carson = 10 nanoCRO) -- in the case that any amounts in genesis need to be specified with sub-CRO denominations

@leejw51crypto
Copy link
Contributor

leejw51crypto commented Sep 4, 2020

it can be done by changing denom in genesis
denom_metadata, when genesis is created

milli, micro, nano will be easy to use

@calvinaco
Copy link
Contributor Author

it can be in changing denom in genesis
denom_metadata, when genesis is created

milli, micro, nano will be easy to use

Oh, how does it work? Because I saw it is in a draft proposal here
https://docs.cosmos.network/master/architecture/adr-024-coin-metadata.html

@leejw51crypto
Copy link
Contributor

leejw51crypto commented Sep 4, 2020

i think, it's given by genesis block.
that adr merged into code, already

how about adding init function to chain-main daemon?
which does, override various parameters for cro

@tomtau
Copy link
Contributor

tomtau commented Sep 4, 2020

I can try to work on this -- I think setting denominations should just be a matter of registering them e.g. here: https://github.com/crypto-com/chain-main/blob/master/app/prefix.go#L17

@calvinaco
Copy link
Contributor Author

calvinaco commented Sep 4, 2020

I can try to work on this -- I think setting denominations should just be a matter of registering them e.g. here: https://github.com/crypto-com/chain-main/blob/master/app/prefix.go#L17

I can share some of my experiments and findings here:

To use the denmo.go, yes I think we just need to call it at app bootstrap. But https://github.com/cosmos/cosmos-sdk/blob/master/types/denom.go#L44 .

On the other hand, I tried deno_metadata, and after some tries I realize the proposal is implemented but only on the master branch
Commit: cosmos/cosmos-sdk@5a7e220#diff-593a9b28254d0c45f2e9c3974b0dd55d
Genesi: https://github.com/cosmos/cosmos-sdk/blob/master/x/bank/keeper/genesis.go#L42
Coin: https://github.com/cosmos/cosmos-sdk/blob/master/types/coin.go
so it is not available on 0.39.1 yet

Either way, they seems to assume always integer arithmetic. For example, let's say we want to say send 1.0000884cro, both the solutions seems won't support it. I could think of two ways, and maybe more:

  • We keep only the base unit in the chain code, and implement the deno logic in client level (SDK, chain-maincli)
  • We implement our own coin denom logic in the chain. Something like this: https://play.golang.org/p/32F5xDD6j55 but more complicated (this is for the Block explorer experiment and I tweak it a bit)
  • and other solutions...

@cdc-Hitesh
Copy link

cdc-Hitesh commented Sep 4, 2020

We keep only the base unit in the chain code, and implement the deno logic in client level (SDK, chain-maincli)

+1 For this approach. Provided BigNumber handling is seamless.

@tomtau
Copy link
Contributor

tomtau commented Sep 5, 2020

Either way, they seems to assume always integer arithmetic. For example, let's say we want to say send 1.0000884cro, both the solutions seems won't support it. I could think of two ways, and maybe more:

integer arithmetic is there for determinism (across platforms and architectures) -- we only do integer arithmetic in the old code.

@calvinaco
Copy link
Contributor Author

Either way, they seems to assume always integer arithmetic. For example, let's say we want to say send 1.0000884cro, both the solutions seems won't support it. I could think of two ways, and maybe more:

integer arithmetic is there for determinism (across platforms and architectures) -- we only do integer arithmetic in the old code.

Oh I see, thanks for the clarification.

tomtau added a commit that referenced this issue Sep 7, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
tomtau added a commit that referenced this issue Sep 8, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
tomtau added a commit that referenced this issue Sep 8, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
@tomtau tomtau closed this as completed in 332d09c Sep 8, 2020
damoncro pushed a commit to damoncro/chain-main that referenced this issue Oct 25, 2021
Solution:
- adopt the one in chain-main project
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants