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

Missing decimal in DecCoin amount string. #247

Closed
Tracked by #278
NoahSaso opened this issue Oct 26, 2022 · 15 comments
Closed
Tracked by #278

Missing decimal in DecCoin amount string. #247

NoahSaso opened this issue Oct 26, 2022 · 15 comments

Comments

@NoahSaso
Copy link
Contributor

NoahSaso commented Oct 26, 2022

Source

method DelegatorDelegations of service cosmos.staking.v1beta1.Query with delegatorAddr: juno1egefqhp4tv9gjat3p4clxhstfhw5djfl920p6zcfe8xdymqpuq7s33csz6

using the client.staking.v1beta1.delegatorDelegations function of interchain-rpc

Issue

This query returns the following object:

{
    "delegationResponses": [
        {
            "delegation": {
                "delegatorAddress": "juno1egefqhp4tv9gjat3p4clxhstfhw5djfl920p6zcfe8xdymqpuq7s33csz6",
                "validatorAddress": "junovaloper184faqy958htcwzhgxqrpv0e3kcwhr3yqzqst0f",
                "shares": "1337000000000000000000000"
            },
            "balance": {
                "denom": "ujuno",
                "amount": "1337000"
            }
        }
    ],
    "pagination": {
        "nextKey": {},
        "total": {
            "low": 1,
            "high": 0,
            "unsigned": true
        }
    }
}

The balance property of the delegation response is properly typed with a ujuno integer, representing 1.337 $JUNO. The delegation.shares property of the delegation response is typed in decimal ujuno, which has 18 decimals, but appears to be missing the decimal in its string serialization.

Expected behavior

junod query staking delegations juno1egefqhp4tv9gjat3p4clxhstfhw5djfl920p6zcfe8xdymqpuq7s33csz6 --output json | jq

outputs the following object:

{
  "delegation_responses": [
    {
      "delegation": {
        "delegator_address": "juno1egefqhp4tv9gjat3p4clxhstfhw5djfl920p6zcfe8xdymqpuq7s33csz6",
        "validator_address": "junovaloper184faqy958htcwzhgxqrpv0e3kcwhr3yqzqst0f",
        "shares": "1337000.000000000000000000"
      },
      "balance": {
        "denom": "ujuno",
        "amount": "1337000"
      }
    }
  ],
  "pagination": {
    "next_key": null,
    "total": "0"
  }
}

and the LCD query responds with the following object:

{
  "delegation_responses": [
    {
      "delegation": {
        "delegator_address": "juno1egefqhp4tv9gjat3p4clxhstfhw5djfl920p6zcfe8xdymqpuq7s33csz6",
        "validator_address": "junovaloper184faqy958htcwzhgxqrpv0e3kcwhr3yqzqst0f",
        "shares": "1337000.000000000000000000"
      },
      "balance": {
        "denom": "ujuno",
        "amount": "1337000"
      }
    }
  ],
  "pagination": {
    "next_key": null,
    "total": "1"
  }
}

Both of these responses correctly insert the decimal place where expected.

Preliminary investigation

This also occurs with interchain-rpc's client.distribution.v1beta1.delegationTotalRewards function, which responds with:

{
    "rewards": [
        {
            "validatorAddress": "junovaloper184faqy958htcwzhgxqrpv0e3kcwhr3yqzqst0f",
            "reward": [
                {
                    "denom": "ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9",
                    "amount": "252437729264000"
                },
                {
                    "denom": "ujuno",
                    "amount": "1417433111993671052000"
                }
            ]
        }
    ],
    "total": [
        {
            "denom": "ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9",
            "amount": "252437729264000"
        },
        {
            "denom": "ujuno",
            "amount": "1417433111993671052000"
        }
    ]
}

The junod and LCD equivalents, as expected, insert the decimal point before their 18 decimal places.

It seems to be an issue serializing DecCoin in interchain-rpc. The correctly typed balance property, which is a ujuno integer, uses the Coin type, whereas all the numbers with missing decimal points in interchain-rpc's responses are typed with DecCoin. Coin and DecCoin in interchain-rpc/src/codegen/cosmos/base/v1beta1/coin.ts appear to encode and decode in the exact same way. It seems likely to me that DecCoin should be decoding the values differently, aware of decimals.

@webmaster128
Copy link

This is tracked here: cosmos/cosmos-sdk#10863

@NoahSaso
Copy link
Contributor Author

Manually using decodeCosmosSdkDecFromProto from @cosmjs/stargate for now:

decCoin.amount = Math.floor(
  decodeCosmosSdkDecFromProto(
    decCoin.amount
  ).toFloatApproximation()
).toString()

@webmaster128
Copy link

Sounds good. But if you want a string anyways, there is no need to convert to a float first an lose precision. You can just do:

decCoin.amount = decodeCosmosSdkDecFromProto(decCoin.amount).toString()

@NoahSaso
Copy link
Contributor Author

