update sendtoaddress for 0.21 #204
Conversation
| &["".into(), "".into(), false.into(), false.into(), 6.into(), null()], | ||
| &["".into(), "".into(), false.into(), false.into(), null(), null(), null(), null()], |
There was a problem hiding this comment.
I guess 6.into() is the confirmation target, is there a reason that was changed?
There was a problem hiding this comment.
According to the docs, the node uses the default value of wallet -txconfirmtarget, this is something each node operator will set on its own, when you are passing 6 as the default, you are overriding that configuration. I am passing null() here so that we don't send any value with the RPC command and the node therefore uses whatever it is configured as default internally to the node, without us having to tell it.
There was a problem hiding this comment.
If this is the case this change should probably be done as a separate patch (at the start of the PR) with full justification. This means next time someone is debugging they can use git blame on this line to see the commit that last touched this line and then check that commit to see why we no longer pass in 6.
Sorry for such a delayed response, I was not focused on this crate for along time.
| @@ -121,9 +121,6 @@ fn handle_defaults<'a, 'b>( | |||
| let defaults_i = defaults.len() - 1 - i; | |||
| if args[args_i] == serde_json::Value::Null { | |||
| if first_non_null_optional_idx.is_some() { | |||
| if defaults[defaults_i] == serde_json::Value::Null { | |||
| panic!("Missing `default` for argument idx {}", args_i); | |||
| } | |||
There was a problem hiding this comment.
What is the reason that this is this removed?
There was a problem hiding this comment.
Hey, thanks for reviewing the PR :) I removed this so that we can pass null() to the rpc commands and therefore make the node use the default configuration created internally by the node.
There was a problem hiding this comment.
I'm only newish to this crate but I'm almost certain we cannot remove this check, doing so changes the behaviour of the whole crate.
integration_test/run.sh
Outdated
| @@ -19,12 +19,12 @@ PID1=$! | |||
| sleep 3 | |||
|
|
|||
| BLOCKFILTERARG="" | |||
| if bitcoind -version | grep -q "v0\.\(19\|2\)"; then | |||
| if bitcoind -version | grep -q "v\\(19\|20\|21\|22\)"; then | |||
There was a problem hiding this comment.
The readme says we only support 0.21.0 and this PR title says its for 0.21 so we probably shouldn't include 22 here. That means you could leave this change out completely, right?
There was a problem hiding this comment.
@tcharding I removed the 22 from the PR, but this was precisely what fixed the integration tests for Bitcoin Core 22. Should I keep this in will this be added in another PR?
There was a problem hiding this comment.
If you are testing against v22 then the title of this PR is likely wrong.
integration_test/run.sh
Outdated
| BLOCKFILTERARG="-blockfilterindex=1" | ||
| fi | ||
|
|
||
| FALLBACKFEEARG="" | ||
| if bitcoind -version | grep -q "v0\.2"; then | ||
| if bitcoind -version | grep -q "v\\(20\|21\|22\)"; then |
| @@ -362,17 +362,38 @@ fn test_set_label(cl: &Client) { | |||
| fn test_send_to_address(cl: &Client) { | |||
There was a problem hiding this comment.
Perhaps we could skip formatting for this?
#[rustfmt::skip]
fn test_send_to_address(cl: &Client) {
let addr = cl.get_new_address(None, None).unwrap();
let est = json::EstimateMode::Conservative;
let _ = cl.send_to_address(&addr, btc(1), Some("cc"), None, None, None, None, None, None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, Some("tt"), None, None, None, None, None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, Some(true), None, None, None, None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, None, Some(true), None, None, None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, None, None, Some(3), None, None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, None, None, None, Some(est), None, None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, None, None, None, None, Some(false), None).unwrap();
let _ = cl.send_to_address(&addr, btc(1), None, None, None, None, None, None, None, Some(5)).unwrap();
let _ =cl.send_to_address(&addr, btc(1), None, None, None, None, None, None, None, None).unwrap();
}
json/src/lib.rs
Outdated
| @@ -835,7 +835,7 @@ impl<'a> serde::Serialize for ImportMultiRequestScriptPubkey<'a> { | |||
| #[derive(Serialize)] | |||
| struct Tmp<'a> { | |||
| pub address: &'a Address, | |||
| }; | |||
| } | |||
There was a problem hiding this comment.
This change is unrelated to this PR, perhaps it should be left out or done as a separate patch explaining that its just a clippy fix.
|
Also, I can verify that this PR fixes the |
luizParreira
force-pushed
the
update-sendtoaddress-for-0.21
branch
from
f0ae3d5
to
0a0c11e
Compare
This PR adds the option of specifying the
fee_rateonsendtoaddress. I am not sure you all want this integrated into the library, since this will probably break if you are using the latest version of the library with the older bitcoin versions.In addition to adding this extra option, I changed the handle_defaults function to allow us to pass a
null()as default. This was needed so that we can passnull()as argument to commands and use the default version that the node uses.Also, I had to change a bit of the regex to identify the current bitcoind version, so that when running v22 of bitcoind, the tests will run as expected.
How can I run the full test suite to validate whether this is breaking older versions?