-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix RPC responses for Hash160 types #3323
Comments
That is not how it is done. There is no way in the VM to know what is CSharp var address = new UInt160(Convert.FromBase64String(byteStringValue)); |
Hello, Maybe not the VM, but the node. The types are stored in the contract manifest. |
That will change the interface and break all current apps using it. |
Yes, and this is too dangerous to change. The best solution might be to add another RPC endpoint and change the SDKs first. |
That's to much. When a developer can easily change the base64 to bytes and reverse bytes. |
This can't be fixed, see neo-project/neo-modules#609 (comment) |
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. |
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:
The return type of
ownerOf
isHash160
, but the returned value is a byte string:RPC Response Type:
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.
The text was updated successfully, but these errors were encountered: