-
Notifications
You must be signed in to change notification settings - Fork 61
feat: Add Input for PSBT #846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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 |
9e2a1b5
to
420f0ff
Compare
This is ready for review |
420f0ff
to
f1fa7be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bdk-ffi/src/bitcoin.rs
Outdated
pub struct PartialSig { | ||
pub pubkey: String, | ||
pub signature: Vec<u8>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for these.
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 |
f1fa7be
to
032a6c8
Compare
Mind rebasing this @ItoroD? I think it's ready to go. |
032a6c8
to
00b6014
Compare
bdk-ffi/src/bitcoin.rs
Outdated
#[derive(Clone, Debug, uniffi::Record)] | ||
pub struct PartialSig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I think this type can now be removed.
This allows easier access to the inputs of our PSBT type
00b6014
to
e3fd002
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad I went over it too quickly earlier.
pub tap_script_sigs: Vec<TapScriptSig>, | ||
/// Map of Control blocks to Script version pair. | ||
pub tap_scripts: Vec<TapScriptEntry>, | ||
/// Map of tap root x only keys to origin info and leaf hashes contained in it. | ||
pub tap_key_origins: Vec<TapKeyOrigin>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 types can be made HashMaps as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yh no worries. I had actually done these locally but i though you wanted them excluded intentionally because of the nature of the key.
tap_script_sigs is a concatenation of xonlypubkey and leafhash
<xonlypubkey>|<leafhash>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd keep the tuple (XOnlyPublicKey, TapLeafHash) as a struct similar to what you have above, but use that as my key on the HashMap, akin to the BTreeMap they have in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
pub proprietary: Vec<ProprietaryKeyValuePair>, | ||
/// Unknown key-value pairs for this input. | ||
pub unknown: Vec<KeyValuePair>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And these last two as well.
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 fmt
andcargo clippy
before committingNew Features: