Cosmos-sdk: JSON Number Consistency

[YunSuk-Yeo]

Summary of Bug

After updating [email protected], some struct like Account or RelegationResponse are using json.Marshal() function on its custom MarshalJSON function. But others are using amino codec like Validator.

This inconsistency causes huge suffer from number parsing on client program. json.Marshal is treating primitive int64/uint64 as number not string, but amino codec is treating those as string.

Moreover sdk.Int is encoded as string on both codec, it is too wired. Some integer types on some objects are encoded as string and some integer types on some objects are encoded as number.

Ex) Account info - account number & sequence number (It was string util v0.37.x)

{
  'type': 'core/Account',
  'value': {
    'address': 'terra1nty4ku4a79zj45jl0fy5rrjun07ny0nrve7j99',
    'coins': [
      {
        'denom': 'ukrw',
        'amount': '1000000000000000'
      },
      {
        'denom': 'uluna',
        'amount': '99989998000000'
      },
      {
        'denom': 'uusd',
        'amount': '100000000000000'
      }
    ],
    'public_key': 'terrapub1addwnpepqftumdzplp6mtz4cp0qavxmw245v9v8vd9gshh940pflk4cahuspx2erv6w',
    'account_number': 0,
    'sequence': 10
  }
}

Ex) Validator query - unbonding_height is int64 treated as string

{
    "operator_address": "terravaloper1vk20anceu6h9s00d27pjlvslz3avetkvnwmr35",
    "consensus_pubkey": "terravalconspub1zcjduepqwgwyky5375uk0llhwf0ya5lmwy4up838jevfh3pyzf5s3hd96xjslnexul",
    "jailed": false,
    "status": 2,
    "tokens": "10100000000",
    "delegator_shares": "10100000000.000000000000000000",
    "description": {
      "moniker": "node0",
      "identity": "",
      "website": "",
      "security_contact": "",
      "details": ""
    },
    "unbonding_height": "0",
    "unbonding_time": "1970-01-01T00:00:00Z",
    "commission": {
      "commission_rates": {
        "rate": "0.100000000000000000",
        "max_rate": "0.200000000000000000",
        "max_change_rate": "0.010000000000000000"
      },
      "update_time": "2020-07-13T08:30:00Z"
    },
    "min_self_delegation": "1"
  }

Maybe there are lots of uint64 data relayed to client in this format.
I think, for the safe application communication, the string format is recommended.

Problematic case can be easily found like decoding response at javascript. All the client program will be affected.

Version

v0.38.x

Steps to Reproduce

create account & query account as JSON format

---

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[marbar3778]

@ethanfrey @webmaster128 Is this an issue in cosmjs?

[webmaster128]

Both yaml and JSON support numbers > 53 bit in their spec. Unfortunately, many implementations convert them to IEEE 754 floats, making it impossible to operate on them without rounding errors.

JSON integer support > 53 bit

• are supported in: Python, Rust with serde-json, Go?
• are not supported in: JavaScript, jq

For fields that can exceed Number.MAX_SAFE_INTEGER (9007199254740991), the number type should be avoided.

For fields that are auto-incrementing and start at 0 or 1, it depends on the expected range. Then the question becomes: do you expect more than 9007199254740991 accounts (more than 1 millions accounts per human being) or more than 9007199254740991 transactions per account (285616 years sending one tx per millisecond)?

[ethanfrey]

I have seen account number and sequence change from int to string to int again. I just adapt the client parse logic.

I think both are fine (what Simon says), it is more updating your parsing code. (btw, what js lib is Terra using?)

[alexanderbez]

Why is YAML being discussed here? This has nothing to do with YAML. So a few things:

1. Queriers were not changed. They still return raw Amino-encoded JSON output.
2. Most types Stringer implementation, return YAML output -- i.e. human-readable text. This is the default response format in the CLI, but can opt to return JSON as well.

What you're stating has nothing to do with Text (YAML) output or String implementations perse, but the field types declared when JSON encoded.

In the example you've posted, the Account has coins which amount are sdk.Int, which _have custom JSON marshaller_ methods that return a string, whereas account_number and sequence are primitive uint64 types, they're not strings when JSON encoded.

In order to have unification in this regard -- i.e. all numbers as strings, you'll need to implement custom JSON marshalling for every single type.

[YunSuk-Yeo]

@alexanderbez thanks for renaming the issue title. This is not the only thing that is inconsistent. When you request POST /txs to rest-server you have to put uint64 variables as String not Number. But the all queriers return uint64 as Number.

Ex) gas of sdk.StdTx has uint64 format

curl -X POST -H "Content-Type: application/json" -d '{"tx": {"msg":[{"type":"bank/MsgSend","value":{"from_address":"terra1fzxfdvpy4l0v8jeagv00u5j93xh9w03rytssw6","to_address":"terra1fzxfdvpy4l0v8jeagv00u5j93xh9w03rytssw6","amount":[{"denom":"uluna","amount":"14000000"}]}}],"fee":{"amount":[{"denom":"ukrw","amount":"3000"}],"gas":200000},"signatures":null,"memo":"937767194"}, "mode": "sync"}' http://127.0.0.1:1317/txs

{"error":"invalid character -- Amino:JSON int/int64/uint/uint64 expects quoted values for javascript numeric support, got: 200000."

@webmaster128 agree on that two fields, but the sdk has more fields with uint64 data type in each module's params or in the block info. We can determine whether the field will exceed int max or not, but it is unnecessary works to do that always.

@ethanfrey we are using our own js library here

[alexanderbez]

Yeah I guess that is pretty annoying. Hopefully, we'll be moving away from Amino JSON soon.

[hanjukim]

I think this inconsistency came with https://github.com/cosmos/cosmos-sdk/commit/82a2c5d6d86ffd761f0162b93f0aaa57b7f66fe7
In my opinion, this is a case of a wrong update. Some modules use the Amino marshaller, some use the JSON marshaller, and so many programs and developers will suffer from this problem.

[YunSuk-Yeo]

@alexanderbez now I understand the problem is not YAMl, it is on custom JSON Marshaler. I updated the issue description. Can you see the fixed issue?

Here is summary,

Account custom JSON Marshaler is using json.Marshal, but other parts are using amino codec.Cdc.MarshalJSON to build custom JSON Marshaler.

[webmaster128]

@webmaster128 agree on that two fields, but the sdk has more fields with uint64 data type in each module's params or in the block info. We can determine whether the field will exceed int max or not, but it is unnecessary works to do that always.

I agree. A generic solution should use strings for integers. Otherwise you run in big trouble like

$ echo 18446744073709551611 | jq
18446744073709552000

[YunSuk-Yeo]

Other issue from the same code #6745

[alexanderbez]

Yeah since we're using Amino still, I suppose any custom JSON marshaling should use strings for numbers.