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

feat(node): expose endpointParameters to all pre/post-processing steps #1831

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Jul 10, 2023

Closes #1673.

I implemented this by passing endpointParameters to the VM context in unsafeEvaluate and unsafeEvaluateAsync for both pre- and post-processing. This allows the endpointParameters to be immutable with respect to the parameters of the aggregatedApiCall (more relevant to pre-processing).

Note to achieve the above immutability and to avoid breaking existing post-processing code, I did not modify the input (the raw API call response data) of the reduce of postProcessingSpecifications.

@dcroote dcroote self-assigned this Jul 10, 2023
@dcroote dcroote requested a review from bbenligiray July 10, 2023 06:13
@bbenligiray
Copy link
Member

The description looks good, I'll review as soon as possible

Copy link
Member

@bbenligiray bbenligiray left a comment

Choose a reason for hiding this comment

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

Requested some minor changes, didn't test it yet

packages/airnode-node/src/api/processing.ts Outdated Show resolved Hide resolved
// provide endpoint parameters immutably
let endpointParameters = JSON.parse(JSON.stringify(payload.aggregatedApiCall.parameters));

// The HTTP gateway is a special case for ChainAPI that is not allowed to access reserved parameters
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I remember writing about this but I couldn't find where, but it may have been vague.

  • We want both pre-processing and post-processing to not have access to the reserved parameters (this seems to only do it for post-processing)
  • This should apply to all cases, and not only calls coming through the gateway. The reasoning is that breaking some old endpoints that use reserved parameters in processing (which to my knowledge don't exist) is preferable to endpoints potentially behaving differently when used in different ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want both pre-processing and post-processing to not have access to the reserved parameters

OK, I've removed access to reserved parameters from pre- and post-processing.

This should apply to all cases, and not only calls coming through the gateway.

The title and description of #1738 was specific for the gateway (because of ChainAPI test calls) and so that is what I addressed in #1794. But easy enough and makes sense. I've made the change.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I now realize that that is about an older PR. @andreogle for visibility I guess 😅 The fix looks good, thanks.

- make endpointParameters immutable between processing steps

- remove access to reserved parameters from pre/post-processing
regardless of API call type
@bbenligiray bbenligiray self-requested a review July 19, 2023 14:53
Copy link
Member

@bbenligiray bbenligiray left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. I can test both #1738 and #1673 once merged. It would be nice to release 0.12 before the end of July.

@dcroote dcroote merged commit 459f09f into master Jul 19, 2023
@dcroote dcroote deleted the dcroote/issue1673 branch July 19, 2023 15:34
@dcroote
Copy link
Contributor Author

dcroote commented Jul 20, 2023

Thanks, looks good. I can test both #1738 and #1673 once merged. It would be nice to release 0.12 before the end of July.

Thanks and releasing v0.12 by end of July should be no problem- the only issue left for the v0.12 milestone is #1692 for which I am finishing up a PR (#1836).

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.

Add request parameters to postProcessingSpecifications's input
2 participants