Bug: Invalid params: Invalid Uint64 0x00: with redundant leading zeros.

[Hanssen0]

Caused by: #268

Request:

{
  "id": 0,
  "method": "get_cells_capacity",
  "params": [
    {
      "script": {
        "code_hash": "0xd23761b364210735c19c60561d213fb3beae2fd6172743719eff6920e020baac",
        "hash_type": "type",
        "args": "0x00015cd17fc7c8c994297a7add0dbe494491de607548"
      },
      "script_type": "lock",
      "script_search_mode": "exact",
      "filter": {
        "script_len_range": [
          "0x00",
          "0x01"
        ],
        "output_data_len_range": [
          "0x00",
          "0x01"
        ]
      }
    }
  ],
  "jsonrpc": "2.0"
}

Response:

{
  "jsonrpc": "2.0",
  "error": {
    "code": -32602,
    "message": "Invalid params: Invalid Uint64 0x00: with redundant leading zeros."
  },
  "id": 0
}

CKB RPC only accepts hex numbers, not hex byte numbers.

[Hanssen0]

cc @phroi

[phroi]

Yeah, I got the feeling that it would turn into something like this. I wouldn't say that the RPC behavior is correct tho.

[phroi]

Old numToHex didn't produce a proper ccc.Hex and that's why we agree on going thru with the change.

[phroi]

It's the kind of situation where we are damned if we do something and damned if we don't. I'd like to point the attention to this message:

Hanseen: We shouldn't do that right now. Let's wait until there are actual needs. [Note: Referred to old behaviour]

The only need is gonna ever appear is when devs stumble in this in a form of a difficult-to-trace bug.

That said, if we modify the behavior of numToHex new bugs may appear in people code, which is undesirable.

That's why I'm also open to deprecate the numToHex

[Hanssen0]

Yeah, I got the feeling that it would turn into something like this. I wouldn't say that the RPC behavior is correct tho.

I don't think there's a correct or incorrect answer here - just two different standards. I think there is nothing wrong with our update for numToHex, especially since it will be included in the next major version.

Yet, we still need to fix this problem. Maybe a new method to handle ABI hex numbers? I don't know what will be a suitable name for it, numToHexNum numToJsonRpc JsonRpcTransformers.num*?

[Hanssen0]

Update:
After some attempts, I decided to revert numToHex's behavior (the negative check is kept).

1. numToHex is useful, so it shouldn't be deprecated/removed. It's mainly used for JSON representations of Nums/bigints, thanks to its readability, which is widely used for front-end to back-end communication.
2. Padding hex number is serializing in big-endian. As we know, CKB prefers little endianness when serializing numbers. That's also true in CCC, like numToBytes.

console.log(
  ccc.numToHex("0xabc"),
  ccc.hexFrom(ccc.numToBytes("0xabc")),
); // "0xabc" "0xbc0a"

We can't regard numToHex as a shorthand of ccc.hexFrom(ccc.numToBytes("0xabc")) because of this, then, it's (almost) meaningless after modification (Who will need big-endian serialization?).

#317 positions numToHex as converting Num to a JSON hex representation, and adds extra documentation to help developers understand this.

[phroi]

I'm just a bit unsettled that ccc.isHex(ccc.numToHex(1n)) === false

Maybe I would just had deprecated numToHex and moved it off to JsonRpcTransformers, possibly declaring the result a string, not an ccc.Hex.

At any rate, the solution applied is a good compromise 👍

Phroi