lnd
lnd copied to clipboard
Add Payment Hash to a subset of htlc events
Change Description
This PR adds the payment hash to the htlc events Forward
, ForwardFail
, Settle
and Linkfail
and fixes #7165.
I intentionally decided to not include it in the FinalHtlcEvent
because it would have broken a lot of compatibility with existing code and from my perspective I see no benefit introducing them there.
Based on Jesse's Code who introduced the router-rpc tests in #6335, I created additional Testcases for the SubscribeHtlcEvents
server feature.
I did not find it reasonable to add itests for this functionality because there are already tests on the unit-test level in the htlcswitch
package, but open for other opinions.
Steps to Test
For the unit tests you can test it by doing:
make unit pkg=htlcswitch
For the router grpc-server you can do:
make unit pkg=lnrpc/routerrpc log="info" case=TestSubscribeHtlcEvents
Pull Request Checklist
Testing
- [ ] Your PR passes all CI checks.
- [x] Tests covering the positive and negative (error paths) are included.
- [x] Bug fixes contain tests triggering the bug to prevent regressions.
Code Style and Documentation
- [x] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
- [x] Commits follow the Ideal Git Commit Structure.
- [x] Any new logging statements use an appropriate subsystem and logging level.
- [x] There is a change description in the release notes, or
[skip ci]
in the commit message for small changes.
📝 Please see our Contribution Guidelines for further guidance.
I cannot really find a suitable unit/integration test where we test this htlc streaming method thats why for testing purposes I included an lncli command to test the new message on regtest.
Yeah the unit test for HtlcNotifier
is missing, something we def want to patch. For itest, if search SubscribeHtlcEvents
, there are multiple usages, for instance testHtlcErrorPropagation
.
I found it reasonable to only include the the payment_hash in events where it actually makes sense to use it. Events like SubscribedEvent and FinalHtlcEvent where intentionally not included.
Make sense.
Does it make sense to include the lncli subscribehtlcs for general usage, for now consider it experimental for testing purposes only
I'd imagine the use case for this endpoint would not be inside a terminal, so I guess not(not sure tho).
Thank you :) Now I can continue my work
I found the unit-tests, so I changed my initial commit.
Testing the gprc stream of htlcs I found an error when not populating the Linkerror in the LinkFail event, though I did not change the behaviour, so asking for your opinion how to handle this.
Also in the htlcswitch package, I had to add the function newPayHash, because the unit tests reveiled that sometimes the payment circuit during a failevent is nil, maybe just returning the payment hash is not the cleanest solution here. I marked the relavant code parts in further comments