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.

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

Alloy signing integration #970

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Alloy signing integration #970

wants to merge 21 commits into from

Conversation

musitdev
Copy link
Contributor

@musitdev musitdev commented Dec 19, 2024

Summary

  • RFCs: Link to RFC, Link to RFC, or $\emptyset$.
  • Categories: any of protocol-units, networks, scripts, util, cicd, or misc.

First version of Alloy crate integration for signing with the API. Inspired from the Alloy AWS implementation alloy_signer_aws::AwsSigner

Changelog

Testing

Provide an integration test to test AWS KMS integration. The test can't be used in CI because it needs AWS KMS auth env variable to set to run (AWS_ACCESS_KEY and AWS_SECRET_KEY).
The test start a local Anvil eth node and execute a transfer Tx using an AWS KMS key defined with the AWS_KEY_ID env var.

AWS_KEY_ID=<>AW key id> AWS_ACCESS_KEY=<access key> AWS_SECRET_KEY=<secret key> cargo test -p movement-signing-alloy test_aws_kms_send_tx -- --nocapture

Outstanding issues

}

#[async_trait::async_trait]
impl alloy_network::TxSigner<AlloySignature> for HsmSigner {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you already worked on the code for how this gets integrated? I think it's still probably easier for us to change the usage s.t. we are producing a raw transaction and signing that rather than tightly coupling with a target API.

/// Instantiate a new signer from an existing `Client` and key ID.
///
/// Retrieves the public key from HMS and calculates the Ethereum address.
pub async fn new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this returns a result, I would call it try_new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@musitdev musitdev changed the base branch from l-monninger/signing-api to main December 24, 2024 10:19
@0xmovses 0xmovses mentioned this pull request Jan 6, 2025
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants