Skip to content

Commit

Permalink
[cli] Fix bug where the address is not decoded from its PublicKey (#1…
Browse files Browse the repository at this point in the history
…5169)

## Description 

The SuiAddress should be decoded from `PublicKey` and not `SuiKeyPair`,
as the alias file encodes the public key as base64.

## Test Plan 

Added new test

---
If 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)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
Fixed a bug where reading the CLI alias file resulted in creating the wrong
address.
  • Loading branch information
stefan-mysten committed Dec 3, 2023
1 parent 1265c0a commit c774f84
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 3 deletions.
15 changes: 12 additions & 3 deletions crates/sui-keys/src/keystore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub trait AccountKeystore: Send + Sync {
fn addresses(&self) -> Vec<SuiAddress> {
self.keys().iter().map(|k| k.into()).collect()
}

fn addresses_with_alias(&self) -> Vec<(&SuiAddress, &Alias)>;
fn aliases(&self) -> Vec<&Alias>;
fn alias_names(&self) -> Vec<&str> {
self.aliases()
Expand Down Expand Up @@ -198,6 +198,10 @@ impl AccountKeystore for FileBasedKeystore {
self.aliases.values().collect()
}

fn addresses_with_alias(&self) -> Vec<(&SuiAddress, &Alias)> {
self.aliases.iter().collect::<Vec<_>>()
}

fn keys(&self) -> Vec<PublicKey> {
self.keys.values().map(|key| key.public()).collect()
}
Expand Down Expand Up @@ -281,8 +285,8 @@ impl FileBasedKeystore {
aliases
.into_iter()
.map(|alias| {
let key = SuiKeyPair::decode_base64(&alias.public_key_base64);
key.map(|k| (Into::<SuiAddress>::into(&k.public()), alias))
let key = PublicKey::decode_base64(&alias.public_key_base64);
key.map(|k| (Into::<SuiAddress>::into(&k), alias))
})
.collect::<Result<BTreeMap<_, _>, _>>()
.map_err(|e| {
Expand All @@ -301,6 +305,7 @@ impl FileBasedKeystore {
.zip(names)
.map(|((sui_address, skp), alias)| {
let public_key_base64 = EncodeDecodeBase64::encode_base64(&skp.public());

(
*sui_address,
Alias {
Expand Down Expand Up @@ -435,6 +440,10 @@ impl AccountKeystore for InMemKeystore {
self.aliases.values().collect()
}

fn addresses_with_alias(&self) -> Vec<(&SuiAddress, &Alias)> {
self.aliases.iter().collect::<Vec<_>>()
}

fn keys(&self) -> Vec<PublicKey> {
self.keys.values().map(|key| key.public()).collect()
}
Expand Down
20 changes: 20 additions & 0 deletions crates/sui-keys/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,26 @@ fn create_alias_keystore_file_test() {
assert!(keystore_path.exists());
}

#[test]
fn check_reading_aliases_file_correctly() {
// when reading the alias file containing alias + public key base 64,
// make sure the addresses are correctly converted back from pk

let temp_dir = TempDir::new().unwrap();
let mut keystore_path = temp_dir.path().join("sui.keystore");
let keystore_path_keep = temp_dir.path().join("sui.keystore");
let mut keystore = Keystore::from(FileBasedKeystore::new(&keystore_path).unwrap());
let kp = keystore
.generate_and_add_new_key(SignatureScheme::ED25519, None, None, None)
.unwrap();
keystore_path.set_extension("aliases");
assert!(keystore_path.exists());

let new_keystore = Keystore::from(FileBasedKeystore::new(&keystore_path_keep).unwrap());
let addresses = new_keystore.addresses_with_alias();
assert_eq!(kp.0, *addresses.get(0).unwrap().0)
}

#[test]
fn create_alias_if_not_exists_test() {
let temp_dir = TempDir::new().unwrap();
Expand Down

2 comments on commit c774f84

@vercel
Copy link

@vercel vercel bot commented on c774f84 Dec 3, 2023

Choose a reason for hiding this comment

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

@vercel
Copy link

@vercel vercel bot commented on c774f84 Dec 3, 2023

Choose a reason for hiding this comment

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

Please sign in to comment.