Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(connext): unlock expired transfer apps #1857

Merged
merged 2 commits into from
Sep 7, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2020

This PR adds logic to attempt to unlock expired connext transfer apps when receiving EXPIRED response from /hashlock-status.

Closes #1849

@ghost ghost requested review from sangaman and raladev September 1, 2020 10:27
@raladev
Copy link
Contributor

raladev commented Sep 2, 2020

  1. it would be good to have simtest for this case (if we can control expiry blocks number on the connext side)
  2. or at least integration test (mocking hashlock response with/without EXPIRED field)
  3. Also, @erkarl may u check tests status? it seems they are in infinite loop. I restarted them once and got same result.

@raladev raladev removed their request for review September 3, 2020 13:33
@kilrau kilrau assigned ghost Sep 3, 2020
@@ -455,6 +457,20 @@ class ConnextClient extends SwapClient {
preimage: transferStatusResponse.preImage?.slice(2),
};
case 'EXPIRED':
const expiredTransferUnlocked$ = defer(() => from(this.settleInvoice(rHash, '', currency))).pipe(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to what this is doing (some comments might be good). Why are we "settling" a payment where we are the sender? I know that "invoice" is not exactly a connext term/concept, maybe we should just use the connext call/terminology directly here rather than settleInvoice which is a generic SwapClient method.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the connext transfer (HTLC) expires the funds are not automatically returned to the channel balance. Expired transfer apps can be unlocked by both the receiver and sender by calling /hashlock-resolve with the paymentId.

In this PR the settleInvoice function has been modified to always include the paymentId in the body of the request in case the transfer has been expired.

It's worth noting that the node also periodically cleans up the expired transfer apps, but from UX perspective it's better if this happens from xud side. This ensures that the funds locked in the transfer app are available to the user sooner.

I've added a short comment above the settleInvoice call.

maybe we should just use the connext call/terminology directly here rather than settleInvoice which is a generic SwapClient method

Are you suggesting to create a wrapper function here that internally calls settleInvoice?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for the explanation this makes sense. I think it would be great to put a brief summary of this in a line or two of comments right above this call.

I think calling settleInvoice here with a blank preimage is still a bit odd, I would just go ahead and call /hashlock-resolve directly like so:

await this.sendRequest('/hashlock-resolve', 'POST', {
  assetId,
  paymentId: sha256(['address', 'bytes32'], [assetId, `0x${rHash}`]),
});

And the settleInvoice call can be left as it currently is unless you think the payment id is useful to include even when we have the preimage?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

And the settleInvoice call can be left as it currently is unless you think the payment id is useful to include even when we have the preimage?

Yep, removed payment id from settleInvoice.

@ghost ghost force-pushed the feat/unlock-expired-connext-transfers branch from 36b67cc to be3a940 Compare September 4, 2020 10:59
@ghost ghost force-pushed the feat/unlock-expired-connext-transfers branch from be3a940 to ae09eb7 Compare September 4, 2020 11:07
@ghost
Copy link
Author

ghost commented Sep 4, 2020

2. or at least integration test (mocking hashlock response with/without EXPIRED field)

Integration test already exists in this PR.

3. Also, @erkarl  may u check tests status? it seems they are in infinite loop. I  restarted them once and got same result.

Tests should now pass.

@ghost ghost requested review from sangaman and raladev September 4, 2020 11:23
@ghost
Copy link
Author

ghost commented Sep 4, 2020

EDIT: removed. Wrong issue.

@raladev raladev removed their request for review September 4, 2020 13:22
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you.

@sangaman sangaman merged commit 023434d into master Sep 7, 2020
@sangaman sangaman deleted the feat/unlock-expired-connext-transfers branch September 7, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xud should unlock expired connext transfer apps
2 participants