-
Notifications
You must be signed in to change notification settings - Fork 78
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: Add new Key
type; allowing public keys to be named keys
#1700
base: main
Are you sure you want to change the base?
Conversation
cca279c
to
10fb1da
Compare
This will allow users to add public keys with `keys add` so that they can be referenced in commands that need public addresses and contract invoke's that take address arguments.
Add tests using
09d72f1
to
31cde54
Compare
Co-authored-by: Leigh McCulloch <[email protected]>
102bc13
to
b36db49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
One thing I'm thinking of is if it would make sense to merge these two PRs before or after the PRs that add the new signers (ledger & keychain), to reduce conflicts. 🤔
cmd/soroban-cli/src/config/secret.rs
Outdated
} | ||
|
||
impl Args { | ||
pub fn read_secret(&self) -> Result<Secret, Error> { | ||
pub fn read_key(&self) -> Result<Key, key::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could go in config/key.rs
instead, since it is also dealing with public keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is just for parsing the Arg struct. So it only should be used with the keys add
command. Though then maybe it should just go into that command? Since there is no reuse of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like the idea of moving it to the add
command, since that is where it's being used. But not merge blocking!
Co-authored-by: Elizabeth Engelman <[email protected]>
Co-authored-by: Elizabeth Engelman <[email protected]>
Co-authored-by: Willem Wyndham <[email protected]>
This simplifies the lookup of the address.
This will allow for exporting the phrase later
This will allow users to add public keys with
keys add
so that they can be referenced in commands that need public addresses and contract invoke's that take address arguments.This is part 1 of 2 in a stack made with GitButler:
Key
type; allowing public keys to be named keys #1700 👈