Add TurnkeySecp256k1 Signer #189
Conversation
|
There was a problem hiding this comment.
I'm not sure this should be a Signer impl we provide in our package, its much larger in scope then the other signers that complete almost immediately and just sign some data. This one can potentially block for a long time and there's no recovery mechanism if it's interrupted in some way
I feel like this would be better suited as a standalone script utility, thoughts?
| import { keccak_256 } from "@noble/hashes/sha3"; | ||
|
|
||
| // --- Turnkey Configuration --- | ||
| const ORG_ID = process.env.TURNKEY_ORG_ID; |
There was a problem hiding this comment.
Could we please move all the env vars to a configuration object instead. This way we don't control how and where these need to be configured we just receive them as parameters
Same as what the actual turnkey client does: https://github.com/Sovereign-Labs/sovereign-sdk-web3-js/pull/189/files#diff-79afd446c73470ec4aea9d8ad8aa8ba905213d5323795579e1c7ad5f78530665R49
|
|
||
| // Bind `this` to the methods to ensure the context is not lost | ||
| // when they are called by the Sovereign SDK. | ||
| this.sign = this.sign.bind(this); |
| } | ||
|
|
||
| public static async create(): Promise<TurnkeySecp256k1Signer> { | ||
| if (!/^[0-9a-fA-F]+$/.test(API_PRIVATE_KEY!)) { |
There was a problem hiding this comment.
Lets remove these formatting checks, we won't always know the shape of their API keys and their API will return a error if they're malformed
| }); | ||
|
|
||
| // Poll every 60s until COMPLETED | ||
| while (true) { |
There was a problem hiding this comment.
What happens if the program that's running this exits or crashes, etc? We currently have no way to resume a sign request or recover
| // Helper function to wait for a certain amount of time. | ||
| const wait = (ms: number) => new Promise(r => setTimeout(r, ms)); | ||
|
|
||
| export class TurnkeySecp256k1Signer implements Signer { |
There was a problem hiding this comment.
Is this in some way specific to secp256k1? It seems like we're call sign with a specific key id which could be any kind of key? If it's not specific lets just call it TurnkeySigner
package.json
Outdated
| "dependencies": { | ||
| "@changesets/cli": "^2.27.9" | ||
| "@changesets/cli": "^2.27.9", | ||
| "@turnkey/api-key-stamper": "^0.4.7", |
There was a problem hiding this comment.
Could you please move these new dependencies to packages/signers/package.json
| import { TurnkeyClient } from "@turnkey/http"; | ||
| import { ApiKeyStamper } from "@turnkey/api-key-stamper"; | ||
| import { ethers } from "ethers"; // v6 helpers | ||
| import 'dotenv/config'; |
There was a problem hiding this comment.
The env vars wont always be defined in a .env file, we shouldn't control loading env vars in library code
Description
Related Issues
Checklist