offer: add a test with expiry info #518
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
While reviewing the code in [1], an interesting issue was discussed related to the expiry of the offer. The reported problem is as follows: > At least locally, if I set the expiry to 1 day (86400), it shows > the same expiry on decoding the offer as expected, but if I try to > pay it, it gives me an InvoiceRequestExpired error. However, if I > do the same with 1000 days, it works fine... This commit adds a test to ensure that we can pay an offer with an expiry of 1 day without encountering an InvoiceRequestExpired error. [1] getAlby/hub#1242 Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
vincenzopalazzo
force-pushed
the
macros/expiry-offer
branch
from
4aefb28 to
abe537d
Compare
|
I’ve reviewed the code, and it looks good to me. The test case is well-written and effectively addresses the issue with the 1-day expiry. I’ve also confirmed that the test passes both locally and in CI. LGTM! |
| } | ||
|
|
||
| // Sleep one more sec to make sure the node announcement propagates. | ||
| std::thread::sleep(std::time::Duration::from_secs(1)); |
There was a problem hiding this comment.
Maybe consider removing this line, the test ran successfully without it
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
There was a problem hiding this comment.
@vincenzopalazzo IIUC, this test can't reproduce the mentioned issue, and hence the bug is likely somewhere on the OP's end? If we don't think this is a regression on our side, this PR should likely be closed?
|
@tnull I want to try out to make a test with cln because probably the error is not happening between the same implementation, but if you do not think this is the right repo, feel free to close it |
Huh, but you didn't add the test to the CLN integration tests but rather copy/pasted the pre-existing Rust test and changed one parameter? Now, if we found this to indeed be a bug on our end, we should add a regression test for it, although then DRYing up the two test cases as they have substantial overlap. I'm going ahead and closing this for now, but feel free to reopen if you want to extend CLN integration testing or find that it's indeed a regression on our end. |
|
Make sense @tnull, I was just going to test before with two ldk-node and then move to cln! I duplicated the code to avoid the original test being too long to read, but I see your point! |
4 participants
While reviewing the code in [1], an interesting issue was discussed related to the expiry of the offer.
The reported problem is as follows:
This commit adds a test to ensure that we can pay an offer with an expiry of 1 day without encountering an InvoiceRequestExpired error.
[1] getAlby/hub#1242
Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com