Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: AWS kms signer support for forc-client #6578

Merged
merged 21 commits into from
Sep 25, 2024
Merged

feat: AWS kms signer support for forc-client #6578

merged 21 commits into from
Sep 25, 2024

Conversation

kayagokalp
Copy link
Member

@kayagokalp kayagokalp commented Sep 22, 2024 •
edited
Loading

Description

closes #6560.
closes #6559.

This PR adds AWS kms signer support. Which is enabled if --aws-kms-signer <ARN> is present. If that is the case, an aws client is created which is used to create a AwsSigner.

With this PR, we are able to supply a signing arn and with that we can sign a transaction. This enables being able to revoke signing authority for aws kms managed keys. So we can control deployments and owner/target changes for a proxy contract. Effectively enabling multi-sig like proxy contracts via aws kms.

@kayagokalp kayagokalp requested review from a team as code owners September 22, 2024 10:11
@kayagokalp kayagokalp marked this pull request as draft September 22, 2024 10:11
@kayagokalp kayagokalp self-assigned this Sep 22, 2024
@kayagokalp kayagokalp added enhancement New feature or request forc-client Everything related to the `forc-client` crate. labels Sep 22, 2024
@kayagokalp kayagokalp mentioned this pull request Sep 23, 2024
Closed
@kayagokalp kayagokalp changed the title [WIP] feat: AWS kms signer support for forc-client. feat: AWS kms signer support for forc-client Sep 23, 2024
@kayagokalp kayagokalp marked this pull request as ready for review September 23, 2024 08:49
@kayagokalp kayagokalp requested review from alfiedotwtf and a team September 23, 2024 09:01
@kayagokalp
Copy link
Member Author

Looks like we hit a flaky test (hopefully, as it was not failing in previous commit and now it is failing): #5376 (comment)

alfiedotwtf
alfiedotwtf previously approved these changes Sep 23, 2024
View reviewed changes
@kayagokalp kayagokalp requested review from a team and alfiedotwtf September 23, 2024 23:53
@kayagokalp kayagokalp enabled auto-merge (squash) September 24, 2024 00:14
@JoshuaBatty JoshuaBatty mentioned this pull request Sep 24, 2024
Merged
JoshuaBatty
JoshuaBatty reviewed Sep 24, 2024
View reviewed changes
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kaya mostly looks good. Just wondering if you can first add doc comments to all public types and functions before we merge this.

@Voxelot
Copy link
Member

Voxelot commented Sep 24, 2024

Is this entirely additive or does it include breaking behavior?

@kayagokalp
Copy link
Member Author

Is this entirely additive or does it include breaking behavior?

This is entirely additive, forc-run changes just a refactor and does not change any behaviour.

kayagokalp
kayagokalp commented Sep 24, 2024
View reviewed changes
forc-plugins/forc-client/src/util/account.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/util/account.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/util/account.rs Outdated Show resolved Hide resolved
@kayagokalp kayagokalp requested review from JoshuaBatty and a team September 24, 2024 19:43
alfiedotwtf
alfiedotwtf previously requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@alfiedotwtf alfiedotwtf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from "is is", it's good to go

@kayagokalp kayagokalp requested review from alfiedotwtf and a team September 24, 2024 21:41
sdankel
sdankel approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

forc-plugins/forc-client/src/util/aws.rs Show resolved Hide resolved
forc-plugins/forc-client/src/util/aws.rs Show resolved Hide resolved
forc-plugins/forc-client/src/op/run/mod.rs Show resolved Hide resolved
forc-plugins/forc-client/src/util/tx.rs Show resolved Hide resolved
forc-plugins/forc-client/src/util/aws.rs Show resolved Hide resolved
JoshuaBatty
JoshuaBatty approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for now but it would be good to address Sophie's feedback in a follow up PR.

@kayagokalp kayagokalp merged commit d2b5431 into master Sep 25, 2024
39 checks passed
@kayagokalp kayagokalp deleted the kayagokalp/6560 branch September 25, 2024 00:28
IGI-111 pushed a commit that referenced this pull request Sep 25, 2024
@JoshuaBatty
chore: bump to v0.64.0 (#6584)
7ee092d 
## Description

~~waiting on #6578~~

This release contains no API changes and along with bug fixes, upgrades
support for fuel-core to 0.36 and fuels SDK to 0.66.5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaBatty JoshuaBatty JoshuaBatty approved these changes

@sdankel sdankel sdankel approved these changes

@alfiedotwtf alfiedotwtf Awaiting requested review from alfiedotwtf

Assignees

@kayagokalp kayagokalp

Labels
enhancement New feature or request forc-client Everything related to the `forc-client` crate.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

forc-client should support signing transactions using a KMS forc-client should support alternative signers
5 participants
@kayagokalp @Voxelot @alfiedotwtf @JoshuaBatty @sdankel