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

Change query input enconding #1

Open
relu91 opened this issue Aug 16, 2022 · 20 comments
Open

Change query input enconding #1

relu91 opened this issue Aug 16, 2022 · 20 comments
Labels
effort: hours good first issue 👍 Good for newcomers priority: medium (3) Medium-priority issue that needs to be resolved type: enhancement New feature or request

Comments

@relu91
Copy link
Member

relu91 commented Aug 16, 2022

Encoding the query JSON with __!_ and __#_ is not valid for production. We should investigate how to correctly escape " and ' characters and if it is not possible to escape it would be probably better to just use an URL encoder (ex: https://meyerweb.com/eric/tools/dencoder/ ) as an encoding algorithm.

@FerrariAndrea FerrariAndrea linked a pull request Aug 22, 2022 that will close this issue
@relu91
Copy link
Member Author

relu91 commented Aug 23, 2022

I saw PR #6 but I still wonder if we need all of this, have you tested different running conditions and understood why the " or ' characters are missing? I need a clear answer to this question before choosing to use one enc/dec algorithm.

@relu91 relu91 added type: enhancement New feature or request priority: medium (3) Medium-priority issue that needs to be resolved good first issue 👍 Good for newcomers effort: hours labels Aug 23, 2022
@FerrariAndrea
Copy link
Contributor

FerrariAndrea commented Aug 24, 2022

In order to understand why the " or the ' is missing, we need to test the DApp from a real call (not from a local run), so with an app order, and the execution of the DApp in the workerpoll.
Because this issue shows up if the DApp is called from a workerpool.
I will try to do some tests with a new DApp that will just check the args (using the app order and the double/single quotes).

@relu91
Copy link
Member Author

relu91 commented Aug 24, 2022

I will try to do some tests with a new DApp that will just check the args (using the app order and the double/single quotes).

Yes exactly, maybe you can leverage on our logging system to know more.

@FerrariAndrea
Copy link
Contributor

I'm doing some experiments, for now, I don't have good news.
Tthe Iexec application that I'm using, just take and print the args, passed by the workerpool.

Starting from the run on my local machine with that 3 args:

 "This is a 'string' with ' ( 3 single quotes)" "this is a \"string\" with \" (double quotes)" ` double " and ' single quote string ` 

The output on mu cmd is:

[Wed Aug 24 2022 11:09:48 GMT+0000 (Coordinated Universal Time)][APP.test_param_0]: This is a 'string' with ' ( 3 single quotes)
[Wed Aug 24 2022 11:09:48 GMT+0000 (Coordinated Universal Time)][APP.test_param_1]: this is a "string" with " (double quotes)
[Wed Aug 24 2022 11:09:48 GMT+0000 (Coordinated Universal Time)][APP.test_param_2]: `

From the online logger we can see that as args array:

[Wed Aug 24 2022 11:09:48 GMT+0000 (Coordinated Universal Time)][APP]: [\"/usr/local/bin/ts-node\",\"/app/app.ts\",\"This is a 'string' with ' ( 3 single quotes)\",\"this is a \\\"string\\\" with \\\" (double quotes)\",\"`\",\"double\",\" and ' single quote string `\"]

Conclusion the "`" can't be used as a delimiter (like I did in this example) for the args, the single and the double quotes are working correctly.

About the workerpool execution, I can't execute the DApp in a workepool from the app-order (yet), but I can run it in a workerpool using the run command. I found some problems here.

Using simpler args in order to understand the problem, in that case, we will pass 3 arguments that are strings with spaces, so we need to use double quotes as delimiters for the args.
The argument of the command that will contain the args, must be a json from the Iexec documentation:
--params <json> # specify the params of the request, this option is reserved to an advanced usage (usage: --params '{"iexec_args":"dostuff","iexec_input_files":["https://example.com/file.zip"]}')

So the correct command should be like:
iexec app run --watch --chain viviani --trust 0 --params '{"iexec_args":"\"This is a string\" \"This is another string\" \"This is another string\""}'
This not work:
Command "iexec app run" failed with ValidationError: '{iexec_args:"This is a string" "This is another string" "This is another string"}' is not a valid params, must be a json

