Skip to content
This repository was archived by the owner on Nov 25, 2025. It is now read-only.

Add the descriptor argument to createwallet #278

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

Add the descriptor argument to createwallet #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e51c69c
Add the descriptor argument to createwallet
sander2 Feb 17, 2023
801ec12
refactor Unsupported error to take rpc & arg
sander2 Feb 20, 2023
6576be9
use serde 1.0.189 in CI
sander2 Oct 23, 2023
ad106c2
set serde_json to 1.0.100 in CI s.t. we can build with Rust 1.41.1
sander2 Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ jobs:
profile: minimal
toolchain: ${{ matrix.rust }}
override: true
- run: cargo update -p serde --precise 1.0.152
- run: cargo update -p serde_json --precise 1.0.100
- run: cargo update -p serde --precise 1.0.189
- name: Running test script
env: ${{ matrix.env }}
run: ./contrib/test.sh
Expand Down
58 changes: 47 additions & 11 deletions client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,18 +284,54 @@ pub trait RpcApi: Sized {
blank: Option<bool>,
passphrase: Option<&str>,
avoid_reuse: Option<bool>,
descriptors: Option<bool>,
) -> Result<json::LoadWalletResult> {
let mut args = [
wallet.into(),
opt_into_json(disable_private_keys)?,
opt_into_json(blank)?,
opt_into_json(passphrase)?,
opt_into_json(avoid_reuse)?,
];
self.call(
"createwallet",
handle_defaults(&mut args, &[false.into(), false.into(), into_json("")?, false.into()]),
)
// the descriptors argument was added in version 21
if self.version()? < 210000 {
// note: we allow Some(false) since it's the default behavior
if let Some(true) = descriptors {
return Err(Error::UnsupportedArgument("createwallet", "descriptors"));
}
// no descriptors argument yet
let mut args = [
wallet.into(),
opt_into_json(disable_private_keys)?,
opt_into_json(blank)?,
opt_into_json(passphrase)?,
opt_into_json(avoid_reuse)?,
];
self.call(
"createwallet",
handle_defaults(
&mut args,
&[false.into(), false.into(), into_json("")?, false.into()],
),
)
} else {
let mut args = [
wallet.into(),
opt_into_json(disable_private_keys)?,
opt_into_json(blank)?,
opt_into_json(passphrase)?,
opt_into_json(avoid_reuse)?,
opt_into_json(descriptors)?,
];
// from 23 on, the default value of the descriptors argument is true
let default_descriptors = self.version()? >= 230000;
Copy link
Member

@apoelstra apoelstra Feb 17, 2023

Choose a reason for hiding this comment

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

I'm a bit confused about the goal of this line. You are setting default_descriptors to whatever its default value would be ... so why provide it at all?

Copy link
Contributor Author

@sander2 sander2 Feb 20, 2023

Choose a reason for hiding this comment

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

It was easier to determine the default value than to use a variable number of arguments. Also there are more createwallet arguments in the newest version that are not added yet (load_on_startup and external_signer), and if they're added in the future, some value will need to be passed here anyway, right?

Copy link
Member

@apoelstra apoelstra Feb 20, 2023

Choose a reason for hiding this comment

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

FWIW in theory, no, it's possible to use named arguments rather than positional ones, so that we don't have to stick values in just to get to further values. But that's a bigger change and unrelated to this PR :).

It was easier to determine the default value than to use a variable number of arguments.

Okay, but simply dropping default_descriptors won't cause you to need a variable number of arguments. Right now you always pass it. I'm suggesting you never pass it.

Copy link
Contributor Author

@sander2 sander2 Jul 10, 2023

Choose a reason for hiding this comment

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

@apoelstra if I don't include default_descriptors in the list of default arguments, the defaults would be applied incorrectly, right? At least if I understand the documentation of handle_defaults correctly:

/// Note, that `defaults` corresponds to the last elements of `args`.
///
/// ```norust
/// arg1 arg2 arg3 arg4
///           def1 def2

Copy link
Collaborator

@stevenroose stevenroose Oct 19, 2023

Choose a reason for hiding this comment

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

Handle defaults never touches the last argument anyway, so this will only become relevant if we add another argument here.

Also, on named arguments, my total refactor for async support migrates to using named arguments so we don't need the ugly handle_defaults stuff anymore.

Copy link
Collaborator

@stevenroose stevenroose Oct 19, 2023

Choose a reason for hiding this comment

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

To answer Andrew's concern:

I'm a bit confused about the goal of this line. You are setting default_descriptors to whatever its default value would be ... so why provide it at all?

He's only setting the default value that is filled if the actual value is not set.

self.call(
"createwallet",
handle_defaults(
&mut args,
&[
false.into(),
false.into(),
into_json("")?,
false.into(),
default_descriptors.into(),
],
),
)
}
}

