CreateWallet update+ListWallets+GetWalletInfo #121

Tibo-lg · 2020-07-30T01:12:14Z

Originally I was only planning on updating create_wallet with the extra parameters (mainly wanted the passphrase), but as I wanted to test that it was working properly I ended up needing list_wallets and get_wallet_info as well.

Some notes:

list_wallets and get_wallet_info are implicitly tested through the create_wallet test, I hope that's ok.
in GetWalletInfoResult I had to make unlocked_until optional even though it is not marked as optional in the docs (https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/getwalletinfo/). I checked manually with bitcoin-cli and I could see that it was not always present, not sure why.
I skipped scanning from the wallet info result because I was not sure how to represent something that can be false or an object.
Made a small refactor to extract get_rpc_url and get_auth in the integration tests so that it can be reused to create clients for the "sub" wallets.
I dont have so much rust experience so apologies in advance if there are some rookie mistakes.

Tibo-lg · 2020-07-30T06:28:08Z

Fixed for version 18 (and also fixed defaults).

justinmoon

Besides my one comment this looks very good!

justinmoon · 2020-07-31T17:17:13Z

json/src/lib.rs

+    #[serde(rename = "hdmasterkeyid")]
+    pub hd_master_key_id: Option<String>,
+    pub private_keys_enabled: bool,
+    pub avoid_reuse: Option<bool>,


Does hd_master_key_id exist? I don't see it in docs or source ...

Seems like avoid_reuse can just be a bool, right?

This implementation is also missing the scanning field:

"scanning" : { (json object) current scanning details, or false if no scan is in progress "duration" : n, (numeric) elapsed seconds since scan start "progress" : n (numeric) scanning progress percentage [0.0, 1.0] }

But I'm not sure how to implement that because it could be either a bool or a struct. Similar scenario to #124 where you have mixed content. We could always add this later, too.

Those are possible to implement, but not easy. I don't know if serde does it automatically if you create an enum. I don't think it does because there is an implied false. I can try do a custom serde implementation after this is merged if you want. (I'll create a ticket for it when we merge this.)

The hdmasterkeyid field seems to have been removed in 0.18. Since we only support 0.18 and 0.19, we shouldn't have it.

Does hd_master_key_id exist? I don't see it in docs or source ...

Thanks, as @stevenroose pointed out it was removed so I deleted it.

Seems like avoid_reuse can just be a bool, right?

It didn't exist in v0.18 which is why I put it as an Option<>, but if there is a better way of handling this let me know (IIRC CI fails for v0.18 if it's not an option).

This implementation is also missing the scanning field:

I was curious so I gave it a try and it seems to work. It cannot be properly tested through CI though because rescanning in regtest is too fast. I gave it a try on testnet and it seems to work as expected. I put it as a separate commit for now, so if you're not happy with the implementation I'll just drop it and someone else can take care of it later. If you're happy with it I can squash it. One downside with this implementation is that it's not really explicit about the fact that the boolean part of the enum can only be false, not sure if there is a better way.

Ah yeah it's unfortunate that it has this bool that is always false. But well. I'm ok with this.

Perhaps add a line of documentation on "NotScanning(bool)" that the bool will always be false?

stevenroose

Nice!

stevenroose · 2020-08-01T15:39:08Z

json/src/lib.rs

+    #[serde(rename = "hdmasterkeyid")]
+    pub hd_master_key_id: Option<String>,
+    pub private_keys_enabled: bool,
+    pub avoid_reuse: Option<bool>,


Those are possible to implement, but not easy. I don't know if serde does it automatically if you create an enum. I don't think it does because there is an implied false. I can try do a custom serde implementation after this is merged if you want. (I'll create a ticket for it when we merge this.)

stevenroose · 2020-08-01T15:41:25Z

integration_test/src/main.rs

+    avoid_reuse: Option<bool>,
+}
+
+fn build_wallet_params<'a>(


This is a little bit of a silly utility method that I'd personally not have created, but I don't bother because it's test stuff :)

Removed, didn't have very strong feelings about it :)

stevenroose · 2020-08-01T15:42:38Z

integration_test/src/main.rs

