lianad: warn createrecovery to an address of the same wallet.

[qlrd]

This commit adds a new error (code 1001, just after the existing 1000) bound to createrecovery -- as warning response when a user wants to re-lock funds from/to addresses derived from the wallet descriptor.

As stated by jp1ac4 the starting point is to check this on the backend since it have easier access to known wallet addresses (needs a check on how to deal with derivation indices beyond what the DB knows).

The proposed flow check if the provided address belongs to the user if the DB have Some or None. If None, just continue the flow; if Some, a RecoveryToOwnAddress error will be added to a warnings field as a string.

EDIT: As stated by jp1ac4 the starting point is to check this on the backend since the backend has easier access to known wallet addresses (needs a check on how to deal with derivation indices beyond what the DB knows -- worth of a follow-up).

If the DB knows the sweep address as a derived one, it belongs to the wallet's descriptor and recovered funds would be locked under the same descriptor again.

In that case a CreateRecoveryWarning::ToOwnAddress enum is used as a warning with the address to be added to a new warnings field of the createrecovery response so the rpc command could respond with {"psbt": ..., "warnings": [{ "to_own_address": "<addr>"}] }.

Close #1654.

Details

A follow-up commit (the GUI confirmation flow) is being drafted while discussing.

EDIT: the gui commit handles the previous commit createrecovery rpc update to Daemon trait, so when user request do a self-transfer when using recovery options on GUI, it will be warned about the re-lock.

EDIT 2: Maybe it needs some GUI tests? maybe now or follow up?

[qlrd]

I've made the required changes and verified tested here with:

cargo fmt --all --check
cargo clippy
cargo test
(venv) python3 -m pytest tests/

If you agree with the current code, I will proceed with the GUI plumbing (if not, let me know). Thxs for the tips, it reduced the diff a lot.

[pythcoiner]

nit, it's not a server error:

diff --git a/lianad/src/jsonrpc/rpc.rs b/lianad/src/jsonrpc/rpc.rs
index d33aae7a5..f6536e01f 100644
--- a/lianad/src/jsonrpc/rpc.rs
+++ b/lianad/src/jsonrpc/rpc.rs
@@ -51,9 +51,6 @@ pub struct Request {
 /// A failure to broadcast a transaction to the P2P network.
 const BROADCAST_ERROR: i64 = 1_000;

-/// A failure while sweeping funds to an address from the wallet itself.
-const RECOVERY_TO_OWN_ADDRESS_ERROR: i64 = 1_001;
-
 /// JSONRPC2 error codes. See https://www.jsonrpc.org/specification#error_object.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum ErrorCode {
@@ -170,16 +167,13 @@ impl From<commands::CommandError> for Error {
             | commands::CommandError::OutpointNotRecoverable(..) => {
                 Error::new(ErrorCode::InvalidParams, e.to_string())
             }
-            commands::CommandError::RescanTrigger(..) => {
+            commands::CommandError::RescanTrigger(..)
+            | commands::CommandError::RecoveryToOwnAddress(..) => {
                 Error::new(ErrorCode::InternalError, e.to_string())
             }
             commands::CommandError::TxBroadcast(_) => {
                 Error::new(ErrorCode::ServerError(BROADCAST_ERROR), e.to_string())
             }
-            commands::CommandError::RecoveryToOwnAddress(_) => Error::new(
-                ErrorCode::ServerError(RECOVERY_TO_OWN_ADDRESS_ERROR),
-                e.to_string(),
-            ),
         }
     }
 }

[pythcoiner]

overall lgtm, feel free to continue with the GUI

[qlrd]

The pumbled code into GUI preview (with floresta backend btw :)

[pythcoiner]

@nondiremanuel I guess you'd like to review the wording of the warning

[nondiremanuel]

@nondiremanuel I guess you'd like to review the wording of the warning

Yes, thanks. I would personally use something more action oriented like:

⚠ Recovery address belongs to the same wallet. If you can no longer spend with the primary path, use an address from a different wallet instead.

Happy to discuss if you guys prefer other options.

[qlrd]

@nondiremanuel I guess you'd like to review the wording of the warning

Yes, thanks. I would personally use something more action oriented like:

⚠ Recovery address belongs to the same wallet. If you can no longer spend with the primary path, use an address from a different wallet instead.

Happy to discuss if you guys prefer other options.

I liked this one. For me sometimes is hard to put things in a clear way. And wen readed that, i think your phrase is the most synthetic and pedagogical than mine.

[qlrd]

@nondiremanuel I guess you'd like to review the wording of the warning

Yes, thanks. I would personally use something more action oriented like:

⚠ Recovery address belongs to the same wallet. If you can no longer spend with the primary path, use an address from a different wallet instead.

Happy to discuss if you guys prefer other options.

Following your idea