Feat/os keychain followup

[elizabethengelman]

What

This PR is based on #1703 and adds some additional compatibility with the secure store like keys rm and keys add.

• keys rm
• keys add
• manual test on Mac OS
  • add the key: cargo run keys add --secure-store --entry-name alice alice
  • make sure we can get the address from the new key: cargo run keys address alice
  • make sure we can sign & send with the new key:

      stello contract deploy --wasm target/test-wasms/hello_world.wasm  --build-only --network testnet --source alice | \
      stello tx simulate --network testnet --source alice | \
      stello tx sign --network testnet --sign-with-key alice | \
      stello tx send --network testnet
  

  • remove the key: stello keys rm alice
  • make sure it is no longer in the list or the keychain
• manual test on Linux
  • add the key: cargo run keys add --secure-store --entry-name alice alice
  • make sure we can get the address from the new key: cargo run keys address alice
  • make sure we can sign & send with the new key:

      stello contract deploy --wasm target/test-wasms/hello_world.wasm  --build-only --network testnet --source alice | \
      stello tx simulate --network testnet --source alice | \
      stello tx sign --network testnet --sign-with-key alice | \
      stello tx send --network testnet
  

  • remove the key: stello keys rm alice
  • make sure it is no longer in the list or the keychain
• manual test on Windows
  • add the key: cargo run keys add --secure-store --entry-name alice alice
  • make sure we can get the address from the new key: cargo run keys address alice
  • make sure we can sign & send with the new key:

      stello contract deploy --wasm target/test-wasms/hello_world.wasm  --build-only --network testnet --source alice | \
      stello tx simulate --network testnet --source alice | \
      stello tx sign --network testnet --sign-with-key alice | \
      stello tx send --network testnet
  

  • remove the key: stello keys rm alice
  • make sure it is no longer in the list or the keychain

Why

I wanted to keep #1703 from getting too big so it was easier to review.

Known limitations

[TODO or N/A]

[willemneal]

I still need to try this out locally, but correct me if I'm wrong, but it's possible to have the name used in keys add differ from what is used in entry-name?

[elizabethengelman]

@willemneal

... it's possible to have the name used in keys add differ from what is used in entry-name?

Good call! Yeah, at the moment, this is possible. It still ends up working because we are saving the value of --entry-name in the <NAME>.toml file, like this: entry_name = "secure_store:org.stellar.cli-<NAME>. And then getting that entry_name from the toml file when accessing the public key, or using the private key.

Though, this may be confusing. 🤔

Instead, we could remove the --entry-name flag, and when adding a new key, require that the key name is the same as the entry name in the OS's secure store. So, this command would change to be: cargo run keys add alice.

But, now I'm starting to wonder if we shouldn't allow users to add keys from the keychain at all. If we do, we'd need to make sure that users add the keys to their secure store with:

1. the key name prepended with secure_store:org.stellar.cli-
2. the key saved as a base64 encoded ed25519_dalek::SigningKey

And we could probably do that, but I'm not sure how necessary this feature is. 🤔

[elizabethengelman]

Since we are now planning to store the seed phrase instead of the signing key in the OS secure storage, I think that a more useful feature would be to allow a user to add a new key with their seed phrase, which is saved in secure storage instead of in the local file. So the command could look like this:

cargo run keys add <NAME> --secure-store

which would then prompt the user to enter their seed phrase or secret key, as is the default behavior of add:

Type a secret key or 12/24 word seed phrase:
...

[elizabethengelman]

not sure if this needs its own mod, but wanted to pull the code out where both add and generate could use it

[elizabethengelman]

Curious to hear what others think - but I think that we'll be able to let user's create new keys that are stored in the OS secure storage, and the values saved can be either private keys or seed phrases. That is not quite supported yet here, but I think its possible. Does that make sense, or is it too confusing?

The only downside i'm seeing right now is that when we add the ability to fetch the seed phrase for a key, we'll need to return an error if the user entered their private key directly.