-
Notifications
You must be signed in to change notification settings - Fork 100
AWS KMS Support #64
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
AWS KMS Support #64
Conversation
We're building your pull request over on Zeet. |
We're building your pull request over on Zeet. |
core/sdk/sdk.ts
Outdated
|
||
// TODO: PLAT-982 | ||
// Currently we require WALLET_PRIVATE_KEY to be set in order to instantiate the SDK | ||
// But we need to implement wallet.generate() and wallet.save() to save the private key to file system | ||
|
||
// Need to make this instantiate SDK with read/write. For that will need wallet information | ||
const sdk = await ThirdwebSDK.fromWallet(wallet, chainName, { | ||
const sdk = await ThirdwebSDK.fromWallet(wallet!, chainName, { |
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.
we should throw if wallet is not initialized here, in general avoid using !
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 have this function in startup/index.ts
-> walletEnvVariablesCheck()
which checks for all the wallet env variables and throws an error if its missing & shuts down the server. But yeah, I can add a throw
here too.
Updated Readme too. |
for (let key in obj) { | ||
for (let str of obj[key]) { | ||
server.log.info( | ||
`\t Checking for ${key.toLowerCase()} -> ${str} in env`, |
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.
probably don't want to log the private key here
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.
This is just logging the ENV variables and not the value of it.
|
||
export const walletEnvVariablesCheck = async ( | ||
server: FastifyInstance, | ||
variables: { [key: string]: 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.
do we actually need this? feels much simpler to just do the checks in getSDK no?
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.
My goal was to not allow the server to start if the ENV variables required are missing. Otherwise, if I added it inside getSDK
then the error will only be thrown when and if user pings an end-point.
Added AWS KMS Support using ENV Variables. The implementation checks for
awsKms
Wallet ENV Variables andwalletPPK
. If neither present prints an Error and stops server.