NoahSaso commented Oct 26, 2022

I want a string of the floored integer first. Though I guess I could do parseInt(decodedDecCoin.toString(), 10)

@webmaster128
Copy link

Decimal can do that for you: decodeCosmosSdkDecFromProto(decCoin.amount).floor().toString()

@NoahSaso
Copy link
Contributor Author

Ah perfect! Thank you.

@webmaster128
Copy link

If you miss any operations on Decimal, let me know. Often they can be implemented without loss of precision in the Decimal type directly.

@NoahSaso
Copy link
Contributor Author

Awesome :) Appreciate it.

@alexdonh
Copy link

So... basically any string fields having cosmos.Dec as scalar are in effect?

I did a quick search:

cosmos-sdk/proto/cosmos/distribution/v1beta1/distribution.proto

string community_tax
string base_proposer_reward
string bonus_proposer_reward

string fraction
string stake

cosmos-sdk/proto/cosmos/gov/v1/gov.proto

string     weight

string quorum
string threshold
string veto_threshold

cosmos-sdk/proto/cosmos/gov/v1beta/gov.proto

string     weight

cosmos-sdk/proto/cosmos/mint/v1beta1/mint.proto

string inflation
string annual_provisions

string inflation_rate_change
string inflation_max
string inflation_min
string goal_bonded

cosmos-sdk/proto/cosmos/staking/v1beta1/staking.proto

string rate
string max_rate
string max_change_rate

string delegator_shares 
string shares 
string shares_dst

cosmos-sdk/proto/cosmos/staking/v1beta1/tx.proto

string commission_rate 

and of course

cosmos-sdk/proto/cosmos/base/v1beta1/coin.proto

string amount

what i did was a little "dirty": i manually modified the encode and decode function in the generated *.ts files like this:

encode(message: DecCoin, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
  ...
  if (message.amount !== "") {
    writer.uint32(18).string(Decimal.fromUserInput(message.amount, 18).atomics);
  }
  ...
}

decode(input: _m0.Reader | Uint8Array, length?: number): DecCoin {
  ...
  message.amount = Decimal.fromAtomics(reader.string(), 18).toString();
  ...
}

this way i don't have to modify either the message or the query response of anything returning Dec

i hope i'm not doing anything wrong :(
wish there could be a way to custom the generator for the time being.

@pyramation
Copy link
Collaborator

what i did was a little "dirty": i manually modified the encode and decode function in the generated *.ts files like this:

encode(message: DecCoin, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
  ...
  if (message.amount !== "") {
    writer.uint32(18).string(Decimal.fromUserInput(message.amount, 18).atomics);
  }
  ...
}

decode(input: _m0.Reader | Uint8Array, length?: number): DecCoin {
  ...
  message.amount = Decimal.fromAtomics(reader.string(), 18).toString();
  ...
}

this way i don't have to modify either the message or the query response of anything returning Dec

i hope i'm not doing anything wrong :( wish there could be a way to custom the generator for the time being.

This is pretty cool! What is Decimal? and where does this come from? I like this approach, I think we can make this an option for sure. Would be curious if we do both encode/decode, while it does seem fine to me, I'd just want to double check with @webmaster128 if this is ok?

We can make this an option and test it out for sure

@alexdonh
Copy link

alexdonh commented Jan 3, 2023

This is pretty cool! What is Decimal? and where does this come from? I like this approach, I think we can make this an option for sure. Would be curious if we do both encode/decode, while it does seem fine to me, I'd just want to double check with @webmaster128 if this is ok?

We can make this an option and test it out for sure

@pyramation I use

import {Decimal} from "@cosmjs/math";

@webmaster128
Copy link

This is pretty cool! What is Decimal? and where does this come from? I like this approach, I think we can make this an option for sure. Would be curious if we do both encode/decode, while it does seem fine to me, I'd just want to double check with @webmaster128 if this is ok?

I'd not pull in the Decimal type here. But what we can do is implement the string conversion

protobuf                   JavaScript
"1234"                 <-> "0.000000000000001234"
"56780000000000000000" <-> "56.780000000000000000"

in Telescope. The resulting string can then be read into a Decimal optionally if a more advanced type is needed.

@alexdonh
Copy link

alexdonh commented Jan 3, 2023

I'd not pull in the Decimal type here. But what we can do is implement the string conversion

protobuf                   JavaScript
"1234"                 <-> "0.000000000000001234"
"56780000000000000000" <-> "56.780000000000000000"

in Telescope. The resulting string can then be read into a Decimal optionally if a more advanced type is needed.

you're right. I use Decimal just to convert it toString()

@pyramation
Copy link
Collaborator

pyramation commented Mar 29, 2023

this could potentially solve this issue, we have a new flag to test for this now:

#332

 @osmonauts/[email protected]

change the default config to be true to enable (default is false)

@pyramation
Copy link
Collaborator

closed with #332

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

4 participants