Add Payment Hash to a subset of htlc events #7309
Conversation
Yeah the unit test for
Make sense.
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 |
ziggie1984
force-pushed
the
payment-hash-subscribehtlc-events
branch
4 times, most recently
from
7580729 to
3156254
Compare
|
I found the unit-tests, so I changed my initial commit. |
ziggie1984
force-pushed
the
payment-hash-subscribehtlc-events
branch
3 times, most recently
from
a544fac to
24430e6
Compare
Adds the payment hash to the particular set of htlc notifiers (Forward, ForwardFail, Settle, LinkFail). Also unit tests are adapted according to the introduced changes.
ziggie1984
force-pushed
the
payment-hash-subscribehtlc-events
branch
2 times, most recently
from
5b19198 to
ce3a579
Compare
|
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 |
|
|
||
| // newPayHash returns the payment hash for the package provided. | ||
| func newPayHash(pkt *htlcPacket) lntypes.Hash { | ||
| // In case the circuit is nil a zero payment hash is returned to satisfy |
There was a problem hiding this comment.
Had to add the zero payment hash here, because some unit tests failed because the circuit was sometimes nil.
| if err != nil { | ||
| t.Fatalf("failed sending htlc event to server: %s", err) | ||
| } | ||
| // Make sure the LinkError is populated otherwise the test fails. |
There was a problem hiding this comment.
The comment speaks for itself, though this is the part where the test will fail if we just pass the empty LinkFailEvent
ziggie1984
force-pushed
the
payment-hash-subscribehtlc-events
branch
from
ce3a579 to
f433790
Compare
Adds the payment hash to a particular set of htlc events (Forwards, ForwardFails, Settle, LinkFail) of the grpc stream. In addition the grpc tests for the routerrpc package are adapted for the subscribehtlcevents.
ziggie1984
force-pushed
the
payment-hash-subscribehtlc-events
branch
from
f433790 to
8de0f51
Compare
Change Description
This PR adds the payment hash to the htlc events
Forward,ForwardFail,SettleandLinkfailand fixes #7165.I intentionally decided to not include it in the
FinalHtlcEventbecause 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
SubscribeHtlcEventsserver 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
htlcswitchpackage, but open for other opinions.Steps to Test
For the unit tests you can test it by doing:
make unit pkg=htlcswitchFor the router grpc-server you can do:
make unit pkg=lnrpc/routerrpc log="info" case=TestSubscribeHtlcEventsPull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.