Identify trades based on their UUID #117
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| await swapEmitter.once('finished'); | ||
| stopNetworkListener(); | ||
| })(); | ||
| swapEmitter.once('completed').then(removeListener); |
There was a problem hiding this comment.
You are the one emitting this event, so couldn't you just call removeListener() on line 60?
There was a problem hiding this comment.
Currently yes, but this will be updated in the future to support other events such as 'failed' so I think it makes more sense to have it here.
|
I really like how much simpler the code is. 👌✨ |
|
I've done many successful trades with this branch, so seems to work well. |
|
Brilliant! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires a custom build of mm with UUID support. We shouldn't merge this until there's an official build with the UUID update.
This has simplified a lot of the swap tracking code and swapDB code. I stopped storing all the pseudo ids (
tradeid,aliceid,requestid,quoteid) in our DB as we didn't actually use them for anything apart from the crazy tracking logic. Now all we need is the UUID. All socket messages should have this and it should be unique.api.subscribeToSwap()now only requires a UUID as an argument as apposed to an object containing atradeid/aliceidorrequestid/quoteidcombo.It's important we always check if
uuidis set before we start tracking a swap, otherwise it'll check formessage.uuid === undefinedon every message, which will be true for every non swap message (there are a huge amount of those) and it'll jsut fill the database with random messages. Learned that the hard way.https://github.com/lukechilds/hyperdex/pull/117/files#diff-ec72782f2b90873687d6bfdd5f291908R45
We now have a much simpler index as we don't need to index all those different ids and create multiple indexes to make sorting work because of bugs in PouchDB.
We still don't properly detect unmatched trades, they will display as pending forever. UUIDs don't help with that, we need to implement our own logic to catch this that works the same was as marketmaker. More info here: jl777/SuperNET#745
There is also a tiny memory leak:
https://github.com/lukechilds/hyperdex/pull/117/files#diff-ec72782f2b90873687d6bfdd5f291908R64
We only remove the event listened on completed swaps, not on failed swaps. This will be very easy to add, we just need to check for both events. However I ran into a very strange situation in my tests where one swap was failed and the a few seconds later completed. Reported here: jl777/SuperNET#756. In that case if I removed the listener on the failed event we wouldn't of ever got the complete event. So I'm aware of this but leaving it as is for now until that issue is resolved. The memory implications of the memory leak should be miniscule.