feat: expose detailed try_finalize_psbt outcomes#433
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #433 +/- ##
==========================================
+ Coverage 80.96% 81.15% +0.19%
==========================================
Files 24 24
Lines 5489 5552 +63
Branches 247 247
==========================================
+ Hits 4444 4506 +62
- Misses 968 970 +2
+ Partials 77 76 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c65f534 to
e046adc
Compare
|
Updated per some good feedback from mammal |
f3fb2a6 to
69966b6
Compare
ValuedMammal
left a comment
There was a problem hiding this comment.
I don't think After and Older are doing much good, and would prefer to leave them out of the new implementation if we plan to support try_finalize_psbt into the future.
|
Concept ACK, glad to see this as a non-breaking change. |
|
Added four commits as small follow-ups from review feedback from mammal: make finalization cleanup more explicit, preserve opaque PSBT/input metadata, stop the new |
| /// The input was already finalized before this call. | ||
| AlreadyFinalized, | ||
| /// The input was successfully finalized during this call. | ||
| Finalized, |
There was a problem hiding this comment.
question: are we using this differently ? otherwise, a single Finalized would be enough imho.
There was a problem hiding this comment.
I see where you're coming from for sure but I do think the distinctions intentional. Finalized would be this call was able to finalize the input, AlreadyFinalized would be the input already had final script/witness data before this call and we skipped it. is_finalized() treats both as success, but I thought preserving that difference was useful since this API is meant to explain what happened per input.
There was a problem hiding this comment.
Good, I think I see where you're going. Agree, it's probably useful to have that distinction if it'll be used for upstream users (e.g UX/UI).
|
tACK 09d2cd0 The concept and approach make sense. I fetched the branch and ran the tests locally, and they all pass. The documentation also clearly specifies when to use the finalize_psbt and try_finalize_psbt methods. |
| } | ||
|
|
||
| /// Whether all inputs are finalized after this call. | ||
| pub fn is_finalized(&self) -> bool { |
There was a problem hiding this comment.
If outcomes is empty (e.g. a PSBT with no inputs), is_finalized() returns true, since .all() on an empty iterator is true. Is this the intended behavior or worth documenting/guarding against explicitly?
There was a problem hiding this comment.
I think this is intended since is_finalized() is only answering whether any input failed finalization, so if there are no inputs there is nothing to fail. This also matches the old finalize_psbt behavior where finished started as true and only changed inside the input loop. But that's my take, open to further thoughts
| let mut finished = true; | ||
| let mut outcomes = BTreeMap::new(); | ||
|
|
||
| for (n, input) in tx.input.iter().enumerate() { |
There was a problem hiding this comment.
If psbt.inputs.len() < tx.input.len() but not 0, this loop will partially mutate earlier inputs before hitting the IndexOutOfBoundsError on a later one. Is it expected to have a partially-mutated PSBT on Err , or should length be validated up front before any mutation starts?
There was a problem hiding this comment.
Agree I think validating up front is cleaner here. 6afd006 adds a guard before the loop so a PSBT with missing input metadata returns IndexOutOfBoundsError before any inputs are mutated, plus a regression test for the partial mutation case.
|
|
||
| /// The finalization status for a single PSBT input. | ||
| #[derive(Debug, PartialEq)] | ||
| pub enum FinalizeInputOutcome { |
There was a problem hiding this comment.
Would it make sense to add a Display impl for FinalizeInputOutcome so callers don't have to match on the variants themselves to produce an outcome that can be logged in user interfaces?
There was a problem hiding this comment.
Maybe, but I lean toward leaving this out for now. I was thinking of FinalizeInputOutcome as structured data that callers can match on and render however they want, especially for UI wording, while Debug is still available for basic logging. Happy to add Display if others think that belongs in the initial API though.
Description
First step for #73, following ValuedMammal's suggestion.
This PR keeps the scope to PSBT finalization only:
try_finalize_psbtwhich returnsFinalizePsbtResultFinalizePsbtInputResultNotes to the reviewers
This is now additive rather than breaking:
Wallet::finalize_psbtstill returnsboolIntentionally not included here:
Wallet::signto return a richer result typeChangelog notice
Wallet::try_finalize_psbt, which returnsFinalizePsbtResultand exposes per-input finalization outcomes while preserving the existingWallet::finalize_psbtAPI.Checklists
All Submissions:
just pbefore pushingNew Features: