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

feat(wallet): add view key commands #6426

Merged

Conversation

stringhandler
Copy link
Collaborator

@stringhandler stringhandler commented Jul 24, 2024

Description

Adds commands for exporting a view key and spend key for an existing wallet, and also commands for creating a read-only wallet that only has access to the view key.

Note this is only possible to the great work by @SWvheerden who enabled this functionality in the wallet.

Motivation and Context

This allows you to keep an offline cold wallet that has spend access, and at the same time listen for received funds and act on them. This enables ecommerce and exchange support without the danger of running a hot spending wallet.

How Has This Been Tested?

Tested locally

What process can a PR reviewer use to test or verify this change?

You can follow the steps in the exchange guide to use all of the additions in this PR

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

Copy link

github-actions bot commented Jul 24, 2024

Test Results (CI)

    3 files    123 suites   38m 48s ⏱️
1 300 tests 1 300 ✅ 0 💤 0 ❌
3 892 runs  3 892 ✅ 0 💤 0 ❌

Results for commit 6cda68e.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jul 24, 2024
Copy link

github-actions bot commented Jul 24, 2024

Test Results (Integration tests)

19 tests   19 ✅  10m 54s ⏱️
 9 suites   0 💤
 2 files     0 ❌
 1 errors

For more details on these parsing errors, see this check.

Results for commit 6cda68e.

♻️ This comment has been updated with latest results.

# Conflicts:
#	applications/minotari_console_wallet/src/automation/commands.rs
#	applications/minotari_console_wallet/src/cli.rs
#	applications/minotari_console_wallet/src/init/mod.rs
@stringhandler stringhandler marked this pull request as ready for review August 5, 2024 07:50
@stringhandler stringhandler requested a review from a team as a code owner August 5, 2024 07:50
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

Looking good. Just check the conversion. I don't think we has base58 anywhere expect for the completed tariAddress.

@@ -75,7 +75,7 @@ use tari_core::{
blocks::pre_mine::{create_pre_mine_genesis_block_file, get_pre_mine_items, PreMineItem},
covenants::Covenant,
transactions::{
key_manager::TransactionKeyManagerInterface,
key_manager::{SecretTransactionKeyManagerInterface, TransactionKeyManagerInterface},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not use this trait:
#6447

return Some(WalletType::default());
}

match boot_mode {
WalletBoot::ViewAndSpendKey => {
let view_key = if let Some(vk) = view_private_key {
match PrivateKey::from_base58(&vk) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the command window you use hex, here its base58?

};
let spend_key = if
let Some(sk) = spend_key {
let spend_key = match PublicKey::from_base58(&sk) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as other command hex vs base58

.private_spend_key
.clone()
.ok_or(KeyManagerServiceError::ImportedPrivateKeyInaccessible);
return Ok(PrivateKey::default());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will have a conflict anyway, make sure to choose the current code in origin. This error here is fixed.

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Aug 5, 2024
@stringhandler stringhandler merged commit 77e5ca9 into tari-project:development Aug 5, 2024
15 of 17 checks passed
@stringhandler stringhandler deleted the st-new-view-key-commands branch August 5, 2024 12:59
@hansieodendaal
Copy link
Contributor

I cannot get this menu to be displayed when I run the consol wallet:

        loop {
            println!("1. Create a new wallet.");
            println!("2. Recover wallet from seed words or hardware device.");
            println!("3. Create a read-only wallet using a view key.");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants