Disallow PubKey as PrivKey for cmd_set_default_account command - display error if user accidentally pass wrong key.
#1645
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rivate key when using cmd_set_default_account, throwing specific error if you accidentally pass in the public key instead of private key.
Sajjon
commented
Nov 10, 2023
| @@ -27,6 +27,16 @@ impl Secp256k1PrivateKey { | |||
| self.0.secret_bytes().to_vec() | |||
| } | |||
|
|
|||
| pub fn to_hex(&self) -> String { | |||
There was a problem hiding this comment.
We convert between key and hex in a few places, makes sense for the type type to have these methods IMO.
Sajjon
commented
Nov 10, 2023
| @@ -19,12 +19,47 @@ pub struct SetDefaultAccount { | |||
| impl SetDefaultAccount { | |||
| pub fn run<O: std::io::Write>(&self, out: &mut O) -> Result<(), Error> { | |||
| let mut configs = get_configs()?; | |||
| let private_key = parse_private_key_from_str(&self.private_key).map_err(|e| { | |||
| if Secp256k1PublicKey::from_str(&self.private_key).is_ok() { | |||
| Error::GotPublicKeyExpectedPrivateKey | |||
There was a problem hiding this comment.
If we fail to create a PrivateKey (32 bytes/64 hex char input) from the passed in string, we try to parse it as a PublicKey to see if the user made the very reasonable error of accidentally copy pasting the public key - like I did!
iamyulong
approved these changes
Nov 10, 2023
iamyulong
approved these changes
Nov 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Validate input to
cmd_set_default_account, ensuring we don't accidentally pass in PublicKey instead of PrivateKey.Details
In
mainit is possible to accidentally pass in the PublicKey to the PrivateKey value for commandcmd_set_default_account. This results in confusing state and error when trying to e.g. perform a transfer.N.B: we don't get any information on what went wrong, that is because of the program panics at unwrap at line 62, so we never reach the next line, with more info:
Error::NoDefaultPrivateKey. This should also be fix (I can do another PR), but this PR fixes so that should never happen...I got into this state since I accidentally copied the PublicKey instead of the PrivateKey, resulting in this bad state.
Testing
Now it is not possible to pass in the PublicKey instead of PrivateKey anymore, and we display a very informative error
GotPublicKeyExpectedPrivateKey.Added a unit test.