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

Re-export secp256k1::SecretKey. #633

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

dandanlen
Copy link
Contributor

secp256k1::SecretKey is required in order to use the offline signing functionality but at present it's not re-exported. This means anyone who want to use the signing functionality has to add an explicit dependency on secp256k1, and has to keep it in-sync with the version in web3.

This PR changes the re-export of the secp256k1::SecretKey from pub(crate) to pub so that is can be accessed through the web3::signing module, removing the requirement for an explicit dependency on secp256k1.

`secp256k1::SecretKey` is required in order to use the offline signing functionality but it's not re-exported. 

This is a simple PR that re-exports the `SecretKey` which allows the following:

```rust
let s = web3::SecretKey::from_str("XYZ");
web3.accounts().sign_transaction(my_tx, &prvk).await?;
```

without having to import `secp256k1` as an explicit dependency alongside `web3`.
@tomusdrw
Copy link
Owner

tomusdrw commented Jun 2, 2023

While I think the original intent was to only expose trait Key to prevent misusing SecretKey (i.e. using it without zeroize), I believe it isn't sufficient to prevent misuse and in reality handling the secret keys properly is way beyond the purpose of this crate.
The secp256k1::SecretKey documentation also mentions that keys should be handled and SecretKey::non_secure_erase method is exposed, linking to zeroize crate for more info, hence I'm willing to merge the PR to improve usability.

@tomusdrw tomusdrw enabled auto-merge (squash) June 2, 2023 19:51
auto-merge was automatically disabled June 5, 2023 11:03

Head branch was pushed to by a user without write access

@dandanlen
Copy link
Contributor Author

Thanks for this - I fixed the formatting issue that was blocking the merge...

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@tomusdrw tomusdrw merged commit 5ff2d93 into tomusdrw:master Jun 5, 2023
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.

2 participants