Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2321 +/- ##
==========================================
+ Coverage 78.84% 79.04% +0.20%
==========================================
Files 31 31
Lines 6060 6066 +6
Branches 288 289 +1
==========================================
+ Hits 4778 4795 +17
+ Misses 1203 1192 -11
Partials 79 79
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:
|
| /// A settled output for which `is_locked` returns true is classified `Locked` and counted in | ||
| /// `Balance::locked` instead of `confirmed`. | ||
| #[test] | ||
| fn test_classify_locked() { |
There was a problem hiding this comment.
I would like a test with script introspection, i.e., verifying the actual use of OP_CSV or OP_CLTV. We cannot know is something else is needed from the graph if we only use the is_locked function as a boolean constant.
There was a problem hiding this comment.
Yes, would be nice to have them. Should I add them as a doctest? is_locked only works as a boolean here, so tests using different kinds of timelocks would probably fit better on the bdk_wallet side.
There was a problem hiding this comment.
Why as a doctest? I think implementing them as a test is better. I would simulate a wallet here if needed, and test it here. This is a more complex unit test, but unit test in the end, it shouldn't live in a downstream dependency.
There was a problem hiding this comment.
I thought on doing some doctests as it would help callers to implement their balances with timelocks, and I was only thinking of the usage of is_locked as it is. But if some complex things have to be done on the callers side, as you said, maybe there's something else needed from the graph.I'll add them as a test and check.
There was a problem hiding this comment.
Solved in 698d67a.
I think it's much clearer now how is_locked should be called for these cases.
I'm sure more complex timelock constructions would require a different is_locked signature, maybe the whole descriptor. I'm not sure whether adding coverage for those cases here would provide additional value or just introduce redundancy.
I'm not sure how we could achieve the time based timelocks, maybe the evaluation of the mtp should be done in chain?
Add an `Eligibility::Locked` variant and a `Balance::locked` field, plus an `is_locked` predicate on `classify_outpoints`/`balance` so the caller marks confirmed outputs whose timelock has not matured yet.
c04bb52 to
833100b
Compare
833100b to
698d67a
Compare
Description
Adds
Eligibility::LockedandBalance::lockedfor confirmed outputs whose timelock hasn't matured yet, so they aren't counted as spendableconfirmedbalance.classify_outpointsandbalancetake a newis_lockedpredicate, evaluated only on settled outputs.Notes to the reviewers
The timelock lives in the output's descriptor, so
CanonicalViewcan't evaluate it from chain data alone. The caller passesis_lockedinstead, the same way it passesdoes_taint. The wallet side will follow in a separatebdk_walletPR.This category is only meant for timelocked (CSV, CLTV) coins. Other locked coins that aren't enforced by consensus shouldn't use it, since
is_lockedis only evaluated on settled outputs. Frozen or reserved coins are expected to be handled in the wallet.Changelog notice
Breaking:
CanonicalView::classify_outpointsandbalancetake a newis_lockedpredicate. AddedEligibility::LockedandBalance::locked.Checklists
All Submissions:
New Features:
Bugfixes: