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

feat(alloy-genesis): pk support #171

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Conversation

Evalir
Copy link
Contributor

@Evalir Evalir commented Feb 1, 2024

Motivation

missing pk support.

Solution

Adds missing private key support to alloy genesis, see foundry-rs/foundry#6970 (review).

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Evalir Evalir force-pushed the evalir/add-genesis-pk-support branch from cf1a1a8 to 01d3522 Compare February 1, 2024 16:49
@Evalir Evalir marked this pull request as draft February 1, 2024 16:50
@@ -14,6 +14,8 @@ exclude.workspace = true
[dependencies]
alloy-primitives.workspace = true
alloy-rpc-types.workspace = true
alloy-signer.workspace = true
k256.workspace = true
Copy link
Member

@DaniPopes DaniPopes Feb 1, 2024

Choose a reason for hiding this comment

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

we should re-export k256 from alloy-signer actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k256 u mean right?

skip_serializing_if = "Option::is_none",
with = "secret_key"
)]
pub private_key: Option<LocalWallet>,
Copy link
Member

Choose a reason for hiding this comment

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

Can we just do B256?

Copy link
Contributor Author

@Evalir Evalir Feb 1, 2024

Choose a reason for hiding this comment

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

yeah would prefer that actually. wdyt @mattsse

Copy link
Member

Choose a reason for hiding this comment

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

@Evalir I'd actually prefer just B256 as well.

point in my review still stands, we should have functions on LocalWallet that take care of that

crates/genesis/src/lib.rs Outdated Show resolved Hide resolved
pub mod secret_key {
use alloy_primitives::Bytes;
use alloy_signer::LocalWallet;
use k256::{ecdsa::SigningKey, SecretKey};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be required here,
the LocalWallet type should have all the necessary functions, if not we should add

Copy link
Member

@DaniPopes DaniPopes Feb 1, 2024

Choose a reason for hiding this comment

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

Not a great idea to serialize pk as raw bytes in general

@Evalir Evalir requested review from mattsse and DaniPopes February 1, 2024 16:59
@Evalir Evalir marked this pull request as ready for review February 1, 2024 16:59
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, lets add an issue for the b256 -> LocalWallet and LocalWallet -> B256 calls?

@Evalir Evalir merged commit 6a7c2da into main Feb 1, 2024
14 checks passed
@Evalir Evalir deleted the evalir/add-genesis-pk-support branch February 1, 2024 17:15
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