-
Notifications
You must be signed in to change notification settings - Fork 194
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
refactor(common,cli): kms deployer gets keyId from environment #2760
Conversation
🦋 Changeset detectedLatest commit: 507f66a The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/cli/src/runDeploy.ts
Outdated
awsKmsKeyId: { | ||
type: "string", | ||
desc: "Optional AWS KMS key ID. If set, the World is deployed using a KMS signer instead of local private key.", | ||
aws: { |
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.
since we call everything else "kms" I wonder if this flag should be --kms
(I know this deviates from forge)
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.
agreed, I also like the specificity of KMS
Co-authored-by: Kevin Ingersoll <[email protected]>
@@ -19,9 +19,9 @@ export type KmsAccount = LocalAccount<"aws-kms"> & { | |||
* @returns A Local Account. | |||
*/ | |||
export async function kmsKeyToAccount({ | |||
keyId, | |||
keyId = process.env.AWS_KMS_KEY_ID || "", |
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.
Defaulting in the account was inspired by this Foundry library https://github.com/foundry-rs/foundry/blob/master/crates/wallets/src/wallet.rs#L95-L97
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.
why an empty string? is it bad if this is undefined? we already specify this as an optional param
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 did this because keyId
is optional for the SDK but our getKeyId
function returns a string
. Gonna try something else
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 a couple small things around messaging but otherwise lgtm!
@@ -25,7 +25,7 @@ export async function getAddressFromKms({ | |||
keyId, | |||
client, | |||
}: { | |||
keyId: SignCommandInput["KeyId"]; | |||
keyId: GetPublicKeyCommandInput["KeyId"]; |
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.
ooc what does this type resolve to?
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.
It's string | undefined
(same as SignCommandInput
actually). I don't know why they make it optional! https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-kms/Interface/GetPublicKeyCommandInput/
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.
oh, should we just make it a regular string and require it?
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 can, i figured we can pipe it through the same way they do. I copied that approach from the ethers signer https://github.com/rumblefishdev/eth-signer-kms/blob/master/src/kms.ts#L16
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.
also this change is totally orthogonal to the point of this PR, it's just a small consistency refactor I noticed and threw in
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 thought we had it as string
before, not sure when it changed
Co-authored-by: Kevin Ingersoll <[email protected]>
Co-authored-by: Kevin Ingersoll <[email protected]>
Co-authored-by: Kevin Ingersoll <[email protected]>
…cexyz#2760) Co-authored-by: Kevin Ingersoll <[email protected]>
We can declare the KMS Key ID
AWS_KMS_KEY_ID
instead of passing it into tomud deploy
. This means the flag to enable KMS signing can just be a boolean.