neo-project / neo-modules

MIT License
60 stars 100 forks source link

TokenId with Base64 #770

Closed Ashuaidehao closed 9 months ago

roman-khimov commented 2 years ago

Why do we need this incompatible change?

Jim8y commented 2 years ago

I dont see why we need this change

shargon commented 2 years ago

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

Jim8y commented 2 years ago

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 commented 2 years ago

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

roman-khimov commented 2 years ago

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.

superboyiii commented 2 years ago

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.

roman-khimov commented 2 years ago

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.

superboyiii commented 2 years ago

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.

joeqian10 commented 2 years ago

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.

superboyiii commented 2 years ago

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.

roman-khimov commented 2 years ago

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).

superboyiii commented 2 years ago

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.

Jim8y commented 1 year ago

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