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

Support kickstarting swaps #163

Closed
lukechilds opened this issue May 3, 2018 · 3 comments · Fixed by #416
Closed

Support kickstarting swaps #163

lukechilds opened this issue May 3, 2018 · 3 comments · Fixed by #416
Assignees

Comments

@lukechilds
Copy link
Member

lukechilds commented May 3, 2018

https://modulelab.slack.com/archives/C8ADPSWTB/p1525328006000031

Sindre Sorhus [1:13 PM]
@Luke Childs https://github.com/jl777/SuperNET/commit/8725909b3c12f8df17de0b075422ad3676cc5195 

Have you thought about how the kickstart thing will work? I assume it's just a temporary workaround until mm does this automatically?

Luke Childs [1:14 PM]
no we’ll need kickstart long term
so if they quit the app with pending trades
or in process trades
then restart
we need to filter all the in progress trades and tell mm to resume them
I think it will be a fair bit of work to integrate
mm will most likely give us no helpful feedback in the event that:
- the trade doesn’t exist 
- exists but can’t be restarted
- was already automatically restarted
- is in the progress of being rolled back (edited)
we need to handle all those cases
and we probably won’t get enough information to
and we also need to re-attach event listeners so we get socket updates on restarted trades
if mm auto restarts them without telling us we’ll miss them
so many things that can go wrong
this is exactly why we shouldn’t have two separate swap DB instances…
😞
For v2 we need to push for a built in swap DB in mm that actually works and scales
so we have no internal copy
we just query mm as the single source of truth
They could use sqlite or LevelDB or something light weight
@lukechilds lukechilds added this to the 0.1.0 milestone May 7, 2018
@lukechilds lukechilds self-assigned this May 11, 2018
@lukechilds
Copy link
Member Author

lukechilds commented May 16, 2018

Just saving this snippet here for future reference:

_swapDB._getSwapData('f8b3d902d9c21ed0a2231ade8965f6f710d04953c27f00156e9a346550de1b20')
	.then(data => data.messages.reduce((ids, msg) => {
		ids.requestid = msg.requestid || ids.requestid;
		ids.quoteid = msg.quoteid || ids.quoteid;
		return ids;
	}, {}))
	.then(ids => {
		console.log(ids);
		_api.debug({method: 'kickstart', ...ids});
	});

@lukechilds
Copy link
Member Author

This behaviour is too unreliable right now and doesn't appear to be as important as we first thought:

kickstart can only make progress if progress is possible. likely you need to wait the 4hrs and unless the .finished file was created in error, kickstart wont be needed

lukechilds/hyperdex-bugtracker#1 (comment)

There's no way for us to reliably know if a kickstart is needed and or if it has been successful.

@lukechilds lukechilds removed this from the 0.1.0 milestone May 18, 2018
sindresorhus pushed a commit that referenced this issue Jul 13, 2018
Resolves #163 

This PR resolves the issue of users getting swaps that are stuck mid swap (e.g in `swap 3/5` state).

If a swap fails due to connectivity issues or closing the app the swap would become "stuck". Although the funds were cryptographically safe and reclaimable via the atomic swap protocol, `marketmaker` wouldn't actually reclaim the funds automatically without custom intervention via the console or CLI.

We are now manually looping over old swaps every 15 mins that are in a stuck state and "kickstarting" them to get `marketmaker` to recheck the status.

There are quite a lot of changes required to the current code base to get this working. I will try and break down the changes here and explain why they are necessary.

**Get updates on stuck swaps** 1e7ceee 5eaedd9

We were previously listening for updates over the socket for a swap by adding a new event listener for each swap that listens out for messages containing it's UUID. This was not a great approach and resulted in a few crazy hacks to avoid memory leaks whilst also correctly handling quirky `marketmaker` behaviour where a swap would be reported as failed and the event listener was removed, only to later send us a completion message.

Now we only attach a single event listener and check all messages against a list of all known swap UUIDs. This means we can guarantee we will always catch updates to swaps, if they were started in a previous session and even if they are received in a strange order, e.g failed then complete. It's also much simpler code and avoids risks of memory leaks having a large amount of event listeners.

**Force kickstart stuck swaps** 2e89499 6514951

This is supposed to be handled automatically by `marketmaker`, but I've never personally seen it actually work. What we now do is loops over all swaps that are currently swapping and were started over 4 hours ago, then call `marketmaker`'s `kickstart` method. This forces it to check the current state of the swap and if something has gone wrong or Bob hasn't responded it will attempt to reclaim the funds from the timelocked address. This is why we only check if the swap is over 4 hours old, because the timelocks last 4 hours. We do this instantly on startup and then check again every 15 minutes.

On the first startup check, we check all pending swaps, even if they weren't started over 4 hours ago, this is because there are some scenarios where a swap can be reverted sooner.

This means you don't need to keep HyperDEX (and `marketmaker` running). You can close the app, open it the next day, and it will instantly reclaim your stuck swap if possible.

**Recalculate the state of a completed swap** fc1caab 45b5b49

