Skip to content

Add Keypair::try_from_base58_string #62

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 1 commit into
base: master
Choose a base branch
from

Conversation

juchiast
Copy link

@juchiast juchiast commented Mar 2, 2025

Add a conversion method that doesn't panic on errors.

Add a conversion method that doesn't panic on errors.
Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! While I think this makes sense, the Box<dyn Error> for the error type really shows the problem with errors in this crate.

We need to add a specific error type for Keypair and use that for this new function. It could probably look very similar to the base error returned by ed25519_dalek, which wraps a Box<dyn Error> if an std feature is enabled: https://docs.rs/signature/2.2.0/signature/struct.Error.html

Then in the future, we can remove ed25519_dalek types from the public interface of this crate, so we can finally upgrade the ed25519_dalek dependency.

All that to say: let's separately add the proper error type and std feature to this crate, then this PR can reuse that. What do you think?

@juchiast
Copy link
Author

I think it make more sense to add a ParseKeypairError for these functions, rather than an opaque Error.

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