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

Implement timestamp post-processing #27

Closed
5 tasks done
bbenligiray opened this issue Nov 6, 2023 · 13 comments
Closed
5 tasks done

Implement timestamp post-processing #27

bbenligiray opened this issue Nov 6, 2023 · 13 comments
Assignees

Comments

@bbenligiray
Copy link
Member

bbenligiray commented Nov 6, 2023

Currently, Airnode uses the API call time as the timestamp of the signed data. This doesn't cover two use cases:

  • We want Airnode to make an API call to an RPC endpoint to get chain data. We want timestamp to be the most recent block timestamp. (This is what Nodary does for LSD feeds at the moment.)
  • The API that the Airnode call returns includes as timestamp field. We want timestamp to be that.

As a solution, I'm proposing a timestampPostProcessingSpecifications field at the same level as postProcessingSpecifications. This works exactly like postProcessingSpecifications, except output of that will be used as timestamp (rather than data).

This obviously requires a change in the OIS format. The field can be optional similar to postProcessingSpecifications, which makes this a non-breaking change.

EDIT: This requires changes in multiple projects.

@bbenligiray
Copy link
Member Author

@dcroote for visibility

@bbenligiray
Copy link
Member Author

The alternative way of doing this is piggybacking on postProcessingSpecifications. In addition to defining an output, postProcessingSpecifications can define a timestamp. This is somewhat breaking in terms of a potential naming collision.

@Siegrift
Copy link
Collaborator

Siegrift commented Nov 7, 2023

An alternative would be to create postProcessingSpecificationsV2 and change the format of the snippets. Couple of possibilities, some already mentioned on the call:

  • Make it return values instead of assigning to output field. The output schema of the function would be {value, timestamp}
  • Revisit how input data is passed. Currently input and endpointParameters and we risk a naming collision whenever we want to add new value. Making it inputData object helps with this because we can just add new keys to the object. This is still not bulletproof though.
  • We could make the specification not be an array (not sure if anyone used this featrue).
  • We could omit the environment and support just Node async. If the processing is a function that returns some value, we can wrap it in new Promise to force it to be async and await it.

@bbenligiray
Copy link
Member Author

We could make the specification not be an array (not sure if anyone used this featrue).

I don't see how this is an improvement

We could omit the environment and support just Node async

It would be nice to have a Node (restricted) environment in the future once that stuff is more fleshed out so I'm not a fan of removing that field. I'm fine with forcing async though.

@Siegrift
Copy link
Collaborator

Siegrift commented Nov 7, 2023

I don't see how this is an improvement

To me the array version feels like an unnecessary complexity. Both user has to think about why the specification is an array and we need to handle chaining the values through multiple processing snippets.

@bbenligiray
Copy link
Member Author

We ended up never using it (says something for modular design imo) but considering that it's already there and working, I don't see the reason to remove it

@Siegrift
Copy link
Collaborator

I think you misunderstood this:

An alternative would be to create postProcessingSpecificationsV2 and change the format of the snippets.

I mean to create a new field which implements couple of improvements + delivers this new feature. We would still support the "legacy" post-processing though. We would ensure that only one of postProcessingSpecifications and postProcessingSpecificationsV2 is specified using OIS validation.

@Siegrift Siegrift self-assigned this Nov 12, 2023
@bbenligiray
Copy link
Member Author

For reference I had agreed with postProcessingSpecificationsV2 and was nit-picking above

@Siegrift
Copy link
Collaborator

As a note, this requires docs changes as well.

@Siegrift
Copy link
Collaborator

I've updated a bunch of PRs (links in the editted issue description).

@metobom
Copy link
Member

metobom commented Nov 20, 2023

Will ChainAPI use this change too?

@bbenligiray
Copy link
Member Author

@Siegrift
Copy link
Collaborator

Siegrift commented Dec 1, 2023

All PRs have been implemented and all except Airnode (which is to be released) are released.

@Siegrift Siegrift closed this as completed Dec 1, 2023
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

No branches or pull requests

3 participants