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