Skip to content

feat(signer): adds signer-stronghold #2420

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

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

Conversation

tuddman
Copy link

@tuddman tuddman commented May 10, 2025

Solution

Adds a signer using a Stronghold as the store

PR Checklist

  • Added Tests
  • Added Documentation

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.

seems okay, simple enough, not much work to maintain, ty

@github-project-automation github-project-automation bot moved this to Reviewed in Alloy May 13, 2025
@mattsse mattsse enabled auto-merge (squash) May 13, 2025 14:58
@mattsse
Copy link
Member

mattsse commented May 13, 2025

@tuddman most of these tests are timing out, mind taking a look?

auto-merge was automatically disabled May 14, 2025 12:36

Head branch was pushed to by a user without write access

@tuddman
Copy link
Author

tuddman commented May 14, 2025

@mattsse you can maybe try running the CI again?

works locally...

running 11 tests
test signer::tests::test_missing_passphrase ... ok
test signer::tests::test_new_from_path ... ok
test signer::tests::test_chain_id_management has been running for over 60 seconds
test signer::tests::test_end_to_end_transaction_with_anvil has been running for over 60 seconds
test signer::tests::test_get_evm_address has been running for over 60 seconds
test signer::tests::test_initialize_new_signer has been running for over 60 seconds
test signer::tests::test_reinitialize_existing_signer has been running for over 60 seconds
test signer::tests::test_sign_hash has been running for over 60 seconds
test signer::tests::test_sign_message has been running for over 60 seconds
test signer::tests::test_sign_transaction has been running for over 60 seconds
test signer::tests::test_get_evm_address ... ok
test signer::tests::test_sign_hash ... ok
test signer::tests::test_chain_id_management ... ok
test signer::tests::test_initialize_new_signer ... ok
test signer::tests::test_sign_message ... ok
test signer::tests::test_sign_transaction ... ok
test signer::tests::test_end_to_end_transaction_with_anvil ... ok
test signer::tests::test_reinitialize_existing_signer ... ok
test signer::tests::test_signer_trait_implementation ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 159.88s

   Doc-tests alloy_signer_stronghold

running 2 tests
test crates/signer-stronghold/src/lib.rs - (line 48) - compile ... ok
test crates/signer-stronghold/src/lib.rs - (line 29) - compile ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s

@@ -515,7 +515,8 @@ impl Geth {

if let Some(genesis) = &self.genesis {
// create a temp dir to store the genesis file
let temp_genesis_dir_path = tempdir().map_err(NodeError::CreateDirError)?.keep();
let temp_genesis_dir_path =
tempdir().map_err(NodeError::CreateDirError)?.path().to_path_buf();
Copy link
Author

Choose a reason for hiding this comment

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

I was getting a compilation error from this when trying to clean + test.
This fixes that compilation error.

@gakonst
Copy link
Member

gakonst commented May 14, 2025

@mattsse @tuddman could maintain this in their own repo? I'm not a fan of having this upstream and it might be hard to maintain

@tuddman
Copy link
Author

tuddman commented May 14, 2025

@mattsse @tuddman could maintain this in their own repo? I'm not a fan of having this upstream and it might be hard to maintain

Up to you, obviously. I'll continue maintaining and supporting it either way. Having it available more broadly tends to surface bugs and improvements faster. It's stable and version-pinned, so maintenance overhead is minimal, imo. And I generally believe offering more options helps the community overall, e.g. like with alloy overall.

@mattsse
Copy link
Member

mattsse commented May 14, 2025

right, now that we have 1.0 which we want to keep stable, I think as a standalone repo this would be more appropriate because iota specific.

@tuddman
Copy link
Author

tuddman commented May 14, 2025

ok. to be clear though - stronghold.rs was built largely to underpin iota's wallet, firefly. afaik, that has never been hacked or compromised. It was originally crafted for iota by @tensor-programming and afaict is truly a 'general purpose' secrets keeper that makes strong protection claims. In that way it's not 'iota-specific' really at all. I'm certainly not using it for that.
.with_chain_id(<not_iota>) is evidence enough it can now be used as a Signer for any EIP-155 chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Reviewed
Development

Successfully merging this pull request may close these issues.

3 participants