fn list_wallets(&self) -> Result<Vec<String>> {
Expand Down
5 changes: 5 additions & 0 deletions client/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ pub enum Error {
UnexpectedStructure,
/// The daemon returned an error string.
ReturnedError(String),
/// The {0} RPC does not support the {1} argument on the connected bitcoin version.
UnsupportedArgument(&'static str, &'static str),
}

impl From<jsonrpc::error::Error> for Error {
Expand Down Expand Up @@ -88,6 +90,9 @@ impl fmt::Display for Error {
Error::InvalidCookieFile => write!(f, "invalid cookie file"),
Error::UnexpectedStructure => write!(f, "the JSON result had an unexpected structure"),
Error::ReturnedError(ref s) => write!(f, "the daemon returned an error string: {}", s),
Error::UnsupportedArgument(rpc, arg) => {
write!(f, "the daemon version does not support the {} argument for {}", arg, rpc)
}
}
}
}
Expand Down
15 changes: 11 additions & 4 deletions integration_test/src/main.rs
View file
Open in desktop
Copy link

@lateminer lateminer Nov 20, 2023

Choose a reason for hiding this comment

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

On L1154 is needed to add a check whether it is a descriptor wallet, as only legacy wallets have HD seeds.

For example:
let has_hd_seed = has_private_keys && !wallet_param.blank.unwrap_or(false) && !wallet_param.descriptor.unwrap_or(true);

Copy link

@lateminer lateminer Nov 20, 2023

Choose a reason for hiding this comment

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

It makes sense to add an assertion in test_create_wallet():

        let has_descriptors = wallet_param.descriptor.unwrap_or(true);
        assert_eq!(wallet_info.descriptors.unwrap_or(true), has_descriptors);

(It will be also required to add descriptors field in GetWalletInfoResult struct).

Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ fn main() {
unsafe { VERSION = cl.version().unwrap() };
println!("Version: {}", version());

cl.create_wallet("testwallet", None, None, None, None).unwrap();
cl.create_wallet("testwallet", None, None, None, None, None).unwrap();

test_get_mining_info(&cl);
test_get_blockchain_info(&cl);
Expand Down Expand Up @@ -1074,6 +1074,7 @@ fn test_create_wallet(cl: &Client) {
blank: Option<bool>,
passphrase: Option<&'a str>,
avoid_reuse: Option<bool>,
descriptor: Option<bool>,
}

let mut wallet_params = vec![
Expand All @@ -1083,20 +1084,23 @@ fn test_create_wallet(cl: &Client) {
blank: None,
passphrase: None,
avoid_reuse: None,
descriptor: None,
},
WalletParams {
name: wallet_names[1],
disable_private_keys: Some(true),
blank: None,
passphrase: None,
avoid_reuse: None,
descriptor: None,
},
WalletParams {
name: wallet_names[2],
disable_private_keys: None,
blank: Some(true),
passphrase: None,
avoid_reuse: None,
descriptor: None,
},
];

Expand All @@ -1107,13 +1111,15 @@ fn test_create_wallet(cl: &Client) {
blank: None,
passphrase: Some("pass"),
avoid_reuse: None,
descriptor: None,
});
wallet_params.push(WalletParams {
name: wallet_names[4],
disable_private_keys: None,
blank: None,
passphrase: None,
avoid_reuse: Some(true),
descriptor: Some(false),
});
}

Expand All @@ -1125,6 +1131,7 @@ fn test_create_wallet(cl: &Client) {
wallet_param.blank,
wallet_param.passphrase,
wallet_param.avoid_reuse,
wallet_param.descriptor,
)
.unwrap();

Expand Down Expand Up @@ -1278,7 +1285,7 @@ fn test_getblocktemplate(cl: &Client) {
}

fn test_unloadwallet(cl: &Client) {
cl.create_wallet("testunloadwallet", None, None, None, None).unwrap();
cl.create_wallet("testunloadwallet", None, None, None, None, None).unwrap();

let res = new_wallet_client("testunloadwallet")
.unload_wallet(None)
Expand All @@ -1296,7 +1303,7 @@ fn test_loadwallet(_: &Client) {
let wallet_client = new_wallet_client(wallet_name);

assert!(wallet_client.load_wallet(wallet_name).is_err());
wallet_client.create_wallet(wallet_name, None, None, None, None).unwrap();
wallet_client.create_wallet(wallet_name, None, None, None, None, None).unwrap();
assert!(wallet_client.load_wallet(wallet_name).is_err());
wallet_client.unload_wallet(None).unwrap();

Expand All @@ -1311,7 +1318,7 @@ fn test_backupwallet(_: &Client) {

assert!(wallet_client.backup_wallet(None).is_err());
assert!(wallet_client.backup_wallet(Some(&backup_path)).is_err());
wallet_client.create_wallet("testbackupwallet", None, None, None, None).unwrap();
wallet_client.create_wallet("testbackupwallet", None, None, None, None, None).unwrap();
assert!(wallet_client.backup_wallet(None).is_err());
assert!(wallet_client.backup_wallet(Some(&backup_path)).is_ok());
}
Expand Down