fix(types): use f64 for DecodePsbt fee field

[deadmanoz]

As per raised in #429, addressing type discrepancies between corepc-types and Bitcoin Core

Bitcoin Core returns the fee field in the decodepsbt RPC response as a floating-point (double) BTC value, not as satoshis (integer). The current implementation uses u64 for the fee field and converts it with Amount::from_sat(), which causes a deserialisation failure.

Minimal repro with a PSBT from Bitcoin Core's functional tests.

https://github.com/bitcoin/bitcoin/blob/13891a8a685d255cb13dd5018e3d5ccc18b07c34/test/functional/data/rpc_psbt.json#L96

use corepc_client::client_sync::v28::Client;
use corepc_client::client_sync::Auth;
use corepc_client::types::v28::DecodePsbt;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    // First valid PSBT from Bitcoin Core's test/functional/data/rpc_psbt.json (valid[0]).
    let psbt = concat!(
        "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////",
        "AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxH",
        "BQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/",
        "hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6Uwpy",
        "N+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLr",
        "CwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m8",
        "0GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc",
        "0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/j",
        "nF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4O",
        "FyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkz",
        "gHNEZPhPKrMAAAAAAAAA",
    );

    let auth = Auth::UserPass("bitcoin".to_string(), "bitcoin".to_string());
    let client = Client::new_with_auth("http://127.0.0.1:8332", auth)?;
    let decoded: DecodePsbt = client.call("decodepsbt", &[serde_json::json!(psbt)])?;
    println!("fee: {:?}", decoded.fee);
    Ok(())
}

Error: JsonRpc(Json(Error("invalid type: floating point `3.01e-6`, expected u64", line: 1, column: 2896)))

This change:

• Changes the fee field type from u64 to f64 in DecodePsbt struct (as required in v17, v23, v24, v30)
• Updates the conversion to use Amount::from_btc() instead of Amount::from_sat()
• Adds a Fee(ParseAmountError) error variant to DecodePsbtError for proper error handling of the fee
• Adds an integration test to verify correct deserialisation

[jamillambert]

Nice fix.

The CI failure is just a missing feature gate on the assert_eq!(json.psbt_version... which only exists for v23 and above.

Could you fix the existing test rather than adding a second one? or rename the two tests to clearly state what they are each testing, so it's clear when one fails and the other passes what the issue is.

If you combine them in this case or for other RPCs make sure you keep the existing syntax that states the error type when converting to the modelled type.
let model: Result<mtype::DecodePsbt, DecodePsbtError> = json.into_model();

[deadmanoz]

Nice fix.

The CI failure is just a missing feature gate on the assert_eq!(json.psbt_version... which only exists for v23 and above.

Could you fix the existing test rather than adding a second one? or rename the two tests to clearly state what they are each testing, so it's clear when one fails and the other passes what the issue is.

If you combine them in this case or for other RPCs make sure you keep the existing syntax that states the error type when converting to the modelled type. let model: Result<mtype::DecodePsbt, DecodePsbtError> = json.into_model();

Thank you for great feedback.

I obviously had insufficient local testing, this spurred me to setup some local CI infrastructure so I can avoid so many CI failures in the future.

I will combine with the existing test

[jamillambert]

I obviously had insufficient local testing, this spurred me to setup some local CI infrastructure so I can avoid so many CI failures in the future.

I find that testing the modified test in v17, the version you made the change in, and v30 catches most problems. Or if there is a version feature gate the versions on either side. Saves running all version tests.

[deadmanoz]

Ok so I opted to make the test handle the various changes to the decodepsbt over the various Bitcoin Core versions.

All except the MuSig2 changes in v30, I don't know enough about this TBH.

Edit: I tested locally across 18 versions, all passing:

• v30.0
• v29.0
• v28.0, v28.1, v28.2
• v27.0, v27.1, v27.2
• v26.2
• v25.2
• v24.2
• v23.2
• v22.1
• v0.21.2
• v0.20.2
• v0.19.1
• v0.18.1
• v0.17.2

[jamillambert]

The test is long and confusing. Some of the issues were already there from before this PR:

• The block in the middle with imports, but no feature gate or any reason to be in a block. A separate helper function would be cleaner.
• The comment on the top is unnecessarily long.
• Instead of hard coding the random xpub it could be derived from a fixed seed like in other tests:

    let secp = bitcoin::secp256k1::Secp256k1::new();
    let seed = [0u8; 32];
    let xprv = Xpriv::new_master(Network::Regtest, &seed).unwrap();
    let xpub = Xpub::from_priv(&secp, &xprv);

• Similarly key can be derived, which makes it clearer that it is just a random key and the long hex currently written in the test isn't something specific or important.

@tcharding wrote the original test so there may be a reason for the block and hard coded xpub I have missed?

[deadmanoz]

The test is long and confusing. Some of the issues were already there from before this PR:

• The block in the middle with imports, but no feature gate or any reason to be in a block. A separate helper function would be cleaner.
• The comment on the top is unnecessarily long.
• Instead of hard coding the random xpub it could be derived from a fixed seed like in other tests:

    let secp = bitcoin::secp256k1::Secp256k1::new();
    let seed = [0u8; 32];
    let xprv = Xpriv::new_master(Network::Regtest, &seed).unwrap();
    let xpub = Xpub::from_priv(&secp, &xprv);

• Similarly key can be derived, which makes it clearer that it is just a random key and the long hex currently written in the test isn't something specific or important.

@tcharding wrote the original test so there may be a reason for the block and hard coded xpub I have missed?

Thank you for your valuable review time @jamillambert.

I agree this can be tightened in the manner you describe, I will move to draft to address

[deadmanoz]

Thanks for the review feedback mate. I've addressed the feedback:

• Shortened top comment
• Replaced hardcoded xpub/tap_internal_key with keys derived from a zero seed
• Removed the inline block with imports
• Added shared TestKeys struct and test_keys() in lib.rs (potential for reuse in other tests like wallet__create_wallet_descriptor)

Hoping test is now more concise, yet still covers off the various changes to the RPC method across versions.

I'm open to explicit direction on how to proceed if you'd like further changes. I get the sense there's a desire to uplift test quality across the codebase - happy to help with that broader effort if you can point me in the right direction.

BTW not expecting any fast response this time of year, happy holidays too!

[jamillambert]

I'll have a proper look after Christmas but from first glance it's much improved and I think you can undraft it.
Hope you have a nice holidays too!

[jamillambert]

Can you squash the last two commits?

In the rust-bitcoin org the convention is to change the existing commits in a PR to address review comments rather than adding the fix as a new commit at the end.

The rest looks good though.

[tcharding]

nit: No need for parenthesis, right?

[tcharding]

LGTM, could be squashed into a single patch IMO. Thanks for the contribution man, you are killing it.

reviewed: 604443e

[deadmanoz]

Can you squash the last two commits?

In the rust-bitcoin org the convention is to change the existing commits in a PR to address review comments rather than adding the fix as a new commit at the end.

The rest looks good though.

Sure thing, thanks for the advice

[tcharding]

ACK 3113bf1