-
Notifications
You must be signed in to change notification settings - Fork 345
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
Remove KeysInterface #1930
Remove KeysInterface #1930
Conversation
c8fd161
to
74a5712
Compare
Codecov ReportBase: 90.73% // Head: 90.76% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1930 +/- ##
==========================================
+ Coverage 90.73% 90.76% +0.02%
==========================================
Files 97 96 -1
Lines 50539 50088 -451
Branches 50539 50088 -451
==========================================
- Hits 45859 45461 -398
+ Misses 4680 4627 -53
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. |
@@ -372,7 +372,7 @@ where | |||
CF::Target: 'static + chain::Filter, | |||
CW::Target: 'static + chain::Watch<<K::Target as SignerProvider>::Signer>, | |||
T::Target: 'static + BroadcasterInterface, | |||
K::Target: 'static + KeysInterface, | |||
K::Target: 'static + EntropySource + NodeSigner + SignerProvider, |
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.
I imagine we can probably start to split each of these out into their own generic constraint (yay more generics). EntropySource
seems like a good candidate since it's somewhat standalone. NodeSigner
and SignerProvider
really depends on the underlying deployment, since they will likely be backed by the same HSM/secure element.
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.
Yea, if we're splitting the traits I have to assume we expect people to have different structs that implement different parts of the traits (otherwise why are we doing this to begin with :) ), in which case we need to break up the generics as well.
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.
I agree we should split things up, but I would lean against doing all the splitting in this PR, maybe with the exception of some very shallow-level API examples, like BackgroundProcessor. BackgroundProcessor definitely lacks generics.
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.
I don't think it will be less effort to do it in one PR? As-is is just more diff and intermediate state, it would be less total diff to review doing it all at once.
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.
Well, one benefit of splitting it up into two is that it would allow @wpaulino to open his node_secret removal PR against the commit that's currently in this one, while I work on the follow-up in parallel. But I'll work on the parameter split tonight; maybe I can get it in fast enough and it's less trouble than I'm anticipating.
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.
I don't think that would conflict, now that we split the traits and moved those functions around. This PR doesn't really heavily touch the keysinterface.rs
file.
Wilmer just made the point that instead of removing KeysInterface entirely, we could simply make it be comprised of |
I'm still not sure why its worth doing that as a separate PR, but, sure, if you want to do that to make forward progress, sounds fine. We'll ultimately need to do it for all of them anyway. |
eafdaa2
to
22d7a0b
Compare
This commit separation makes the PR really hard to review (ie generates more total lines of diff to look at) - can you instead break it up as individual commits which change different structs to use individual traits (including the lifetime changes) and then only as the very last commit remove the (at that point unused) |
I'll try |
Needs rebase after #1812, sorry. Should be mechanical but may be a bit annoying. |
There are only like two files that are letting their KeysInterface arguments be split up atomically, inbound_payment.rs and blinded_path.rs. Everything else percolates down to the Channel and ChannelManager struct, resulting in an all-or-nothing scenario. |
Can't |
Funny you say that, because those two are exactly the types I've been trying to make separate. But no, they both have dependency chains that suck in all of Channel and ChannelManager. |
22d7a0b
to
a65fa82
Compare
a65fa82
to
49c1f30
Compare
If you remove the |
I'm talking about splitting up the keys_interface argument into implementations of its comprising traits. Once you have two or three arguments of different traits, that dependency chain sneaks through pretty much everything. That is unrelated to removing KeysInterface. |
@@ -317,7 +317,7 @@ pub struct Node<'a, 'b: 'a, 'c: 'b> { | |||
pub router: &'b test_utils::TestRouter<'c>, | |||
pub chain_monitor: &'b test_utils::TestChainMonitor<'c>, | |||
pub keys_manager: &'b test_utils::TestKeysInterface, | |||
pub node: &'a ChannelManager<&'b TestChainMonitor<'c>, &'c test_utils::TestBroadcaster, &'b test_utils::TestKeysInterface, &'c test_utils::TestFeeEstimator, &'b test_utils::TestRouter<'c>, &'c test_utils::TestLogger>, | |||
pub node: &'a ChannelManager<&'b TestChainMonitor<'c>, &'c test_utils::TestBroadcaster, &'b test_utils::TestKeysInterface, &'b test_utils::TestKeysInterface, &'b test_utils::TestKeysInterface, &'c test_utils::TestFeeEstimator, &'b test_utils::TestRouter<'c>, &'c test_utils::TestLogger>, |
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.
Pre-existing, but we really need a type alias for this test ChannelManager
...
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.
true that
No description provided.