-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: generate keypair and add to .env if one doesn't exist #5
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.
Good PR, appending keypairs is awesome, but let's move it out of the get function to avoid possible surprises - see comments.
src/index.ts
Outdated
@@ -45,7 +46,13 @@ export const getKeypairFromFile = async (filepath?: string) => { | |||
export const getKeypairFromEnvironment = (variableName: string) => { | |||
const secretKeyString = process.env[variableName]; | |||
if (!secretKeyString) { | |||
throw new Error(`Please set '${variableName}' in environment.`); | |||
// Generate a new keypair if one doesn't exist and add it to a `.env` file |
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.
Having an easy way to save keypairs is good PR, but my concern with adding this to an existing function is that something called getKeypairFromEnvironment()
shouldn't have any side effects. Generally things called get()
should be fetching data, not changing it. Also I'm concerned if someone wants to get a keypair with a particular name, it doesn't exist, and they get it made - it's possible we could be silently throwing away errors. Maybe I have a keypair called TEMP_WALLET
and run getKeypairFromEnvironment('TEMPORARY_WALLET')
and what should have failed succeeds.
I'd called the code that appends keypairs something like appendKeypairToFile()
. Whatever you think is the most obvious for users.
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.
Just one minor bit we've already chatted about on Slack.
@@ -79,3 +73,17 @@ export const getKeypairFromEnvironment = (variableName: string) => { | |||
} | |||
return Keypair.fromSecretKey(decodedSecretKey); | |||
}; | |||
|
|||
export const addKeypairToEnvironment = (variableName: string) => { |
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'd expect something that adds a keypair to be addKeypairToEnvironment(keyPair: Keypair, name: string)
- I think we might have discussed this on Slack.
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.
Separately, as your comment already notes, this adds a keypair to the .env file, rather than the environment itself. The function name should reflect that, ie addKeypairToEnvFile()
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.
Ah it looks like you already updated this, just didn't reply! Woo I can merge in now.
const keypair = Keypair.generate(); | ||
appendFileSync( | ||
".env", | ||
`\n${variableName}=${JSON.stringify(Array.from(keypair.secretKey))}`, |
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.
Not super important, but this (JSON.stringify(Array.from(keypair.secretKey))
) is useful elsewhere so worth pulling out and exporting as keypairToJSON()
or similar.
Update
getKeypairFromEnvironment
to generate a new keypair and write it to a.env
file if one does not already exist with the providedvariableName
.