Migrate Psbt to pro-macros#736
Conversation
|
Here is an example of what I mean by rust specific terminology in our jvm doc
|
| use bdk_wallet::bitcoin::consensus::encode::serialize; | ||
| use bdk_wallet::bitcoin::consensus::Decodable; | ||
| use bdk_wallet::bitcoin::io::Cursor; | ||
| use bdk_wallet::bitcoin::psbt::ExtractTxError; |
There was a problem hiding this comment.
This caused issues when I was testing after migrating to pro-marcos in this pr. Was there a reason why we were not using ExtractTxError from the error crate like the rest of the errors?
I updated this to how we use the other errors like PsbtError, PsbtParseError and the issue was resolved.
There was a problem hiding this comment.
No this was totally an oversight! Thanks for the fix.
thunderbiscuit
left a comment
thunderbiscuit
left a comment
There was a problem hiding this comment.
A few small tweaks but this is looking good!
| use bdk_wallet::bitcoin::consensus::encode::serialize; | ||
| use bdk_wallet::bitcoin::consensus::Decodable; | ||
| use bdk_wallet::bitcoin::io::Cursor; | ||
| use bdk_wallet::bitcoin::psbt::ExtractTxError; |
There was a problem hiding this comment.
No this was totally an oversight! Thanks for the fix.
| Ok(Psbt(Mutex::new(psbt))) | ||
| } | ||
|
|
||
| /// Serialize the PSBT into a writer. |
There was a problem hiding this comment.
How do you feel about this line instead? It feels like it would match your doc from the constructor above.
Serialize the PSBT into a base64-encoded string.
| Ok(Arc::new(Psbt(Mutex::new(original_psbt)))) | ||
| } | ||
|
|
||
| /// Finalizes the current PSBT (Partially Signed Bitcoin Transaction) and produces a result indicating |
There was a problem hiding this comment.
I would remove the "(Partially Signed Bitcoin Transaction)" here and on the json_serialize() method docs, since we're in the type itself and so there is little chance of confusion.
|
Agreed that the docs are not always clear and appropriate. It's a bit up in the air what our policy with those are at the moment; in some cases you'd want the docs to be a little different between languages, which is not possible. In other cases the Rust docs clearly refer to things that don't exist in the ffi layer (in which case I have taken the liberty of removing those lines). We should talk about this over the next dev call. For now we've been trying to just copy/paste the Rust docs, but some cleanup might be required. |
|
My bad! After looking into an odd cargo warning, I realize that the FinalizedPsbtResult should use https://mozilla.github.io/uniffi-rs/latest/proc_macro/records.html |
Wow! Nice catch. So structs with named fields should use |
|
Yes exactly. You either get fields or methods (one of the quirks of uniffi). This is why we have methods on Transaction that are actually just fields in Rust, because we cannot have both, and in cases where Rust uses both we expose the fields through getters. |
|
Ok I will create pr for the fix now |
|
or would you rather do this in your pr? @thunderbiscuit |
|
No feel free to open a PR for it! |
2 participants
Notes to the reviewers
json_serializeandfinalizedo not have direct mapping doc in rust api docs. I wrote my own doc comments there.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: