Make add_recipient() take any ScriptPubKey

[thunderbiscuit]

Fixes #159.

Changelog notice

Breaking Changes:
    - `TxBuilder.add_recipient()` now takes a `Script` instead of an `Address` [#192]
    - `AddressAmount` is now `ScriptAmount` [#192]

New Structs:
    - `Address` and `Script` structs have been added

New features:
  - Add `PartiallySignedBitcoinTransaction.extract_tx()` function.

[#192](https://github.com/bitcoindevkit/bdk-ffi/pull/192)

To do:

• Inline documentation
• Clean up commits

[thunderbiscuit]

The new error is coming from the ffi layer:

error[E0308]: mismatched types
    --> /Users/user/bdk-ffi/target/debug/build/bdk-ffi-e89d1caffbe2e3c9/out/bdk.uniffi.rs:1491:24
     |
1491 |             Ok(val) => val,
     |                        ^^^ expected struct `Script`, found struct `std::string::String`

I can see how it relates to the upgrade: the TxBuilder used to take a String for the address and now takes a script. But at the same time I think I've fixed all Rust code to migrate... not sure where this string is getting delivered from.

[tnull]

From a first look the approach seems reasonable to me. In fact I hand no issue building? I assume above comment regarding a build error has already been addressed?

[thunderbiscuit]

Oh shoot! Yes Steve and I fixed the problems mentioned above and I forgot to say it here to avoid confusion. Sorry man!

[tnull]

Generally LGTM, just a few nits.

[tnull]

nit: Can be removed.

Suggested change

// use bitcoin::hashes::serde::Serialize;

[thunderbiscuit]

@tnull I agree that the script.script is rather verbose. I tried using the tuple struct to refactor and it appears to work, but because we need to access the internals of the tuple struct we end up having to use script.0.clone() (unless there is another way to do this I'm not seeing?). I don't have a strong opinion either way, it's mostly replacing the script.script by script.0.

Here are what the methods look like in the TxBuilder:

fn add_recipient(&self, script: Arc<Script>, amount: u64) -> Arc<Self> {
    let mut recipients: Vec<(BdkScript, u64)> = self.recipients.clone();
    recipients.append(&mut vec![(script.0.clone(), amount)]);
    Arc::new(TxBuilder {
        recipients,
        ..self.clone()
    })
}

fn set_recipients(&self, recipients: Vec<ScriptAmount>) -> Arc<Self> {
    let recipients = recipients
        .iter()
        .map(|script_amount| (script_amount.script.0.clone(), script_amount.amount))
        .collect();
    Arc::new(TxBuilder {
        recipients,
        ..self.clone()
    })
}

[notmandatory]

If ready to review/merge you should remove the part of the description about it totally not working. Also update changelog with:

New features:

• Add PartiallySignedBitcoinTransaction.extract_tx() function.

[thunderbiscuit]

Done and done. @notmandatory do you have an opinion as to whether to use the tuple struct or the normal struct? I'm ok with both, but we should be consistent if we use the tuple struct and also apply it elsewhere we have a wrapper with only one field.

I wonder if it makes the code less explicit just because you end up using numbers instead the wrapper/field, but it's not a big deal either way.

[tnull]

@tnull I agree that the script.script is rather verbose. I tried using the tuple struct to refactor and it appears to work, but because we need to access the internals of the tuple struct we end up having to use script.0.clone() (unless there is another way to do this I'm not seeing?). I don't have a strong opinion either way, it's mostly replacing the script.script by script.0.

Right, I personally prefer the shorter for of the definition and script.0 over script.script, but don't have strong feelings about it either. So feel free to leave as is.

[thunderbiscuit]

As per a discussion in the dev call we'll be merging this PR without using the Tuple Structs and leave the migration to the Tuple Structs to a separate PR (since there are a few different places where these refactorings should be applied).

[notmandatory]

ACK 7aafc64

But prior to merging please remove the dbg!.