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

Convert InteropInterface<IEnumerator> to Array for RpcClient to digest #245

Closed
wants to merge 13 commits into from

Conversation

joeqian10
Copy link
Contributor

Close #230

Any suggestions and modifications are welcome.

@@ -70,6 +75,27 @@ private JObject GetInvokeResult(byte[] script, IVerifiable checkWitnessHashes =
return json;
}

public static StackItem[] ConvertIEnumeratorToArray(StackItem[] stackItems)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the items in the same array, we should not return an array, because it's the same.

12345,
"hello",
new InteropInterface(new int[] { 1, 2, 3 }.GetEnumerator()),
new InteropInterface(new Nep5AccountState[] {new Nep5AccountState()}.GetEnumerator())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a StorageMap type here, which used in nep11, see neo-project/neo-devpack-dotnet#268

while (enumerator.MoveNext())
{
var current = enumerator.Current;
array.Add(current is StackItem stackItem ? stackItem : current is IInteroperable interoperable ? interoperable.ToStackItem(null) : new InteropInterface(current));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too line to read, it's better to split into multiple lines.

shargon
shargon previously approved these changes May 26, 2020
@ProDog
Copy link
Contributor

ProDog commented May 26, 2020

If the contract contains return Enumerator<byte[]> , an exception will be thrown when call invokefunction.

Contract like:

        public static Enumerator<byte[]> GetEnumerator()
        {
            var a = new byte[] { 0x22, 0x32, 0x24 };
            var b = new byte[] { 0x10, 0x11, 0x22 };
            var c = new byte[] { 0x33, 0x44, 0x55 };
            var enumerator = Enumerator<byte[]>.Create(new byte[3][] { a, b, c });
            
            return enumerator;
        }

invokefunction:

{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "invokefunction",
  "params": [
    "0xa02a4bb9dbadb03b41507efe1910ecdfb7d3d146",
    "getEnumerator"
  ]
}

result:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -2147024809,
        "message": "StackItemType(Buffer) is not supported to ContractParameter.",
        "data": "   at Neo.VM.Helper.ToParameter(StackItem item, List`1 context) in D:\\Work\\neo_code\\neo\\src\\neo\\VM\\Helper.cs:line 340\r\n   at Neo.VM.Helper.<>c__DisplayClass16_0.<ToParameter>b__0(StackItem p) in D:\\Work\\neo_code\\neo\\src\\neo\\VM\\Helper.cs:line 289\r\n   at System.Linq.Enumerable.SelectEnumerableIterator`2.ToList()\r\n   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)\r\n   at Neo.VM.Helper.ToParameter(StackItem item, List`1 context) in D:\\Work\\neo_code\\neo\\src\\neo\\VM\\Helper.cs:line 289\r\n   at Neo.VM.Helper.ToParameter(StackItem item) in D:\\Work\\neo_code\\neo\\src\\neo\\VM\\Helper.cs:line 271\r\n   at Neo.Plugins.RpcServer.<>c.<GetInvokeResult>b__31_0(StackItem p)\r\n   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext()\r\n   at System.Collections.Generic.List`1.InsertRange(Int32 index, IEnumerable`1 collection)\r\n   at Neo.IO.Json.JArray..ctor(IEnumerable`1 items) in D:\\Work\\neo_code\\neo\\src\\neo\\IO\\Json\\JArray.cs:line 18\r\n   at Neo.Plugins.RpcServer.GetInvokeResult(Byte[] script, IVerifiable checkWitnessHashes)\r\n   at Neo.Plugins.RpcServer.InvokeFunction(JArray _params)\r\n   at Neo.Plugins.RpcServer.ProcessRequest(HttpContext context, JObject request)"
    }
}

Co-authored-by: HaoqiangZhang <gripzhang@outlook.com>
@gsmachado
Copy link

just to mention that this is pretty important to us since many of our tests are failing at the moment (we had to ignore them) due to this. 😄

@ProDog ProDog requested review from shargon and Tommo-L May 28, 2020 14:15
Tommo-L
Tommo-L previously approved these changes May 29, 2020
@joeqian10
Copy link
Contributor Author

This PR also closes #254

Now the GetInvokeResult method directly converts a StackItem to json, so there will no longer be ByteArray type in the response.

shargon
shargon previously approved these changes Jun 5, 2020
@joeqian10 joeqian10 dismissed stale reviews from shargon and Tommo-L via 2ef4f8c June 8, 2020 02:28
shargon
shargon previously approved these changes Jun 8, 2020
@ProDog
Copy link
Contributor

ProDog commented Jun 12, 2020

Please review this PR.@erikzhang

Copy link
Member

@erikzhang erikzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the enumerator returns too many items, it may cause out of memory exception. That's why we need enumerator instead of using array directly.

@erikzhang
Copy link
Member

erikzhang commented Jun 14, 2020

Paging does not solve all problems. Because if the user requests the last page, you have to traverse all the items.

@joeqian10
Copy link
Contributor Author

@erikzhang @shargon @gsmachado @Tommo-L Got any other ideas? 🤔😂

@erikzhang
Copy link
Member

erikzhang commented Jun 17, 2020

Currently the only way I can think of is to modify the API of the contract so that the results returned by the contract support paging natively. But one problem I can't solve this way is that Storage.Find() itself cannot support paging. So the contract should also be powerless.

@erikzhang
Copy link
Member

So I don't think RPC should do the conversion anymore.

@shargon
Copy link
Member

shargon commented Jun 17, 2020

Maybe we should limit to the X first registers, and allow more by config. Then you can allow to your own node to return whole data

@Tommo-L
Copy link
Contributor

Tommo-L commented Jul 7, 2020

Maybe we should limit to the X first registers, and allow more by config. Then you can allow to your own node to return whole data

I think it's okay.

But one problem I can't solve this way is that Storage.Find() itself cannot support paging.

We can create a StorageOrderedMap, the key of map is prefix + number + key => value, when we want to delete one key:value, we just move the last element to the current position. But the implementation is a little complicated.

int low = page * ResultsPerPage; // 0, 50, 100...
int high = (page + 1) * ResultsPerPage - 1; // 49, 99, 149...
while (sysEnum.MoveNext() && index <= high)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxPage in config?

@erikzhang erikzhang closed this Apr 17, 2021
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.

RpcServer convert enumerator to array for rpcclient
6 participants