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

TokenId with Base64 #770

Closed

Conversation

Ashuaidehao
Copy link
Contributor

No description provided.

@roman-khimov
Copy link
Contributor

Why do we need this incompatible change?

@Jim8y
Copy link
Contributor

Jim8y commented Oct 14, 2022

I dont see why we need this change

@shargon
Copy link
Member

shargon commented Oct 15, 2022

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

@Jim8y
Copy link
Contributor

Jim8y commented Oct 15, 2022

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
Copy link
Contributor

Jim8y commented Oct 15, 2022

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

@roman-khimov
Copy link
Contributor

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
Copy link
Member

superboyiii commented Oct 17, 2022

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

roman-khimov commented Oct 27, 2022

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
Copy link
Member

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
Copy link
Contributor

Jim8y commented Oct 9, 2023

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

@shargon shargon closed this Jan 8, 2024
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

Successfully merging this pull request may close these issues.

6 participants