+
+    assert_eq!(wallet_info.wallet_name, wallet_param.name);
+
+    fn option_to_bool(option: Option<bool>) -> bool {


Isn't this just .unwrap_or(false)?

Yes thanks fixed.

stevenroose · 2020-08-01T15:48:21Z

integration_test/src/main.rs

+    }
+
+    for param in wallet_params {
+        test_create_single_wallet(&cl, param);


This method is only used here, right? I wouldn't mind just having it expanded inside this for loop. Than the WalletParams type can also be made method-local. It's just a suggestion won't be blocking for this.

Expanded in loop and put WalletParams as method-local.

stevenroose · 2020-08-01T15:52:52Z

json/src/lib.rs

+    #[serde(rename = "hdmasterkeyid")]
+    pub hd_master_key_id: Option<String>,
+    pub private_keys_enabled: bool,
+    pub avoid_reuse: Option<bool>,


The hdmasterkeyid field seems to have been removed in 0.18. Since we only support 0.18 and 0.19, we shouldn't have it.

sgeisler · 2020-08-03T19:58:46Z

@stevenroose @Tibo-lg do you consider this PR ready for another round of review?

Tibo-lg · 2020-08-03T23:49:55Z

@sgeisler I think I addressed all the comments so I would say yes. If I missed something let me know.

sgeisler

Looks good, some minor nits, but if someone is annoyed by these there can be a separate PR I guess. We don't need to block on these imo, but feel free to fix them if you want and I'll re-ACK.

sgeisler · 2020-08-06T09:57:30Z

json/src/lib.rs

+    pub keypool_size: usize,
+    #[serde(rename = "keypoolsize_hd_internal")]
+    pub keypool_size_hd_internal: usize,
+    pub unlocked_until: Option<usize>,


Would SystemTime make more sense here?

I double checked but all the timestamps seem to be either usize or u64. Changed to u64 as it might be better and I don't mind changing to SystemTime but I feel it might be inconsistent.

sgeisler · 2020-08-06T09:59:33Z

json/src/lib.rs

+    #[serde(rename = "paytxfee", with = "bitcoin::util::amount::serde::as_btc")]
+    pub pay_tx_fee: Amount,
+    #[serde(rename = "hdseedid")]
+    pub hd_seed_id: Option<String>,


How do we handle binary data elsewhere? Should this be decoded to [u8; 20]?

Thanks for pointing this out, I checked and actually there was already a hd_seed_id field defined for GetAddressInfoResult so I reused the type (bitcoin::XpubIdentifier).

stevenroose

LGTM, I'll add the line of comment.

Tibo-lg force-pushed the create-wallet-options-and-listwallets branch 2 times, most recently from 9548c67 to 3616145 Compare July 30, 2020 06:11

justinmoon reviewed Jul 31, 2020

View reviewed changes

stevenroose requested changes Aug 1, 2020

View reviewed changes

justinmoon mentioned this pull request Aug 2, 2020

Bitcoin RPC Blockchain bitcoindevkit/bdk#22

Closed

Tibo-lg force-pushed the create-wallet-options-and-listwallets branch from 3616145 to 1240252 Compare August 3, 2020 04:24

sgeisler approved these changes Aug 6, 2020

View reviewed changes

Tibo-lg added 2 commits August 6, 2020 21:37

CreateWallet update+ListWallets+GetWalletInfo

1e3935a

Add scanning field to wallet info

0e203cf

Tibo-lg force-pushed the create-wallet-options-and-listwallets branch from 0244b98 to 0e203cf Compare August 6, 2020 12:38

stevenroose approved these changes Aug 23, 2020

View reviewed changes

stevenroose merged commit f394b7a into rust-bitcoin:master Aug 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CreateWallet update+ListWallets+GetWalletInfo #121

CreateWallet update+ListWallets+GetWalletInfo #121

Tibo-lg commented Jul 30, 2020

Tibo-lg commented Jul 30, 2020

justinmoon left a comment

justinmoon Jul 31, 2020

stevenroose Aug 1, 2020

stevenroose Aug 1, 2020

Tibo-lg Aug 3, 2020

stevenroose Aug 23, 2020

stevenroose Aug 23, 2020

stevenroose left a comment

stevenroose Aug 1, 2020

stevenroose Aug 1, 2020

Tibo-lg Aug 3, 2020

stevenroose Aug 1, 2020

Tibo-lg Aug 3, 2020

stevenroose Aug 1, 2020

Tibo-lg Aug 3, 2020

stevenroose Aug 1, 2020

sgeisler commented Aug 3, 2020

Tibo-lg commented Aug 3, 2020

sgeisler left a comment

sgeisler Aug 6, 2020

Tibo-lg Aug 6, 2020

sgeisler Aug 6, 2020

Tibo-lg Aug 6, 2020

stevenroose left a comment


		assert_eq!(wallet_info.wallet_name, wallet_param.name);

		fn option_to_bool(option: Option<bool>) -> bool {

CreateWallet update+ListWallets+GetWalletInfo #121

CreateWallet update+ListWallets+GetWalletInfo #121

Conversation

Tibo-lg commented Jul 30, 2020

Tibo-lg commented Jul 30, 2020

justinmoon left a 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

stevenroose left a 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

sgeisler commented Aug 3, 2020

Tibo-lg commented Aug 3, 2020

sgeisler left a 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

stevenroose left a comment

Choose a reason for hiding this comment