We can see that the JSON string removes the first level of double quotes, so I tried with:
iexec app run --watch --chain viviani --trust 0 --params '{\"iexec_args\":\"\\\"This is a string\\\" \\\"This is another string\\\" \\\"This is another string\\\"\"}'
And the result is:
Command "iexec app run" failed with ValidationError: '{"iexec_args":"\"This is not a valid params, must be a json
We can see, the double quotes are correct, but for some reason, Iexec cant handle that JSON as a valid one.

@relu91
Copy link
Member Author

relu91 commented Aug 25, 2022

Conclusion the "`" can't be used as a delimiter (like I did in this example) for the args, the single and the double quotes are working correctly.

Yes ` is a well know delimiter for bash and it is used to call subshell. Not sure if you use PowerShell for your experiments but I guess it is used there similarly.

Using simpler args in order to understand the problem, in that case, we will pass 3 arguments that are strings with spaces, so we need to use double quotes as delimiters for the args.
The argument of the command that will contain the args, must be a json from the Iexec documentation:
--params # specify the params of the request, this option is reserved to an advanced usage (usage: --params '{"iexec_args":"dostuff","iexec_input_files":["https://example.com/file.zip"]}')

So the correct command should be like:
iexec app run --watch --chain viviani --trust 0 --params '{"iexec_args":""This is a string" "This is another string" "This is another string""}'
This not work:
Command "iexec app run" failed with ValidationError: '{iexec_args:"This is a string" "This is another string" "This is another string"}' is not a valid params, must be a json

We can see that the JSON string removes the first level of double quotes, so I tried with:
iexec app run --watch --chain viviani --trust 0 --params '{"iexec_args":"\"This is a string\" \"This is another string\" \"This is another string\""}'
And the result is:
Command "iexec app run" failed with ValidationError: '{"iexec_args":""This is not a valid params, must be a json
We can see, the double quotes are correct, but for some reason, Iexec cant handle that JSON as a valid one

Ok, something is weird, but why don't you use --args?

--args # specify the arguments to pass to the app

I would try this:

iexec app run --watch --chain viviani --trust 0 --args '"test first string" "test second string" "test" '

@FerrariAndrea
Copy link
Contributor

FerrariAndrea commented Aug 26, 2022

Yea, this worked, you can see here the transaction with that params: Iexec explorer deal

I tested that too:
iexec app run --watch --chain viviani --trust 0 --args '"test 01: \\\" TEST" "test 02 \" TEST" "test 03 \"T\" TEST"'
And it worked: Iexec explorer deal

Now we are pretty sure that, the problem is the --param, --args seems to work.
We need to ensure how the DApp is run in the DESMO-LD demo (is the contract that runs it, right?).

@FerrariAndrea FerrariAndrea pinned this issue Aug 26, 2022
@relu91
Copy link
Member Author

relu91 commented Aug 26, 2022

Good news, can you try passing the JSON serialization of the query in this testing app? Perhaps it would be nice to add a simple JSON.parse call to get more information about what is going on.

We need to ensure how the DApp is run in the DESMO-LD demo (is the contract that runs it, right?).

After that test, we can move on with DESMO-LD. It is the SDK that is calling the execution.

@relu91
Copy link
Member Author

relu91 commented Aug 26, 2022

Check also this StackOverflow answer about how to escape ' in bash.

@FerrariAndrea
Copy link
Contributor

Confirmed it work with --args:
iexec app run --watch --chain viviani --trust 0 --args '"test 01: '"'"' TEST" "test 02 \" TEST" "test 03 '"'"'T'"'"' TEST"'
Iexec explorer deal

@relu91
Copy link
Member Author

relu91 commented Aug 26, 2022

Good news, can you try passing the JSON serialization of the query in this testing app? Perhaps it would be nice to add a simple JSON.parse call to get more information about what is going on.°

What about this?

@FerrariAndrea
Copy link
Contributor

FerrariAndrea commented Aug 26, 2022

Sorry, I missed that comment.
That is more complex to do... to simplify the new desmo-dapp_test_param will require just on param and will use JSON.parse on that.
First onchain run test:
JSON.stringify({p1:"this is a string",p2:"this is a string with \" double quote",p3:55}):{"p1":"this is a string","p2":"this is a string with \\" double quote","p3":55}

The problem maybe is that JSON.stringify is converting \" as \\" instead of \\\". (Edit: that is correct, makes sense, because it is not escaping the \ but just the ")

We need to escape all the special chars inside:
"{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\\\" double quote\",\"p3\":55}"
The run cmd will be: iexec app run --watch --chain viviani --trust 0 --args "{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\\\" double quote\",\"p3\":55}"
Iexec explorer deal

Error: SyntaxError: Unexpected token d in JSON at position 57

After that I tried that:
iexec app run --watch --chain viviani --trust 0 --args "{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\" double quote\",\"p3\":55}"
Iexec explorer deal
Logger args printed:
[APP]: [\"/usr/local/bin/ts-node\",\"/app/app.ts\",\"{\\\"p1\\\":\\\"this is a string\\\",\\\"p2\\\":\\\"this is a string with \\\\\\\" double quote\\\",\\\"p3\\\":55}\"]"
That seems to work, the output of the JSON.parse is: [object Object]
(the current app can't print it in the logger, but it should be correct)

@FerrariAndrea
Copy link
Contributor

The new version of the desmo-dapp_test_param, same command of the last test:
iexec app run --watch --chain viviani --trust 0 --args "{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\" double quote\",\"p3\":55}"
Iexec explorer deal
Logger output:

    "id": "Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)",
    "requestID": "NOT SETTED REQEUSTID",
    "log": "[Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)][APP]: [\"/usr/local/bin/ts-node\",\"/app/app.ts\",\"{\\\"p1\\\":\\\"this is a string\\\",\\\"p2\\\":\\\"this is a string with \\\\\\\" double quote\\\",\\\"p3\\\":55}\"]",
    "partial": true
  },
  {
    "id": "Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)",
    "requestID": "NOT SETTED REQEUSTID",
    "log": "[Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)][APP.JSON.parse(test_param_0)[p2]]: this is a string with \" double quote",
    "partial": true
  },
  {
    "id": "Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)",
    "requestID": "NOT SETTED REQEUSTID",
    "log": "[Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)][APP]: DApp started!",
    "partial": true
  },
  {
    "id": "Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)",
    "requestID": "NOT SETTED REQEUSTID",
    "log": "[Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)][APP.JSON.parse(test_param_0)[p3]]: 55",
    "partial": true
  },
  {
    "id": "Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)",
    "requestID": "NOT SETTED REQEUSTID",
    "log": "[Fri Aug 26 2022 13:12:47 GMT+0000 (Coordinated Universal Time)][APP.JSON.parse(test_param_0)[p1]]: this is a string",
    "partial": true
  }

It works!!!

@relu91
Copy link
Member Author

relu91 commented Aug 26, 2022

The new version of the desmo-dapp_test_param, same command of the last test:

iexec app run --watch --chain viviani --trust 0 --args "{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\" double quote\",\"p3\":55}"

Why don't you use
iexec app run --watch --chain viviani --trust 0 --args '{"p1":"this is a string","p2":"this is a string with \\\" double quote","p3":55}' ? it should be easier cause it requires fewer escapes right?

@FerrariAndrea
Copy link
Contributor

That does not work:
iexec app run --watch --chain viviani --trust 0 --args '{"p1":"this is a string","p2":"this is a string with \\\" double quote","p3":55}'
Iexec explorer deal

I think this is cause of IExec CLI.

I tried with iexec app run --watch --chain viviani --trust 0 --args '{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\" double quote\",\"p3\":55}' too and that goes in error before the lunch of the DApp with:
× Command "iexec app run" failed with ValidationError: is is not a valid ethereum address

Is possible that the ' are ignored by the command.

At the end I tested that: iexec app run --watch --chain viviani --trust 0 --args '"{\"p1\":\"this is a string\",\"p2\":\"this is a string with \\\" double quote\",\"p3\":55}"'
Iexec explorer deal
Obviously, this doesn't work, but you can see the ' inside the " so the order is inverted, this is why the external " is put by Iexec as always, and the internal ' is our external one.

In my opinion that is a little bit strange, but it seems that: if the single quote is internal to a double quote it will not be removed by the iexec command, but if you use the single quote as a string delimiter for the Args that will not work, it will be removed.

@relu91
Copy link
Member Author

relu91 commented Sep 19, 2022

@FerrariAndrea regarding this issue we would like to know the max string length that we can encode in the iexec args. Can you give us a number?

@relu91
Copy link
Member Author

relu91 commented Sep 19, 2022

@FerrariAndrea regarding this issue we would like to know the max string length that we can encode in the iexec args. Can you give us a number?

Also seems that somebody didn't get through enough in the iexec documentation. We can use input-files as described in the link above. @FerrariAndrea can you do some preliminary tests and see how this feature work in iexec? The file has to be published on ipfs, can you try to do that, too?

@FerrariAndrea
Copy link
Contributor

Yea I know about the Input-files... but I thought that was not the best solution for us, we do not have files. Of course, we can use a file in order to pass the JSON query, but just because the query passed by args is not working well.

I will have a look

@FerrariAndrea
Copy link
Contributor

I don't find any on the documentation about ARGs size, I think it depends on the OS that is running on the worker pool.

I tested a long query string with 2187 chars and it works, you can see that on the explorer: https://explorer.iex.ec/viviani/deal/0xfe3cc81f895615ae8dcf42752a5ad16c9afbdc70ea353081783c90761b77782a

Using the input file method instead pass the query by args can be a problem for our project structure.
How we will pass the file?
We need to expose them on a web server and give the DAPP the URL of the file, I think in this way the query (that is in the file) will not save on the chain. Using Args, the args are written on a chain if I'm not wrong (in the app order) using files, only the URL will be saved in the app order).

@relu91
Copy link
Member Author

relu91 commented Sep 20, 2022

I tested a long query string with 2187 chars and it works, you can see that on the explorer:

Can you do more tests with longer strings?

Using the input file method instead pass the query by args can be a problem for our project structure.
How we will pass the file?
We need to expose them on a web server and give the DAPP the URL of the file, I think in this way the query (that is in the file) will not save on the chain. Using Args, the args are written on a chain if I'm not wrong (in the app order) using files, only the URL will be saved in the app order).

Sometimes I wonder if you really read the comments.... :

The file has to be published on ipfs, can you try to do that, too?

Read the ipfs documentation and how it works you'll see that ipfs is a distributed storage for files and it is pretty common in the Web3 world as an immutable record of files. We already have considered the pros and cons of this solution and it is not that bad. I just want to know if it works.

@FerrariAndrea
Copy link
Contributor

Considering only the length of the query, in the args, we have the requestID in addition.

Query length 13542 chars:
× Command "iexec app run" failed with Web3ProviderSendError: processing response error: Transaction cost exceeds current gas limit. Limit: 6700000, got: 10464190. Try decreasing supplied gas

Query length 11082 chars:
× Command "iexec app run" failed with Web3ProviderSendError: processing response error: Transaction cost exceeds current gas limit. Limit: 6700000, got: 8682430. Try decreasing supplied gas.

Query length 9277 chars:
× Command "iexec app run" failed with Web3ProviderSendError: processing response error: Transaction cost exceeds current gas limit. Limit: 6700000, got: 7363783. Try decreasing supplied gas.

Query length 5475 chars, it worked

Query length 8250 chars, it worked


6700000:10464190=x:13542 -> x=8.670
6700000:8682430=x:11082 -> x=8.551
6700000:7363783=x:9277 -> x=8.440
the limit should be less than: 8.440 chars for a query

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: hours good first issue 👍 Good for newcomers priority: medium (3) Medium-priority issue that needs to be resolved type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants