Remove generic Signer parameter where it can be inferred from KeysInterface
#1806
Conversation
arik-so
force-pushed
the
2022-10-background-processor-deparametrization
branch
from
e5724a5 to
d8082ad
Compare
Codecov ReportBase: 90.72% // Head: 90.68% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1806 +/- ##
==========================================
- Coverage 90.72% 90.68% -0.04%
==========================================
Files 87 87
Lines 47364 47364
Branches 47364 47364
==========================================
- Hits 42970 42953 -17
- Misses 4394 4411 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
lightning-block-sync/src/init.rs
Outdated
| @@ -61,16 +61,16 @@ BlockSourceResult<ValidatedBlockHeader> where B::Target: BlockSource { | |||
| /// | |||
| /// async fn init_sync< | |||
| /// B: BlockSource, | |||
| /// K: KeysInterface<Signer = S>, | |||
| /// K: KeysInterface, | |||
| /// S: keysinterface::Sign, | |||
There was a problem hiding this comment.
Looks like this can be dropped
There was a problem hiding this comment.
that's so odd. This was literally part of my search string; I'm now paranoid that I missed a bunch of other things.
There was a problem hiding this comment.
Maybe the doc comments threw it off.
| @@ -381,7 +380,7 @@ pub async fn process_events_async< | |||
| OMH: 'static + Deref + Send + Sync, | |||
| EH: 'static + EventHandler + Send, | |||
| PS: 'static + Deref + Send, | |||
| M: 'static + Deref<Target = ChainMonitor<Signer, CF, T, F, L, P>> + Send + Sync, | |||
| M: 'static + Deref<Target = ChainMonitor<<K::Target as KeysInterface>::Signer, CF, T, F, L, P>> + Send + Sync, | |||
There was a problem hiding this comment.
Rather than moving to a signer here (and on chain::Watch and Persist and....) let's just parameterize those by KeysInterface directly.
There was a problem hiding this comment.
Should that be part of this PR or in a followup?
There was a problem hiding this comment.
Seems like you should just do that first - it would substantially reduce churn here, no?
There was a problem hiding this comment.
I actually think there's a big difference between removing Signer from types that were parametrized by both Signer and KeysInterface, and reparametrizing types from Signer to KeysInterface. I'd much prefer to do the latter in a follow-up PR, if possible.
There was a problem hiding this comment.
Sure, but it's about the same patch size and reduces churn - if we're gonna do it imminently anyway (which I thought was the point?) we should do it in one go because otherwise we're touching the same (sets of) line(s) of code in repeated PRs.
There was a problem hiding this comment.
If it's ok, I'd rather work a bit more on the Taproot refactors first because I'm starting to be a bit less sure if we wanna convert all the instances from using Signer to using KeysInterface. That's why I'd rather get this out of the way first, but not commit to the rest of the refactor yet.
There was a problem hiding this comment.
Ah, okay. This needs rebase, then.
arik-so
force-pushed
the
2022-10-background-processor-deparametrization
branch
from
29f4cab to
2298c99
Compare
|
Can you squash the second commit into the first. |
arik-so
force-pushed
the
2022-10-background-processor-deparametrization
branch
from
2298c99 to
1c8a06c
Compare
No description provided.