neo-modules icon indicating copy to clipboard operation
neo-modules copied to clipboard

TokenId with Base64

Open Ashuaidehao opened this issue 2 years ago • 13 comments

Ashuaidehao avatar Oct 14 '22 08:10 Ashuaidehao

Why do we need this incompatible change?

roman-khimov avatar Oct 14 '22 09:10 roman-khimov

I dont see why we need this change

Jim8y avatar Oct 14 '22 13:10 Jim8y

It's less amount of data to store, base64 is shorter than hex

shargon avatar Oct 15 '22 00:10 shargon

incompatible

Indeed, but as @roman-khimov said, this change is minor but incompatible. Does it worth it to have an incompatible update to save a few bytes?

Jim8y avatar Oct 15 '22 01:10 Jim8y

Well, actually this pr dont have to change [RpcMethod], right?

Jim8y avatar Oct 15 '22 03:10 Jim8y

Storage-wise it's the same, we store raw id bytes, network-wise yes, there is some difference, but IDs are limited to 64 bytes, so we can save some tens (not even hundreds) of bytes, but we'll definitely break some applications. I'd rather leave it as is.

roman-khimov avatar Oct 15 '22 06:10 roman-khimov

I think the basic issue is all nep11 related RPC params in RpcServer return Base64 but TokensTracker still need hex input which is not uniform. For example: when someone tried to get tokenid list from invokefunction tokensOf method, he got Base64 tokenId but he can't push it directly into getnep11properties since it needs hex input. Less storage is not the point. Also, convert base64 tokenId into hexstring is not necessary since we don't know which type that tokenId is. Although it should be ByteString in output, finally it can be integer or string in decoding, only the contract deployer knows what it is.

superboyiii avatar Oct 17 '22 02:10 superboyiii

someone tried to get tokenid list from invokefunction tokensOf method

Pretty similar to #609 situation.

Base64 is better in some aspects, that's true. The problem is that this change will break some existing code interacting with RPC servers. If we want to switch, we can make an additional tokenidb64 field and deprecate (but still return) the old one.

roman-khimov avatar Oct 17 '22 07:10 roman-khimov

someone tried to get tokenid list from invokefunction tokensOf method

Pretty similar to #609 situation.

Base64 is better in some aspects, that's true. The problem is that this change will break some existing code interacting with RPC servers. If we want to switch, we can make an additional tokenidb64 field and deprecate (but still return) the old one.

Then input is a problem. It doesn't accept base64 tokenId in getnep11properties.

superboyiii avatar Oct 17 '22 08:10 superboyiii

Request:

{
  "jsonrpc": "2.0",
  "method": "getnep11properties",
  "params": ["0x9f344fe24c963d70f5dcf0cfdeb536dc9c0acb3a","ed00"],
  "id": 1
}

Response:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -2147024809,
        "message": "Unable to translate bytes [ED] at index 0 from specified code page to Unicode."
    }
}

Maybe using base64 encoded token id could avoid this kind of errors.

joeqian10 avatar Oct 27 '22 03:10 joeqian10

Request:

{
  "jsonrpc": "2.0",
  "method": "getnep11properties",
  "params": ["0x9f344fe24c963d70f5dcf0cfdeb536dc9c0acb3a","ed00"],
  "id": 1
}

Response:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -2147024809,
        "message": "Unable to translate bytes [ED] at index 0 from specified code page to Unicode."
    }
}

Maybe using base64 encoded token id could avoid this kind of errors.

@roman-khimov @Liaojinghui We find this error when emit hex, it's time to change to base64.

superboyiii avatar Oct 27 '22 03:10 superboyiii

We find this error when emit hex, it's time to change to base64.

How is it related to hex/base64 question?

$ curl -d '{ "jsonrpc": "2.0", "id": 5, "method": "getnep11properties", "params": ["0x9f344fe24c963d70f5dcf0cfdeb536dc9c0acb3a","ed00"] }' https://rpc10.n3.nspcc.ru:10331
{"id":5,"jsonrpc":"2.0","result":{"name":"\ufffd\u0000","tokenURI":"ipfs.io/ipfs/bafybeies3gtnfneg5mez45em2nhwpfp3r7gzizlyz2sxtfvvxdeqyha7o4/237.json"}}

P.S. BTW, the token just seems to be broken to me. "\ufffd\u0000" is not a proper UTF-8 string and C# node barfs at it for a reason (see neo-project/neo#2984 as well).

roman-khimov avatar Oct 27 '22 05:10 roman-khimov

We find this error when emit hex, it's time to change to base64.

How is it related to hex/base64 question?

$ curl -d '{ "jsonrpc": "2.0", "id": 5, "method": "getnep11properties", "params": ["0x9f344fe24c963d70f5dcf0cfdeb536dc9c0acb3a","ed00"] }' https://rpc10.n3.nspcc.ru:10331
{"id":5,"jsonrpc":"2.0","result":{"name":"\ufffd\u0000","tokenURI":"ipfs.io/ipfs/bafybeies3gtnfneg5mez45em2nhwpfp3r7gzizlyz2sxtfvvxdeqyha7o4/237.json"}}

P.S. BTW, the token just seems to be broken to me. "\ufffd\u0000" is not a proper UTF-8 string and C# node barfs at it for a reason (see neo-project/neo#2984 as well).

Yes, you're right, it's a UTF-8 issue but not hex/base64 issue.

superboyiii avatar Oct 27 '22 06:10 superboyiii

I will close this pr if no further claims to support it.

Jim8y avatar Oct 09 '23 09:10 Jim8y