Due to the fact that HyperDEX (and `marketmaker`) might not be running for the entire swap, we can't rely on the WebSocket message history to provide an accurate overview of what happened during the swap. The completed message we get at the end of every normal swap and "kickstarted" stuck swaps contains all the information we need to rebuild the full history of what happened.

So we now have two ways of building a formatted swap object. The socket messages are used while a swap is in progress. Once a swap is completed, the final completed socket message is used to reconstruct the entire swap transaction history and state which we know contains all data. The resulting structure of the swap object follows the same pattern in both methods so all existing UI code works no matter how the swap data was calculated.

This means if a swap is completed it should never have missing tx data, it should always list the entire chain of TXs, even if it wasn't online the whole time. It also means we correctly handle weird states like when the swap doesn't go to plan and we reclaim our original payment or claim `bobdeposit` instead of `bobpayment`. It also abstracts out quite a few quirks in `marketmaker` responses.

**Showing kickstarted swaps TXs in the swap modal** 1a95811 0e55952

Kickstarted swaps don't always have the same transactions exposed as our current code expects which breaks the modal layout. I've modified the transactions array to not allow duplicate transactions (sometimes `marketmaker` sends multiple messages for the same TX which we were pushing into the transactions array). This means while the swap is in progress we can rely on just looping over the transaction array for the current data are less the 5 items fill them in with the placeholders.

Once a swap is completed however, there may not be 5 transactions due to the reversion and we don't want to show the placeholder boxes as those transactions are no longer expected to occur. However the reversion transactions are handled by the completed message and added to the transactions array, so just looping over that means they will show up, we just skip showing the placeholder boxes.

---

There's also a few other bits handling weird mm responses and adding extra data for stuck swaps, but they're fairly self explanatory.

To show how successfully resumed swaps look in the UI:

**A swap that was stuck, but then resumed in time to proceed as normal**

By either a reboot, losing network connectivity, accidentally closing the app etc.

This swap appears just like a normal swap that completed without issue:

<img width="608" alt="screen shot 2018-07-12 at 3 21 59 pm" src="https://user-images.githubusercontent.com/2123375/42621378-d46a7a94-85e7-11e8-8f30-b977b55e91fc.png">

<img width="697" alt="screen shot 2018-07-12 at 3 22 11 pm" src="https://user-images.githubusercontent.com/2123375/42621389-da408fb2-85e7-11e8-9628-09909c73b438.png">

*Notice the final TX is `alicespend`, the expected TX for a successful swap*

**A swap that was stuck due to Bob not sending `bobpayment`**

If Bob doesn't send `bobpayment` you need to wait four hours for the timelock on `bobdeposit` to expire. The we can claim `bobdeposit` with an `aliceclaim` TX. `bobdepost` is 12.5% more than the amount we were supposed to get. We get to keep this to punish Bob for being a dick. (In a normal trade Bob would reclaim his own `bobdeposit` after we spend `bobpayment` with `alicespend`).

You don't actually need to keep the app running for four hours though, you could go to bed and run the app in the morning and it will reclaim everything.

This swap appears just like a normal swap in the swap list. However in the modal it will show only four TXs in the TX chain and you'll notice that it shows a much higher percentage message than normal due to our 12.5% bonus:

<img width="610" alt="screen shot 2018-07-12 at 3 24 10 pm" src="https://user-images.githubusercontent.com/2123375/42621736-c1c5dc2a-85e8-11e8-8d32-88aca1f2c333.png">

<img width="691" alt="screen shot 2018-07-12 at 3 23 52 pm" src="https://user-images.githubusercontent.com/2123375/42621744-c52de4e8-85e8-11e8-8895-3a52a8a179f2.png">

*Notice the final TX is `aliceclaim` and is higher than what we asked for*

**A swap that was stuck due to Bob not sending any payments**

If Bob doesn't send any payments (`bobdeposit` or `bobpayment`) we can't really do much about it. We can reclaim our `alicepayment` funds but we lose our `dexfee` payment.

Although `marketmaker` reports the status as successfully completed (which kind of makes sense in terms of the atomic swap protocol), I'm mapping this to a failed swap in our logic because if the user doesn't get their desired funds, it's failed from their perspective.

So it will show in the swap list as reverted with the same styles as a failed/unmatched swap. The received value will be set as 0 and you will see the reclaim TX as the final TX in the chain. There is also a relatively vague message explaining what went wrong. (We really don't know what went wrong, it could have been many things):

<img width="606" alt="screen shot 2018-07-12 at 3 24 21 pm" src="https://user-images.githubusercontent.com/2123375/42622055-a00e2a5a-85e9-11e8-91bb-e27e9b2866c9.png">

<img width="691" alt="screen shot 2018-07-12 at 3 24 29 pm" src="https://user-images.githubusercontent.com/2123375/42622059-a2278ba6-85e9-11e8-9dca-955a55c58672.png">

*Notice the final TX is `alicereclaim` which just claims back our original funds*

---

As far as I'm aware, this correctly handles all stuck states and displays them correctly in the UI. You are guaranteed to either:

- Get the amount you were supposed to
- Get the amount you were supposed to + 12.5%
- Reclaim your original payment (but lose your `dexfee`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant