-
Notifications
You must be signed in to change notification settings - Fork 72
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
OEV GW timestamp calculation fix #1676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -80,7 +86,9 @@ export async function signOevData( | |||
null, | |||
{ | |||
success: true, | |||
data: signatures.map((signature) => ({ signature, timestamp, encodedValue: encodedUpdateValue })), | |||
data: signatures.map((signature) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it would be better to return an array of string | null
for every beacon. (No need to return object with one field). This makes makes in general more sense to me and will also simplify the implementation on the oev-relay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that returning a one-field object doesn't make much sense but I don't agree with returning null
s (as commented https://github.com/api3dao/oev-relay/pull/404/files#r1140277606).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is more explicit which beacons were actually signed (that why I said I believe it's better in general).
For example, say the gateway receives beacons data A1, B1, A2, A3, B2 where A and B are Airnodes. If we later change the algorithm in Airnode to support not signing a particular beacon (e.g. only sign A1 and A3). But I agree with your comment, that it's mostly a cosmetic change so I am OK with leaving this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit related to this... Currently, the gateway accepts an array of beacons, but that means you can call it with duplicates of a single beacon. This won't cause Airnode to misreport (or mis-sign) but it's wierd. I think it would make the most sense to accept a record {[beaconId]: BeaconData}
and make the response be {[beaconId]: signature}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a fair point. Ok, let's do it this way then. I'll merge this PR and you can make changes in #1681.
0175036
to
8a302ae
Compare
No description provided.