feat: Add Input for PSBT #846
Conversation
|
Anything else you'd like to add to this PR or should we start reviewing now? No rush, just want to know where it's at. |
A few more fields to add before this is good for review |
ItoroD
force-pushed
the
expose-psbt-input
branch
from
9e2a1b5 to
420f0ff
Compare
|
This is ready for review |
ItoroD
force-pushed
the
expose-psbt-input
branch
from
420f0ff to
f1fa7be
Compare
There was a problem hiding this comment.
It's looking good!
I think a few of the Vec could be simply HashMap<K, V> as in the rust-bitcoin codebase, unless there is something I'm missing.
It's a lot of conversion between simple types, so I'm wondering if stealing one or two of the rust-bitcoin tests on this type would be good as well? I haven't looked at their tests so don't take that as a requirement. I don't know if they actually do a lot of tests just on this Input type.
| pub struct PartialSig { | ||
| pub pubkey: String, | ||
| pub signature: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
Since the rust-bitcoin type for this field is a map, I think we could use the uniffi equivalent here instead of a vector of structs. Let me know if you've tried this and it doesn't work for some reason!
There was a problem hiding this comment.
Yes, you are correct. I struggled with trying to use BTreeMap in a record this made me think maps are not just one of the Uniffi compatible type. HashMap is definitely closer and it works. I just checked. I did
pub bip32_derivation: HashMap<String, KeySource>
instead of
pub bip32_derivation: Vec<Bip32Derivation>
where keysource is
#[derive(Clone, Debug, uniffi::Record)]
pub struct KeySource {
pub fingerprint: String,
pub path: String,
}
There was a problem hiding this comment.
I was thinking about this again, the only thing about collecting these into Hashmap is that we loose the ordering that the BTreeMap gave from the actual rust-bitcoin types. Should we care about the ordering ?
There was a problem hiding this comment.
Yeah I wondered that too. But looking at the fields, I do think the keys identify everything you need, and so the ordering doesn't matter. If anything devs would need an extra line of lookup to find the associated data instead of relying on the ordering, but even then I don't know if that's even how people use the BTreeMap for these fields. My hunch would be to use the HashMap.
There was a problem hiding this comment.
Alright, I will update. Thanks.
bdk-ffi/src/bitcoin.rs
Outdated
| pub struct Bip32Derivation { | ||
| /// hex-encoded public key (serialized) | ||
| pub pubkey: String, | ||
| /// master key fingerprint | ||
| pub fingerprint: String, | ||
| /// derivation path segments | ||
| pub path: String, | ||
| } |
There was a problem hiding this comment.
Same as above. We might be able to use a map instead of a Vec of this struct.
There was a problem hiding this comment.
Thanks for pointing these out. BTreeMap not working threw me off. But I see now
bdk-ffi/src/bitcoin.rs
Outdated
| pub ripemd160_preimages: Vec<PreimagePair>, | ||
| /// SHA256 hash to preimage map. | ||
| pub sha256_preimages: Vec<PreimagePair>, | ||
| /// HASH160 hash to preimage map. | ||
| pub hash160_preimages: Vec<PreimagePair>, | ||
| /// HASH256 hash to preimage map. | ||
| pub hash256_preimages: Vec<PreimagePair>, |
Thanks
They do have a few test that concern input. Though they are not direct tests. The intention of some of the test are to validate correctness of an action performed, like signing. See here I am sure I can squeeze out a few good test like this from there. Will be a great idea to have them! An additional thing I will have to perform is ensure whatever Psbt sample I create/use has values for the fields I will be checking/testing. Might take a while to do that for all fields. (Simulating different Psbt states, signed, unsigned, partially signed, finalized ). I could start with a few. |
|
Just an update on this. I will be pushing new changes today with some tests as well |
This allows easier access to the inputs of our PSBT type
ItoroD
force-pushed
the
expose-psbt-input
branch
from
f1fa7be to
032a6c8
Compare
Description
Since signing will soon be done with the PSBT type, I think its a good time to expose some of the other PSBT fields like
Input.This will allow easier access to the input in a PSBT. It might be specifically important because the sign method of PSBT can do interesting stuff like returning a map of input index and keys used to sign it.
I would like to take a pause here and see how this is looking on the bindings side before I go any further.
Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: