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

Add config.json based pre and post-processing #1008

Merged
merged 18 commits into from
Apr 29, 2022

Conversation

aquarat
Copy link
Contributor

@aquarat aquarat commented Apr 12, 2022

This PR adds relatively simple pre and post-processing to Airnode.
This was originally in support of a specific provider, but I think it's usable in Airnode master and saves us from having more custom Airnodes.

Processing code is supplied per endpoint in the OIS in config.json, in a schema based on this documentation

This is discount airnode-plug:

  • Processing code is supplied as an array of chunks
  • The array is run through reduce, which executes each chunk in sequence.
  • A chunk can expect to have access to input (which is the result of the previous chunk's execution or the initial value)
  • Each chunk is expected to produce an output variable. We append an ending statement which returns output the calling code.
  • The reduce is called inside go and limited to 500ms gross timeout (perhaps this is too small or maybe this should be in constants.ts

Processing code is not supplied on-chain.
The idea is to support other environments (like WASM) at a later stage.

The idea is that this enables a bunch of high value use cases for limited development time. Security mitigations for processing code are not required because we can reasonably expect the API provider to not hack themselves.

Post processing processes the API response prior to extraction and encoding.
Pre processing processes the parameters used for an API call.

An example from a testing config.json endpoint:

           [
            {
              "operationParameter": {
                "in": "query",
                "name": "sparkline"
              },
              "value": "false"
            }
          ],
          "preProcessingSpecifications": [
            {
              "environment": "Node 14",
              "value": "const output = {...input, coinId: 'nope-bitcoin'};"
            },
            {
              "environment": "Node 14",
              "value": "const output = {...input, coinId: input.coinId.substring(5)};"
            }
          ],
          "postProcessingSpecifications": [
            {
              "environment": "Node 14",
              "value": "const output = parseInt(input.market_data.current_price.usd)*10000;"
            },
            {
              "environment": "Node 14",
              "value": "const output = input * require('crypto').randomInt(100);"
            }
          ],
          "reservedParameters": [
            {}]

Further context, these are discussions between Burak and I on this topic:

  • No parameters, we can add them later.
  • The array implies that these can be chained.
  • We specify the environment, meaning that this can do WASM, different runtime versions of Node, something entirely different, etc. We can require environment to be Node 14 (or whatever we use with Airnode) for now
  • There is an argument to be made for inputs where inputs[0] is the output of the previous step and inputs[1] is the initial API request parameters for preprocessing. However, I don't really see why we would need inputs[1] (aquarat says: stack)

@aquarat aquarat marked this pull request as ready for review April 12, 2022 16:22
@aquarat aquarat self-assigned this Apr 12, 2022
Copy link
Contributor

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. But I have some questions.

Do we want to land this to master now? Shall it be released as part of 0.6 or we should first release 0.6 and then add this as part of 0.7?

packages/airnode-node/src/api/index.test.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/index.test.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/index.ts Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Outdated Show resolved Hide resolved
packages/airnode-node/src/api/processing.ts Outdated Show resolved Hide resolved
@amarthadan
Copy link
Contributor

Overall looks good to me. But I have some questions.

Do we want to land this to master now? Shall it be released as part of 0.6 or we should first release 0.6 and then add this as part of 0.7?

If possible we should wait till 0.6 is released (shouldn't be long now, everything needed is merged it just needs to be tested). Worst case we should merge it to 0.7 branch and wait for the 0.6 release before merging is to master. But if think keeping it open is fine in this situation.

@Siegrift Siegrift force-pushed the add-pre-and-post-processing branch from 9d32273 to e133372 Compare April 15, 2022 08:55
Copy link
Contributor

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

@aquarat I wanted to do sooner, but yesteday got unexpectedly busy for me so I found some more time today. Please take a look.

I will be heading off to vacation shortly, but will be able to take a look on Sunday :D

Copy link
Contributor

@Ashar2shahid Ashar2shahid left a comment

Choose a reason for hiding this comment

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

Looks good, just a note, when looking up eval I came across Function which seems to be the preferred over eval due to the scope eval is invoked in . Here is the same code using Function someone else can have a look

function execute(_input: any, code: string) {
  // this is the recommended way of executing code with Function
  return Function(`
 "use strict";
  const ethers = require('ethers');
  const {BigNumber} = ethers;
  const input = _input;

  ${code};

  return output;
  `)(_input);
}

@aquarat
Copy link
Contributor Author

aquarat commented Apr 15, 2022

Function

Interesting, I thought that Function and eval were equivalent, I didn't realise they had scope differences.
We're trying to run trusted pre and post-processing code and I think there are scenarios where the provider would want the code to be able to access parent-scoped objects. The plan is to eventually support multiple execution environments (with different security privileges), but this is a long term low priority goal. We want basic processing soon so we can reduce the usage of custom Airnodes.

I think for now this is fine, but thanks for pointing out Function's difference 😄 💪

@aquarat aquarat requested a review from Siegrift April 15, 2022 12:20
@aquarat
Copy link
Contributor Author

aquarat commented Apr 15, 2022

@Siegrift Thanks for introducing these changes. It's perfect for now IMO.

@Siegrift
Copy link
Contributor

I agree with @Ashar2shahid suggestion. I do not see any reason why the postprocessing should have access to Airnode internals that we can are to change freely. That code should only care about input and ethers which we are supplying.

One thing, shall we expose BigNumber from ethers directly?

return Function(`
"use strict";
const [ethers, input] = arguments
const {BigNumber} = ethers;
Copy link
Contributor

Choose a reason for hiding this comment

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

@aquarat shall we have this line here? I am not sure why only expose BigNumber drectly and not other stuff.

Copy link
Contributor Author

@aquarat aquarat Apr 17, 2022

Choose a reason for hiding this comment

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

We can remove it, no problem :)
(To avoid ambiguity, the developer can be expected to import everything they need)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...that was fast @ commit :P

@Ashar2shahid
Copy link
Contributor

Ashar2shahid commented Apr 16, 2022

Yes the more I think about it , we should definitely use Function instead, say the provider sets an err variable in their code, this would now either throw or give an exception at const[err,processedParameters] whereas it shouldn't have or maybe it should......... 🤔. Well either way I think Function is the safer alternative.

@aquarat
Copy link
Contributor Author

aquarat commented Apr 17, 2022

Yes the more I think about it , we should definitely use Function instead, say the provider sets an err variable in their code, this would now either throw or give an exception at const[err,processedParameters] whereas it shouldn't have or maybe it should......... thinking. Well either way I think Function is the safer alternative.

All good, Function can be used instead. I agree that it is a cleaner alternative and it shouldn't impact the immediate use case for this functionality. This functionality is a stop-gap to enable use cases that will take some time for us to support. The whole thing is expected to be advanced in nature and "here be dragons".

Copy link
Contributor

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

I think this is good to go, but since I was the co-author I think someone else should give an approve as well.

Maybe @Ashar2shahid or @amarthadan

Copy link
Contributor

@Ashar2shahid Ashar2shahid left a comment

Choose a reason for hiding this comment

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

Yeah looks good to me

@aquarat
Copy link
Contributor Author

aquarat commented Apr 20, 2022

I just tried using this for a real task and I needed to import various libraries. Unfortunately neither import nor require work and this meant I had to customise processing to switch it back to eval.

I could have gotten around the limitation by inserting a chunk of the of the libraries I wanted (lodash), but that just seems unfortunate and unnecessarily inefficient. I think unsafeEvaluate should be exactly that, unsafe. The only variables eval could accidentally clobber in this implementation are input and code. The code is sourced from the trusted config.json and one has to write this code with the context of what it is processing; this isn't the realm of the ignorant. This is not your normal night of theatre.

If we really must have Function then it should be a different procession option imo, like Node 14 Function vs Node 14 eval.

@Siegrift
Copy link
Contributor

Ok, I'll revert it back to use eval but you should be careful about importing libraries as they might change or get removed from Airnode. But I understand that you need to do this for your use case.

@aquarat
Copy link
Contributor Author

aquarat commented Apr 20, 2022

Sorry for being difficult and thank you :)

I'll make a few notes for when we document this (initially and for some time to come for our internal use).

@aquarat
Copy link
Contributor Author

aquarat commented Apr 21, 2022

Just an additional note; I'm actively using this code and I've discovered a few interesting quirks along the way that we may want to address prior to merging/don't merge until I've documented these cases.

As a taster, I think we might want this instead of eval: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncFunction
and https://nodejs.org/api/vm.html#vmruninnewcontextcode-contextobject-options

@aquarat
Copy link
Contributor Author

aquarat commented Apr 22, 2022

It's probably best if I package up my changes and base them on this branch, opening a separate PR.

@aquarat
Copy link
Contributor Author

aquarat commented Apr 27, 2022

Looks like something was introduced that broke one of the tests. I'll check it out tomorrow.

@aquarat
Copy link
Contributor Author

aquarat commented Apr 29, 2022

I am working on this now fyi

@aquarat
Copy link
Contributor Author

aquarat commented Apr 29, 2022

Tests are now passing locally, now just waiting for CI.

@aquarat aquarat merged commit fe86308 into master Apr 29, 2022
@aquarat aquarat deleted the add-pre-and-post-processing branch April 29, 2022 07:41
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.

4 participants