neo icon indicating copy to clipboard operation
neo copied to clipboard

Fix RPC responses for Hash160 types

Open lock9 opened this issue 1 year ago • 8 comments

Describe the bug Nodes return ByteStrings when a method return/event type is Hash160 (probably Hash256 too). Due to this bug, developers must reverse the array before using it as a scripthash / address. Methods like 'ownerOf' will return the incorrect owner script hash.

To Reproduce Please try this script from Edge:

import { rpc, sc, wallet, u } from "@cityofzion/neon-js";

const client = new rpc.RPCClient("https://testnet1.neo.coz.io:443");
const ghostmarketContract = "0xc414275b3eca3969c4ca49f6a1fb67013fbe0544";

const res = await client.invokeFunction(ghostmarketContract, "ownerOf", [
    sc.ContractParam.byteArray("JwE="),
]);

const b64 = res.stack[0].value;
const hex = u.base642hex(b64);
const address = wallet.getAddressFromScriptHash(hex);

console.log(address);
// NdfRZ4JvyKw4F2kK7K7ffSNUm5Y18F7ASw <-- wrong, but why?

The return type of ownerOf is Hash160, but the returned value is a byte string:

Screenshot 2024-06-11 at 03 09 53

RPC Response Type: Screenshot 2024-06-11 at 03 18 56

Expected behavior The value must be reversed / parsed properly if the type is Hash160 or Hash256.

** Additional context** This is a major breaking change, but it also fixes an annoying issue on Neo. Most applications already deal with this bug as it was a feature, so changing this will require developers to make changes to their applications. This will be 'chaotic' for existing developers, but will make it much easier for new ones.

**I'm not 100% sure if this is something on the SDKs or on the RPC node, but it's probably on the RPC API.

lock9 avatar Jun 11 '24 06:06 lock9

That is not how it is done. There is no way in the VM to know what is hash160 or hash256

CSharp

var address = new UInt160(Convert.FromBase64String(byteStringValue));

cschuchardt88 avatar Jun 11 '24 06:06 cschuchardt88

Hello,

Maybe not the VM, but the node. The types are stored in the contract manifest.

lock9 avatar Jun 11 '24 06:06 lock9

That will change the interface and break all current apps using it.

cschuchardt88 avatar Jun 11 '24 06:06 cschuchardt88

Yes, and this is too dangerous to change. The best solution might be to add another RPC endpoint and change the SDKs first. We can argue that developers should use their own RPC nodes, but most applications use public RPC nodes. Then, it's probably ideal to introduce a new endpoint instead of changing the existing ones.

lock9 avatar Jun 11 '24 07:06 lock9

That's to much. When a developer can easily change the base64 to bytes and reverse bytes.

cschuchardt88 avatar Jun 11 '24 07:06 cschuchardt88

This can't be fixed, see https://github.com/neo-project/neo-modules/issues/609#issuecomment-875128591

roman-khimov avatar Jun 11 '24 08:06 roman-khimov

Why it can't be fixed if it's an RPC endpoint?

The 'GetInvokeResults' method is not part of the VM. It could accept an additional parameter to parse types using the manifest.

If the conversion fails, it's because there is a bug in the smart contract. Most compilers won't allow you to return different types. This can only happen using "incorrect" storage calls.

If the conversion fails, the developer can either fix their contract or omit the "parse types" parameter.

lock9 avatar Jun 11 '24 08:06 lock9

  • In C# contract compiler (and other compilers) it is allowed to return ByteString when the declared return type is UInt160. This saves GAS.
  • At runtime in the VM, the type of returned value is not strictly enforced.
  • ByteString is little-endian
  • ScriptHash is big-endian.
const b64 = res.stack[0].value;  // little-endian
const hex = u.base642hex(b64);   // little-endian
const address = wallet.getAddressFromScriptHash(hex);  // getAddressFromScriptHash accepts only big-endian, but `hex` is little-endian

Hecate2 avatar Dec 04 '24 03:12 Hecate2