update the new auth method and trigger matcher array #56
Conversation
tests/templates.ts
Outdated
| @@ -19,6 +19,8 @@ import { NodeType } from "@avaprotocol/types"; | |||
| export const FACTORY_ADDRESS = "0x29adA1b5217242DEaBB142BC3b1bCfFdd56008e7"; | |||
| export const EXPIRED_AT = Math.floor(Date.now() / 1000) + 24 * 60 * 60; // 24 hours from now | |||
|
|
|||
| export const defaultTriggerId = `trigger${getNextId()}`; | |||
There was a problem hiding this comment.
Do we need a trigger prefix to the id of a trigger?
There was a problem hiding this comment.
no we dont. this is just test. let me know if you want to remvoe it. i put it to easily found in in console log when develop locally. so hard to know which is which with all the uuid. because now we use uuid all so hard to know whether a uuid belong to trigger or node.
There was a problem hiding this comment.
Right, we need to store that in a variable. The fix looks good.
| apiKey: string, | ||
| expiredAtEpoch: number, | ||
| ): Promise<KeyRequest> { | ||
| const chainId = CHAIN_ID; |
There was a problem hiding this comment.
I think the chainId needs to be passed in, but not a const.
There was a problem hiding this comment.
It's passed in in the actual function.
The place you're commention is a test utils function. and the CHAIN_ID const is pre-define in the same with CHAIN_ENDPOINT.
tests/utils.ts
Outdated
|
|
||
| return signature; | ||
| // Helper function to generate api key message | ||
| export async function generateAuthPayloadWithApiKey( |
There was a problem hiding this comment.
This function doesn’t need the async keyword.
There was a problem hiding this comment.
Is this func used only by the authWithAPIKey? I think it can be merged into the authWithAPIKey
There was a problem hiding this comment.
This function doesn’t need the async keyword.
Will remove. I fetch the Chain ID dynamically from the RPC before but it make the test a bit slower than normal so i revert to setting the Chain ID together with the chain endpoint
Is this func used only by the authWithAPIKey? I think it can be merged into the authWithAPIKey
hmm, this function is inside tests/utils.ts for testing purpose...
There was a problem hiding this comment.
Right, sorry I thought it was in source code. Then this is fine.
|
@chrisli30 please review again |
No description provided.