router: add new interface MPPayment
#7391
Conversation
13251be to
1436be6
Compare
1436be6 to
7475af9
Compare
7475af9 to
c4f416c
Compare
d271cf5 to
fc51882
Compare
fc51882 to
8a55a1c
Compare
There was a problem hiding this comment.
Commits are really smooth to digest very clean work 👍 . Left some questions to deepen my understanding. Just reviewing this PR I had a bit of a hard time bringing it all into the greater picture.
Did a first pass-through for the review club today will come back and for a second review :)
There was a problem hiding this comment.
Nice simplifications 💪, first pass done. I have peeked inside the next PR as well, but need another round to see the whole picture more clearly (the end result of the payment lifecycle and its tests look awesome). Perhaps you could update the PR description as well to guide through the most important refactors.
|
Thank you for the thorough reviews @ziggie1984 @bitromortac 🙏 Think all comments are addressed, let me know if anything is missing! |
33d5863 to
0bc9fee
Compare
|
@bitromortac: review reminder |
|
|
||
| // Update the payment state and status. | ||
| m.State = &MPPaymentState{ | ||
| NumAttemptsInFlight: len(m.InFlightHTLCs()), |
There was a problem hiding this comment.
I just thought about removing the setState methods and inline them in fetchPayment, it seems they are not used for mocking, but not blocking
|
|
||
| // If the payment is already in a terminal state, no need to wait. | ||
| case StatusSucceeded: | ||
| case StatusFailed: |
There was a problem hiding this comment.
If the remaining amount is zero, it means that there are either HTLCs inflight or that all HTLCs are settled, so can the payment then be failed? It seems like we should error here, looks like we can handle that case together with StatusInitiated
There was a problem hiding this comment.
Nice catch! Yeah indeed this is an error state and we should return an error, updated!
This commit moves the creations of hop and htlcAdd message from `createNewPaymentAttempt` to `sendPaymentAttempt` to clean up the code and further pave the way to decomposite the lifecycle.
This commit renames the `handleSendError` to be `handleSwitchErr` to explicitly express that it's handling the error from htlcswitch.
This commit moves the struct `paymentState` used in `routing` into `channeldb` and replaces it with `MPPaymentState`. In the following commit we'd see the benefit, that we don't need to pass variables back and forth between the two packages. More importantly, this state is put closer to its origin, and is strictly updated whenever a payment is read from disk. This approach is less error-prone comparing to the previous one, which both the `payment` and `paymentState` need to be updated at the same time to make sure the data stay consistant in a parallel environment.
0bc9fee to
9531f6f
Compare
9531f6f to
47a1952
Compare
|
Linter complains about a newline. Can merge after that's fixed. |
This commit adds a new method, `NeedWaitAttempts`, to properly decide whether we need to wait for the outcome of htlc attempts based on the payment's current state.
This commit turns `MPPayment` into an interface inside `routing`. Having this interface gives us the benefit to write more granular unit tests inside payment lifecycle. As seen from the modified unit tests, several hacky ways of testing the `SendPayment` method is now replaced by a mock over `MPPayment`.
This commit adds a new method to properly init a payment lifecycle so we can easily see the default values used here.
This commit refactors the params used in lifecycle to prefer `HTLCAttempt` over `HTLCAttemptInfo`. This change is needed as `HTLCAttempt` also wraps settled and failure info, which is useful in the following commits.
47a1952 to
4a7f88e
Compare
Depends on,
And prepares for.
This PR continues refactoring the payment lifecycle to prepare the coming fixes in the above PR. It moves payment state management into
MPPayment, and further makesMPPaymentinto an interface so it's easier to write unit tests for the payment lifecycle.