-
Notifications
You must be signed in to change notification settings - Fork 405
docs(chain): add doctest for min confirmation balance filtering #2007
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
docs(chain): add doctest for min confirmation balance filtering #2007
Conversation
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.
Thank you for working on this.
I do not think modifying the canonicalization is the right approach.
For example, if we have a min-confirmation-count of 3. We expect that a transaction confirmed at depth 2 should be prioritized over a transaction with no confirmations. With the current state of the PR, this guarantee is nonexistent.
Additionally, different min-confirmation-count values should not influence the Balance::total
value. It should only shift values between *pending,immature,confirmed
.
Idea 1
The simplest solution would be to not do any code changes and provide a doctest showing how we could manipulate the chain_tip
input of TxGraph::[try_]balance
. I.e. with LocalChain
/CheckPoint
s.
let tip = chain.tip();
let target_height = tip.height().saturating_sub(confirmation_threshold);
let target_tip = tip
.range(..=target_height)
.next()
.expect("checkpoint from local chain must have genesis");
// Use `target_tip` as the `chain_tip` input of `TxGraph::[try_]balance`.
For bdk_wallet::Wallet
, we should probably provide a convenience method of sorts so the caller does not need to dig into the internal structures.
Idea 2
We can change the trust_predicate
to be a more general predicate
.
pub fn balance(
/* Other Inputs */
// predicate inputs: (txout_meta, txout, tip_height)
mut predicate: impl FnMut(&OI, FullTxOut<A>, u32) -> BalanceType,
) -> Balance { todo!() }
// Maybe put this in `bdk_core`?
pub enum BalanceType {
Immature,
TrustedPending,
UntrustedPending,
Confirmed,
}
However, this is a breaking change and it makes it more involved to implement the predicate
.
Let me know what you think.
Thanks for the suggestions. I like Idea 1, and will work on implementing this. |
ad19250
to
1bc9534
Compare
1bc9534
to
2c6c10b
Compare
2c6c10b
to
657a044
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.
ACK 657a044
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.
ACK 657a044
Implements #1942.
Description
This PR adds a new doctest demonstrating how to simulate a minimum confirmation threshold for confirmed balance calculations by adjusting the
chain_tip
height passed toTxGraph::balance
.Changelog notice
chain_tip
height.Checklists
All Submissions:
New Features:
Bugfixes: