optimize invoke output #316
Conversation
|
Hardly check manifest outside of the vm :( . |
| { | ||
| try | ||
| { | ||
| text = Encoding.UTF8.GetString(data); |
There was a problem hiding this comment.
Wouldn't be easier to use Strict UTF-8?
There was a problem hiding this comment.
It will get very low perfomance to catch the exception when using random data with Strict UTF-8. try-catch here is use for robust, and I don't expect it runs into catch case.
| byte[] reencodedBytes = Encoding.UTF8.GetBytes(text); | ||
| for (int i = 0; i < data.Length; i++) | ||
| { | ||
| if (data[i] != reencodedBytes[i]) |
There was a problem hiding this comment.
Could check for char.IsAscii()
There was a problem hiding this comment.
I'm sorry I don't get it. If the data is encoded utf-8 string, checking IsAscii is not enough.
There was a problem hiding this comment.
You allow strings to be displayed that are not human readable?
There was a problem hiding this comment.
If utf-8 convert successfully, it is already readable without extra checking.
There was a problem hiding this comment.
ok thought you were using ASCII for some reason... 😂
| await WriteLineAsync(Neo.Helper.ToHexString(byteString.GetSpan())).ConfigureAwait(false); | ||
| if (byteString.GetSpan().TryGetUtf8String(out var text)) | ||
| { | ||
| await WriteLineAsync($"{Neo.Helper.ToHexString(byteString.GetSpan())}({text})").ConfigureAwait(false); |
There was a problem hiding this comment.
I think text 1st then hex. Or put on different lines when printing.
Example: Return: "Hello World" (Hex-Format: "48656c6c6f20576f726c64")
There was a problem hiding this comment.
It is not a accurate convert, some random data could be converted to string. So I prefer show its text as a possible value after the certain value(HexString).
There was a problem hiding this comment.
Yes but you are showing string and hex-string. Think it neater for it to look like this "Hello World" (Hex-Format: "48656c6c6f20576f726c64") and more understandable.
There was a problem hiding this comment.
You have the StackItem, why not use StackItem.GetString()? that will fix this problem.
There was a problem hiding this comment.
StackItem.GetString() will throw exception if it is not a valid string.
|
Merge? |
|
@Liaojinghui i still think that we should be checking the manifest for return type string or be using |
|
Hello! I'm a bit worried about this PR 🤔 It doesn't look like I expected it to be 🤔 🤔 🤔 Can we do a post-processing step to parse the JSON and use the manifest to ensure String compatibility? @Ashuaidehao |
Yes, but the best way to implement it requires neo-vm expose more infomations, it may cost more time to discuss and couldn't process custom scripts without contract invoking. This PR is just a quick way show possible value to developers, and the developers should know what the return value is. |
What if result comes from an oracle? |
Could oracle results show here? I'm not sure if I miss some scenarios. |
|
I dont remember, but i can't see why not. |
Try to convert ByteString to string when invoke contract with "--result".
Partial solution of #274.