[cli] add alias support for sui addresses in the CLI #13738
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
There was a problem hiding this comment.
I realised only after I reviewed that this PR was in draft! I just got the email for it and blindly started reviewing. (I guess I was excited cos this is a cool feature :) ).
High level notes:
- You'll need to add tests for this new command, especially for the edge cases around aliases not existing and already existing.
- You need to handle the case of a keystore that does not have an alias for a particular address, because people will update their CLI and use it with an existing key store.
Sorry about that @amnn. I wanted to push it up to be able to advertise it and get early feedback on this, and was planning to work on it more. It's a bigger feature than I expected so it will require a few more iterations! Thanks though for taking a look at it! |
|
No problem, usually it's fine to push it as a draft, I think the issues happen if the PR is ever published! |
stefan-mysten
force-pushed
the
cli_add_address_alias
branch
from
24f360e to
f6dcadc
Compare
stefan-mysten
force-pushed
the
cli_add_address_alias
branch
from
f6dcadc to
bac3075
Compare
There was a problem hiding this comment.
Much improved, thanks @stefan-mysten ! Main comments in this review:
- Can we store the public key, rather than the private key in the aliases table?
- Make sure error messages make sense to users of the tool, not just us as developers (no debug output, no mention of internal types, etc).
- Some other clean-ups.
crates/sui-keys/src/keystore.rs
Outdated
| @@ -270,6 +394,10 @@ impl AccountKeystore for InMemKeystore { | |||
| None => Err(anyhow!("Cannot find key for address: [{address}]")), | |||
| } | |||
| } | |||
|
|
|||
| fn create_alias_if_not_exists(&self, _alias: &Option<String>) -> Result<String, anyhow::Error> { | |||
There was a problem hiding this comment.
This makes me think that this function doesn't belong on this trait. Instead, why don't you implement it as a free function that accepts a &dyn AccountKeystore to get access to the existing aliases from?
There was a problem hiding this comment.
Yes, I've been torn on it. Initially I had aliases for the InMemKeystore but it kinda does not make sense to have one, I think. I'll fix this, thank you.
There was a problem hiding this comment.
&dyn AccountKeystore might not work because sign_hashed takes a generic type T, and the compiler complains. I'll find a workaround.
There was a problem hiding this comment.
@amnn this is the only thing that I am not sure how to go about. Otherwise, PR is ready for re-review.
There was a problem hiding this comment.
I'll need to see the rest of the error message (e.g. why this trait cannot be made into an object) to know for sure how to fix this issue, but I think it's possible to side step this entirely, because I was only interested in creating a function like this:
fn create_alias_if_not_exists(store: &dyn AccountKeyStore, alias: &Option<String> -> Result<String> {
let existing: HashSet<_> = store.alias_names().into_iter().collect();
match alias {
Some(a) if existing.contains(a) {
bail!("Alias {a} already exists. Please choose another alias.");
}
Some(a) => Ok(a),
None => random_name(&existing_aliases)
}
}Or better yet, change the interface, so that AccountKeyStore can tell you whether an alias_exists, and that should be an easier to re-use primitive.
crates/sui-keys/src/keystore.rs
Outdated
| @@ -270,6 +394,10 @@ impl AccountKeystore for InMemKeystore { | |||
| None => Err(anyhow!("Cannot find key for address: [{address}]")), | |||
| } | |||
| } | |||
|
|
|||
| fn create_alias_if_not_exists(&self, _alias: &Option<String>) -> Result<String, anyhow::Error> { | |||
There was a problem hiding this comment.
I'll need to see the rest of the error message (e.g. why this trait cannot be made into an object) to know for sure how to fix this issue, but I think it's possible to side step this entirely, because I was only interested in creating a function like this:
fn create_alias_if_not_exists(store: &dyn AccountKeyStore, alias: &Option<String> -> Result<String> {
let existing: HashSet<_> = store.alias_names().into_iter().collect();
match alias {
Some(a) if existing.contains(a) {
bail!("Alias {a} already exists. Please choose another alias.");
}
Some(a) => Ok(a),
None => random_name(&existing_aliases)
}
}Or better yet, change the interface, so that AccountKeyStore can tell you whether an alias_exists, and that should be an easier to re-use primitive.
|
Thanks @amnn for the suggestions. |
Here's the error; as per the docs, |
stefan-mysten
force-pushed
the
cli_add_address_alias
branch
from
9264ae8 to
6e50efa
Compare
|
Got it, yep. So the trick would be to try and make that API use dynamic dispatch as well by changing it to accept a This may not work because |
I'll give it a try in a different PR. There's a bunch of commands that need integration with alias now, and that's a bit more important. But I'd love to refactor this a bit later once everything is in, because then I will have a more clear picture of what's going on, what's needed, and where. Thanks @amnn for guiding this one! |
|
My 0.2 cents and trying to rationalize community SDK impacts. Firstly (pet peeve) Clarity
I see that in 1.16 the alias file is being generated already so I assume as per current schedule we are looking at Jan '24... happy new year! |
|
I will let @stefan-mysten clarify the detailed workings of this feature, but on this point:
I think this is something that we'll need to think about more holistically than in the context of just this feature. Technically speaking, there is no association between a given keypair and a network: People are free to use the same keypair across multiple networks and this is also true in the wallet (the wallet allows users to switch addresses independently of the network). In practice, people do tend to associate addresses with networks though, so we should discuss whether that is a feature we want to support, but need to be careful about not breaking backwards compatibility for people who do use the same address across multiple networks. |
Yes, separation of concerns so to speak. Again, pet-peeve. Notwithstanding the Telegram aspect of 'use at own risk' reading the binary setup config has been as base capability of pysui, there are other options that let dev set RPC URL and private keys (ephemeral). However; aliases (IMO) is a nice feature and of course I'd rather leverage what is sitting there than build something anew. |
|
@stefan-mysten
And, mapping to your comments in Telegram General:
You:
Additional questions:
|
|
@FrankC01 thanks for following up and apologies for the slow response (I was OOO).
Now I understand the question, yes, it should be available there too!
Good question. I understand that all sorts of weird stuff can be done when naming things, so here's what I've done. Given that the alias is supposed to make it easier to distinguish addresses, and its main purpose is to aid and simplify the experience for the user when using the CLI, I think a few simple rules are useful here. |
|
Thanks! |
Description
This PR adds support for basic alias naming of Sui addresses in the keystore. If an alias is not provided, it will automatically generate a random name as
first-secondbased on a list of predefined strings (adjectives and precious stone gems).Test Plan
Added tests to ensure new functionality works as expected.
Manual tests:
sui client new-address ed25519 my_alias_test / sui client new-address ed25519If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
Added initial support for basic alias naming of Sui addresses in the keystore.