-
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
Simplify heartbeat signature scheme #1537
Changes from all commits
2e18bfe
7fd06cf
c026aa1
24a028c
ed52de5
145a8ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@api3/airnode-node': minor | ||
--- | ||
|
||
Simplify heartbeat scheme |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import { ethers } from 'ethers'; | ||
import { execute } from '@api3/airnode-adapter'; | ||
import { logger, PendingLog } from '@api3/airnode-utilities'; | ||
import { go } from '@api3/promise-utils'; | ||
|
@@ -22,23 +21,10 @@ export function getHttpSignedDataGatewayUrl(config: Config) { | |
return getEnvValue('HTTP_SIGNED_DATA_GATEWAY_URL'); | ||
} | ||
|
||
export const signHeartbeat = ( | ||
heartbeatPayload: { | ||
cloud_provider: string; | ||
stage: string; | ||
region?: string; | ||
http_gateway_url?: string; | ||
httpSignedDataGatewayUrl?: string; | ||
}, | ||
timestamp: number | ||
) => { | ||
export const signHeartbeat = (heartbeatPayload: string) => { | ||
const airnodeWallet = getAirnodeWalletFromPrivateKey(); | ||
|
||
return airnodeWallet.signMessage( | ||
ethers.utils.arrayify( | ||
ethers.utils.solidityKeccak256(['uint256', 'string'], [timestamp, JSON.stringify(heartbeatPayload)]) | ||
) | ||
); | ||
return airnodeWallet.signMessage(heartbeatPayload); | ||
}; | ||
|
||
export async function reportHeartbeat(state: CoordinatorState): Promise<PendingLog[]> { | ||
|
@@ -56,16 +42,22 @@ export async function reportHeartbeat(state: CoordinatorState): Promise<PendingL | |
const httpGatewayUrl = getHttpGatewayUrl(config); | ||
const httpSignedDataGatewayUrl = getHttpSignedDataGatewayUrl(config); | ||
|
||
const heartbeatPayload = { | ||
const timestamp = Math.round(Date.now() / 1_000); | ||
|
||
/* | ||
The heartbeat payload is serialised as JSON and then serialised again in JSON as the value of the payload field. | ||
The reason this is done is to avoid any inconsistencies between different JSON serialisation implementations. | ||
*/ | ||
const heartbeatPayload = JSON.stringify({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Maybe it would be wise to make sure that the payload content fields are always sorted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we serialise it like this we will always get a consistent result. |
||
timestamp, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd suggest using the UNIX timestamp (in seconds) and not the JS timestamp which is in ms... But this is a breaking change so we might just not do it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Unix time would be much better imo. And just to confirm, is this in UTC or is it the server timezone? Why is it a breaking change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you rely on the heartbeat timestamp being in ms? (That's how it worked until now). Changing that to seconds would mean the old values would need to be migrated. EDIT: But tou will be implementing new handling for this anyway, so maybe it's not a big problem. But I am not sure if there aren't any other users of heartbeat that would be affected. (But they will be affected either way by the packing removal). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ChainAPI hasn't been using the timestamp until now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made the change by using |
||
stage, | ||
cloud_provider: cloudProvider.type, | ||
...(cloudProvider.type !== 'local' ? { region: cloudProvider.region } : {}), | ||
...(httpGatewayUrl ? { http_gateway_url: httpGatewayUrl } : {}), | ||
...(httpSignedDataGatewayUrl ? { http_signed_data_gateway_url: httpSignedDataGatewayUrl } : {}), | ||
}; | ||
const timestamp = Date.now(); | ||
}); | ||
|
||
const goSignHeartbeat = await go(() => signHeartbeat(heartbeatPayload, timestamp)); | ||
const goSignHeartbeat = await go(() => signHeartbeat(heartbeatPayload)); | ||
if (!goSignHeartbeat.success) { | ||
const log = logger.pend('ERROR', 'Failed to sign heartbeat', goSignHeartbeat.error); | ||
return [log]; | ||
|
@@ -78,9 +70,8 @@ export async function reportHeartbeat(state: CoordinatorState): Promise<PendingL | |
'airnode-heartbeat-api-key': apiKey, | ||
}, | ||
data: { | ||
...heartbeatPayload, | ||
payload: heartbeatPayload, | ||
signature: goSignHeartbeat.data, | ||
timestamp, | ||
}, | ||
timeout: 5_000, | ||
}; | ||
|
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's worth adding a comment explaining why this is stringified. i.e. to avoid any minor differences that might occur when encoding/decoding in other environments. The payload/message needs to be exactly the same everywhere
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've added the following comment, LMK if you disagree: