Add missing Listen/Readable/methods for OutputSweeperSync and drop non-BestBlock Readable impl #4131
Conversation
It appears we just forgot to add these when we added the sync wrapper.
`OutputSweeper`, like any `Confirm`/`Listen` client, needs to have the chain synced back up on startup. Thus, like `ChannelMonitor` and `ChannelManager`, we force users to `read` it via a method that gives them the latest chain tip so that they can sync.
|
👋 I see @valentinewallace was un-assigned. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4131 +/- ##
==========================================
- Coverage 88.63% 88.61% -0.02%
==========================================
Files 180 180
Lines 134878 134869 -9
Branches 134878 134869 -9
==========================================
- Hits 119543 119520 -23
- Misses 12567 12581 +14
Partials 2768 2768
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| impl<B: Deref, D: Deref, E: Deref, F: Deref, K: Deref, L: Deref, O: Deref> | ||
| ReadableArgs<(B, E, Option<F>, O, D, K, L)> for OutputSweeper<B, D, E, F, K, L, O> |
There was a problem hiding this comment.
Huh, no, why are we removing this? How do we read an OutputSweeper only then?
For Confirm we don't need to handle the best block as we're not walking the chain anyways.
There was a problem hiding this comment.
Also note that for example in LDK Node we can't currently deal with the best block at time of initialization anyways, so we'll just call OutputSweeper::current_best_block when we're ready to start syncing. IMO, this API change doesn't 'force' anybody, it makes it just weirder/harder to use.
There was a problem hiding this comment.
Huh, no, why are we removing this? How do we read an OutputSweeper only then?
Its not that much effort to read a pair and then just take the second element.
Also note that for example in LDK Node we can't currently deal with the best block at time of initialization anyways, so we'll just call OutputSweeper::current_best_block when we're ready to start syncing. IMO, this API change doesn't 'force' anybody, it makes it just weirder/harder to use.
Define "ready to start syncing"? Like other LDK structs, ISTM OutputSweeper syncing should happen prior to normal operation. I'm a bit confused why this API is fine for ChannelMonitor/ChannelManager and not fine for OutputSweeper - they have largely the same restrictions/init process.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
It appears we just forgot to add these when we added the sync
wrapper.