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.

Sign up for GitHub

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

Jump to bottom

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

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

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

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

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

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

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

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

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
Loading