Skip to content
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

CLI watch-only account implementation #470

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

1bananagirl
Copy link

The new account type watch-only supports:

  • bip32
  • multisig

Changes:

  • print the extended public key for the command "export mnemonic" like it is in the golang kaspwallet
  • added handling of keyless accounts in select_account_with_args and list functions
  • watch-only and xpub text hints added to the list function to differentiate account type

Future:

  • creating unsigned transactions with watch-only wallets

cli/src/cli.rs Outdated
tprintln!(self, "• {}", account.get_list_string()?);
let watch_only = account.downcast_arc::<WatchOnly>()?;
let xpubs = watch_only.xpub_keys();
if xpubs.len() > 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have multisig wallets. Why multisig is not extended to use watch only?
Imo watch-only must have a single xpub. Watch only multisig must be a part of multisig

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually gave that guidance as I think that it is better to be explicit about the purpose of the account. But you are right as multisig is technically a watch account by default. Perhaps the account kind should then be explicit, as in "bip32-watch"?

cli/src/cli.rs Outdated
}
xpubs.iter().enumerate().for_each(|(_idx, xpub)| {
let info_xpub: String = format!("{}", xpub.to_string(Some(Prefix::KPUB)));
tprintln!(self, " • [xpub] {}", style(info_xpub).dim());
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo Choose right prefix according to the network

@@ -49,13 +50,19 @@ async fn export_multisig_account(ctx: Arc<KaspaCli>, account: Arc<MultiSig>) ->
let prv_key_data = prv_key_data_store.load_key_data(&wallet_secret, prv_key_data_id).await?.unwrap();
let mnemonic = prv_key_data.as_mnemonic(None).unwrap().unwrap();

let xpub_key: kaspa_bip32::ExtendedPublicKey<kaspa_bip32::secp256k1::PublicKey> = prv_key_data.create_xpub(None, MULTISIG_ACCOUNT_KIND.into(), 0).await?; // todo it can be done concurrently
let xpub_export = xpub_key.to_string(Some(Prefix::KPUB));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefix should depend on the current network


tprintln!(ctx, "extended public key:");
tprintln!(ctx, "");
tprintln!(ctx, "{}", xpub_key.to_string(Some(Prefix::KPUB)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefix

@@ -85,3 +85,49 @@ async fn create_multisig(ctx: &Arc<KaspaCli>, account_name: Option<String>, mnem
wallet.select(Some(&account)).await?;
Ok(())
}

pub(crate) async fn watch_only(ctx: &Arc<KaspaCli>, name: Option<&str>) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name watch-only is suitable for getter. Creation should be prefixed with create/new

&xpub_keys,
ecdsa,
0,
Some(u32::MIN),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cosigner index is none since watch only doesn't have even a single signer

}

fn sig_op_count(&self) -> u8 {
1
Copy link
Collaborator

Choose a reason for hiding this comment

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

equal minimum signatures

Comment on lines 99 to 115
let mut xpub_keys = Vec::with_capacity(1);
let xpub_key = term.ask(false, &format!("Enter extended public key: ")).await?;
xpub_keys.push(xpub_key.trim().to_owned());

let mut additional_xpubs: Vec<String> = vec![];
let mut n_required: u16 = 1;
if additional_xpubs.is_empty() {
loop {
let xpub_key = term
.ask(false, "For a multisig watch-only account enter an additional extended public key: (empty to skip or stop) ")
.await?;
if xpub_key.is_empty() {
break;
}
additional_xpubs.push(xpub_key.trim().to_owned());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be part of multisig import functionality

Comment on lines +85 to +91
pub struct WatchOnly {
inner: Arc<Inner>,
xpub_keys: ExtendedPublicKeys,
minimum_signatures: u16,
ecdsa: bool,
derivation: Arc<AddressDerivationManager>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

make watch only acc only suitable for a single acc

…ter NetworkId for Prefix. BIP32-Watch Account renamed from WatchOnly, multisig feature removed. Multisig account import as watch-only command added.
@1bananagirl
Copy link
Author

Thanks for you review Maxim!

Refactoring

I've renamed the watch-only account to bip32-watch and removed multisig polymorphism, thus multisig watch-only remaining solely in the multisig account.

I've followed aspect's advice to add a helper function in the wallet module to format XPUB export according to network.

The new helper function is triggered by providing a new type TaggedXpub that is a pair formed of an extended public key and the network ID.

Inside the bip32 prefix submodule resides a new converter to get the right prefix for a given network.

Account feature trait

The notion of feature was added to the account trait as a fn, defaulting to Option None, and is implemented in Bip32Watch and Multisig/watch-only to return a string giving an indication on the special feature of an account - which is watch-only in our case - an indication used to highlight that feature in the list and details commands output.

That step was necessary for the iterator filtering in details and list commands to be simple stupid finding out which account without private keys to consider printing.

Commands

CLI commands for importing watch-only accounts now look like this:

   account import bip32-watch
   account import multisig-watch

I'm open to switch this to "account create" if needed.

I've found it more efficient to make a new parameter "multisig-watch" instead of bypassing "account create multisig", but if you insist, I can make that work instead.

Output

The display of xpubs for watch-only account has been moved from the CLI list command output to the details output in order to not clutter the list view.

According to the currently selected network, the xpubs are now formatted with the appropriate prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants