Refactor/wallet generic signer #1783

OBorce · 2024-06-18T08:58:20Z

add a generic signer provider for creating a software or a hardware signer
make account key chain generic to allow using it with and without a VRF keychain
add an implementation for the trezor signer
copy auto generated trezor client to communicate with a connected trezor device
add new hardware-wallet option when creating and opening a wallet in the CLI and RPC wallet commands

do_checks.sh

wallet/wallet-controller/src/lib.rs

wallet/src/signer/trezor_signer/mod.rs

build-tools/codecheck/codecheck.py

crypto/src/key/secp256k1/extended_keys.rs

crypto/src/key/extended.rs

azarovh · 2024-08-06T13:20:16Z

node-gui/src/backend/backend_impl.rs

+            }
+            #[cfg(feature = "trezor")]
+            (WalletType::Trezor, _) => {
+                let client = make_cold_wallet_rpc_client(Arc::clone(&self.chain_config));


why cold mode? I thought that's the whole point that hardware wallet is both convenient and secure.

let's make it an error? Not sure if there is a use case for Trezor + Cold

Cargo.toml

wallet/src/signer/trezor_signer/mod.rs

wallet/Cargo.toml

wallet/wallet-cli-commands/src/helper_types.rs

wallet/wallet-cli-commands/src/command_handler/mod.rs

wallet/wallet-controller/src/lib.rs

wallet/src/signer/trezor_signer/mod.rs

wallet/src/signer/trezor_signer/tests.rs

wallet/src/signer/trezor_signer/mod.rs

azarovh · 2024-08-24T10:53:15Z

wallet/wallet-cli-commands/src/lib.rs

+
+        /// Create a wallet using a connected hardware wallet. Only the public keys will be kept in
+        /// the software wallet
+        #[arg(long, conflicts_with_all(["mnemonic", "passphrase"]))]


it also conflicts with whether_to_store_seed_phrase

whether_to_store_seed_phrase was a required parameter I am not sure if we should break backwards compatibility or not, that is why I left it alone, and just check that it is set to false.

Should thenwhether_to_store_seed_phrase be marked with required_unless_present("hardware_wallet") or something like that? So people won't have to type it every time for hardware.

it also conflicts with whether_to_store_seed_phrase

I guess this comment is still relevant.

Also, I suggest updating descriptions for all options that don't work with "--hardware_wallet" so that they explicitly mention this.

wallet/wallet-cli-commands/src/lib.rs

wallet/wallet-controller/src/types/mod.rs

wallet/wallet-cli-commands/src/lib.rs

wallet/src/signer/trezor_signer/mod.rs

wallet/wallet-cli-commands/src/helper_types.rs

wallet/wallet-rpc-lib/src/rpc/types.rs

wallet/wallet-rpc-lib/src/service/worker.rs

wallet/types/src/wallet_type.rs

wallet/wallet-cli-commands/src/command_handler/mod.rs

wallet/src/signer/trezor_signer/tests.rs

azarovh · 2024-10-08T07:58:46Z

wallet/src/signer/trezor_signer/tests.rs

+            Destination::AnyoneCanSpend,
+        ),
+        TxOutput::DataDeposit(vec![1, 2, 3]),
+        TxOutput::Htlc(OutputValue::Coin(burn_amount), Box::new(hash_lock)),


there also should be a case for spending Htlc utxo with a secret

wallet/src/signer/trezor_signer/tests.rs

azarovh · 2024-10-08T08:15:18Z

wallet/src/signer/trezor_signer/tests.rs

+    assert!(!devices.is_empty());
+    let client = devices.pop().unwrap().connect().unwrap();
+
+    let mut signer = TrezorSigner::new(chain_config.clone(), Arc::new(Mutex::new(client)));


There should be some negative cases: where trezor failed to sign inputs with unknown keys or something like that.

How does device become aware of private keys? I see that at the beginning of the test a key chain is created from mnemonic, but how trezor is related to that I don't understand (like initialising it with test mnemonic of something)

ok I keep forgetting that this is manual test. So basically you have to recover a wallet from test mnemonic right?

ImplOfAnImpl · 2024-12-12T10:33:53Z

wallet/wallet-controller/src/helpers.rs

+        TxOutput::ProduceBlockFromStake(_, pool_id) => {
+            let staker_balance = rpc_client
+                .get_staker_balance(*pool_id)
+                .await
+                .map_err(ControllerError::NodeCallError)?
+                .ok_or(WalletError::UnknownPoolId(*pool_id))?;
+            Ok(UtxoWithAdditionalInfo::new(
+                utxo,
+                Some(UtxoAdditionalInfo::PoolInfo { staker_balance }),
+            ))
+        }
+        TxOutput::IssueNft(_, _, _)


I find this somewhat scary. E.g. we're signing a raw transaction, which was created who knows when, and then here we're obtaining the pool balance which now may be different from the one that it had when the tx was created (e.g. due to a reorg). So we're basically producing potentially incorrect data and rely on it being ignored (because, as I can see, it's not used in places where this function is called).
Which makes me think that "dynamic" data, like pool balances (as opposed to "static" data, like token decimals) shouldn't be in UtxoWithAdditionalInfo.
But then it'd make sense to move the static data out of it as well, because e.g. it'd be more natural for token decimals to be stored in a single map and passed alongside PartiallySignedTransaction (or maybe it can still be a part of PartiallySignedTransaction, but not a part of individual utxos).
I wonder whether we should reconsider this whole approach with UtxoWithAdditionalInfo. @azarovh what do you think?

Not sure there is a way around it because we have to display what is being signed and a tx has to have an output with specific decommissioned amount.

The thing is that the same dynamic data is also used on tx creation and once you submit the tx to the node to validate the tx. So if it is wrong now it will also be wrong on submittion.

~~The only issue I see is if the user's node itself creates 2 blocks which cause a reorg. They create the tx while on the first block and then a reorg happens with the other block which can have more or less fees so the tx might be rejected if the new balance is less or silently accepted and the excess coins going to fees. ~~ actually the block id will be different and the utxo will not be the same

ImplOfAnImpl · 2024-12-16T08:46:07Z

wallet/src/signer/trezor_signer/mod.rs

+    let device = find_devices(false)
+        .into_iter()
+        .find(|device| device.model == Model::Trezor)
+        .ok_or(TrezorError::NoDeviceFound)?;


Regarding 'Model' variants - why ignore TrezorEmulator? Also, TrezorLegacy is the Model One as I can see, so we should support it too.

What if multiple devices are connected? I think it's not a good idea to select one randomly. Probably the easiest solution is to refuse to proceed asking the user to leave only one device connected. But ideally we should allow to select the device.

ImplOfAnImpl · 2024-12-16T09:10:24Z

Some additional thoughts:

I don't think you ever check the device for Capability_Mintlayer. If so, we'll be getting bug reports like "nothing works", only to find out later that the user didn't install our firmware.
It's better to show to the user the name of the device that is being used, to avoid confusion (the label field of message Features).
It may even make sense to store the device name in the wallet file itself, so that when there is a mismatch, we could tell the user what device must be used with this wallet file.
(But since the label can be changed any time, we'll probably have to update it every time after we successfully connect to the proper device).
We should probably also store the device_id in the wallet file, so that when we detect public keys mismatch, we can tell the user whether the device itself is wrong or the entered passphrase.
The tricky thing is that device_id is not hardcoded in the device and instead is randomly generated on device setup. So if the user resets the device and restores the same seed phrase, the id will be different. So we'll still have to check the keys first and then if the device_id doesn't match, tell the user that the device seems to be different.

ImplOfAnImpl · 2024-12-16T09:45:00Z

Some minor issues:

The wallet file is created before a device is successfully discovered. So, if there are no connected devices, the user ends up with a useless wallet file.
The error dialogs in node-gui are messed up. This is the one when no device can be found:

and this is when a file with the same name already exists:

Btw, when I press "ok, the previous dialog is still shown and has to be closed manually:

This happens for the software wallet too.
Also, shouldn't we overwrite the existing file instead of failing? Because the user has already confirmed overwriting when selecting the file.

OBorce force-pushed the refactor/wallet-generic-signer branch from e1f4596 to 37b71b9 Compare June 20, 2024 04:25

OBorce force-pushed the refactor/wallet-generic-signer branch 6 times, most recently from 626b7d1 to fb2af0d Compare August 1, 2024 07:47

OBorce marked this pull request as ready for review August 1, 2024 12:04

OBorce requested review from erubboli, azarovh and ImplOfAnImpl as code owners August 1, 2024 12:04

OBorce commented Aug 1, 2024

View reviewed changes

do_checks.sh Outdated Show resolved Hide resolved

OBorce commented Aug 1, 2024

View reviewed changes

wallet/wallet-controller/src/lib.rs Outdated Show resolved Hide resolved

azarovh reviewed Aug 5, 2024

View reviewed changes

wallet/src/signer/trezor_signer/mod.rs Outdated Show resolved Hide resolved

OBorce force-pushed the refactor/wallet-generic-signer branch from 726bb45 to 855cfce Compare August 6, 2024 10:54

azarovh reviewed Aug 6, 2024

View reviewed changes

ImplOfAnImpl approved these changes Aug 7, 2024

View reviewed changes

OBorce force-pushed the refactor/wallet-generic-signer branch 2 times, most recently from 1cd8110 to c23f38d Compare August 13, 2024 23:23

azarovh reviewed Aug 24, 2024

View reviewed changes

OBorce force-pushed the refactor/wallet-generic-signer branch from c23f38d to 4824da8 Compare August 30, 2024 16:09

OBorce force-pushed the refactor/wallet-generic-signer branch 4 times, most recently from 6c3d058 to 037f258 Compare September 17, 2024 19:42

OBorce requested review from azarovh and ImplOfAnImpl September 18, 2024 08:53

azarovh reviewed Oct 7, 2024

View reviewed changes

wallet/wallet-cli-commands/src/helper_types.rs Outdated Show resolved Hide resolved

azarovh reviewed Oct 7, 2024

View reviewed changes

azarovh reviewed Oct 8, 2024

View reviewed changes

OBorce added 23 commits December 10, 2024 17:10

clean up from review comments

89d3a9f

update rpc docs

1c14326

Add additional utxo info for tx outputs

8c5932e

move PartiallySignedTx to wallet

ad37936

refactor review comments

e236789

refactor wallet utxo token_id uses

aba9987

add separate additional data for anyonecanstake

fdc3715

add support for signing HTLC outputs with trezor

2ad94fd

fix comments

7589749

extract runtime wallet functionality

1e18f52

fix functional tests

d00d161

fix trezor test

35b4db9

add ChangeTokenMetadataUri to trezor singer

955fd6c

Convert TrezorError to string

c4b6bca

Add support for orders in trezor

d7775a2

fix trezor produce from stake additional info

cbdffea

add all inputs/outputs to trezor test

b3b9ce3

fix fill order change after rebase

66a3bc7

Update trezor dependency and new messages

2e88f4b

Reuse referenced tokens util in API Server

722d70e

Add test with signatures from trezor

6713e04

fix comments

1d766e5

Implement sign tx intent for Trezor signer

511bba3

OBorce force-pushed the refactor/wallet-generic-signer branch from fd48654 to 511bba3 Compare December 10, 2024 16:56

remove default feature

46cf688

OBorce force-pushed the refactor/wallet-generic-signer branch from 3b76519 to 46cf688 Compare December 11, 2024 10:35

ImplOfAnImpl reviewed Dec 12, 2024

View reviewed changes

ImplOfAnImpl reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/wallet generic signer #1783

Refactor/wallet generic signer #1783

OBorce commented Jun 18, 2024 •

edited

Loading

azarovh Aug 6, 2024

azarovh Nov 26, 2024

azarovh Aug 24, 2024

OBorce Sep 17, 2024

azarovh Oct 7, 2024

ImplOfAnImpl Dec 16, 2024

azarovh Oct 8, 2024

azarovh Oct 8, 2024

azarovh Oct 8, 2024

ImplOfAnImpl Dec 12, 2024

azarovh Dec 12, 2024

OBorce Dec 12, 2024 •

edited

Loading

ImplOfAnImpl Dec 16, 2024

ImplOfAnImpl commented Dec 16, 2024

ImplOfAnImpl commented Dec 16, 2024

Refactor/wallet generic signer #1783

Are you sure you want to change the base?

Refactor/wallet generic signer #1783

Conversation

OBorce commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OBorce Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImplOfAnImpl commented Dec 16, 2024

ImplOfAnImpl commented Dec 16, 2024

OBorce commented Jun 18, 2024 •

edited

Loading

OBorce Dec 12, 2024 •

edited

Loading