-
Notifications
You must be signed in to change notification settings - Fork 3.4k
lnworker: split LNWallet and LNWorker: LNWallet "has an" LNWorker #10372
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
lnworker: split LNWallet and LNWorker: LNWallet "has an" LNWorker #10372
Conversation
98866b7 to
d13ac4e
Compare
|
This is ready for review. There is potential for follow-ups ofc, but I think it is in a good state we could merge for incremental progress. |
2ee0c55 to
e0284e8
Compare
|
I would prefer to merge this asap, because of possible conflicts |
f321x
left a comment
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.
Running this for ~1-2 weeks now on android, had no issues. looks good to me.
- LNWallet no longer "is-an" LNWorker, instead LNWallet "has-an" LNWorker - the motivation is to make the unit tests nicer, and allow writing unit tests for more things - I hope this makes it possible to e.g. test lnsweep in the unit tests - some stuff we would previously have to write a regtest for, maybe we can write a unit test for, now - in unit tests, MockLNWallet now - inherits LNWallet - the Wallet is no longer being mocked
no functional changes
These better hold, lol: wallet.lnworker.wallet == wallet lnworker.wallet.lnworker == lnworker
e0284e8 to
9277241
Compare
This is a major simplification re how we are mocking things in
test_lnpeer.pyand friends.not done:
{wallet,chan}.lnworkerto*.lnwalletpeer.lnworkeris polymorphic betweenLNWalletandLNGossipLNPeerManager._lnwallet_or_lngossiphas the same polymorphismlnworker.py(LNWallet is 3000 lines out of 4000 of that file) but that would be really annoying for some branches/PRs