Skip to content

Conversation

@SomberNight
Copy link
Member

@SomberNight SomberNight commented Dec 17, 2025

This is a major simplification re how we are mocking things in test_lnpeer.py and friends.

  • 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 is now almost a real LNWallet: it simply inherits LNWallet
    • MockWallet is now almost a real Abstract_Wallet: it simply inherits Abstract_Wallet

not done:

  • we could rename {wallet,chan}.lnworker to *.lnwallet
  • but e.g. peer.lnworker is polymorphic between LNWallet and LNGossip
    • LNPeerManager._lnwallet_or_lngossip has the same polymorphism
  • could split lnworker.py (LNWallet is 3000 lines out of 4000 of that file) but that would be really annoying for some branches/PRs

@SomberNight SomberNight force-pushed the 202512_lnworker_split branch 7 times, most recently from 98866b7 to d13ac4e Compare December 19, 2025 15:02
@SomberNight SomberNight marked this pull request as ready for review December 20, 2025 17:51
@SomberNight
Copy link
Member Author

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.

@SomberNight SomberNight force-pushed the 202512_lnworker_split branch from 2ee0c55 to e0284e8 Compare December 30, 2025 16:34
@SomberNight
Copy link
Member Author

rebased from 2ee0c55 to e0284e8

@ecdsa
Copy link
Member

ecdsa commented Jan 5, 2026

I would prefer to merge this asap, because of possible conflicts

Copy link
Member

@f321x f321x left a 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
These better hold, lol:
wallet.lnworker.wallet == wallet
lnworker.wallet.lnworker == lnworker
@SomberNight SomberNight force-pushed the 202512_lnworker_split branch from e0284e8 to 9277241 Compare January 5, 2026 15:56
@SomberNight SomberNight added this to the 4.7.0 milestone Jan 5, 2026
@SomberNight SomberNight merged commit 3bd5f1a into spesmilo:master Jan 5, 2026
3 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants