Conversation
| checkpoint: HashCheckpoint::from_genesis(network), | ||
| } | ||
| } | ||
| RecoveryPoint::Other { birthday } => bdk_kyoto::ScanType::Recovery { |
There was a problem hiding this comment.
Could we add a regression test using RecoveryPoint::Other on a non-mainnet wallet and assert that the resulting checkpoint preserves the supplied birthday? This exact branch was previously bypassed for every non-mainnet network, and the current test suite would not catch that behavior returning.
There was a problem hiding this comment.
Yeah I considered this when I was writing the code because I agree it'd be strong regression coverage, and actually did it at first before I changed to whats currently in the PR. I opted to change it to what I have now because previously the cleanest unit test approach required extracting the mapping into a private helper solely to create a test seam, so I wasn’t loving that because I couldn’t find any real similar patterns of that here since conversions are generally handled through From/TryFrom. And then testing CbfBuilder::build directly would instead require running a Kyoto node with a loopback peer because the configured checkpoint is private upstream, etc. So what I preferred was what I ended up with but happy to see how you feel still and what others think, totally open to it if we feel like the tradeoff makes more sense.
There was a problem hiding this comment.
Thanks, that makes sense given CbfBuilder::build does not expose the configured checkpoint. I still think a small pure mapping helper would be justified here because it isolates this network-sensitive conversion and would let us regression-test the exact branch, but I don’t consider that blocking for this focused fix.
|
Looks good to me |
rustaceanrob
left a comment
rustaceanrob
left a comment
There was a problem hiding this comment.
Seems fine to go in without a dedicated test. The match is straightforward and this only effects test networks.
thunderbiscuit
left a comment
thunderbiscuit
left a comment
There was a problem hiding this comment.
ACK 9155ecd. Please rebase and merge.
4 participants
Description
Honors explicit Kyoto recovery birthdays on every network (previously we had non mainnet birthdays fall back to genesis)
We had a non mainnet guard that predated
RecoveryPoint::Otherso I thought this might just be an oversight to fix now, but let me know if I'm wrong!Notes to the reviewers
Documentation
bdk_walletbitcoinuniffiChangelog
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingchangelog:*labelNew Features:
Bugfixes: