Fix parallel open + deposit and disable channel closing if Metamask prompt is cancelled
#2967
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
rxjs@7 renamed `merge` operator to `mergeWith`, which merges the content of observable arguments with input (like `merge` function, but as inline operator), and this conflicts with the name we've choosen for our custom operator (no functionality yet provided ootb). The new name `withMergeFrom`, is also more descriptive, as it acts like `withLatestFrom` operator, but without losing values from input before arguments emitted (which the later does) and also emitting for each inner emition as well.
andrevmatos
force-pushed
the
fix/parallel_deposit
branch
from
77f3f08 to
d14cbe7
Compare
Codecov Report
@@ Coverage Diff @@
## master #2967 +/- ##
==========================================
- Coverage 93.30% 93.26% -0.04%
==========================================
Files 207 207
Lines 8813 8761 -52
Branches 1372 1353 -19
==========================================
- Hits 8223 8171 -52
Misses 489 489
Partials 101 101
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
andrevmatos
force-pushed
the
fix/parallel_deposit
branch
from
27763e5 to
284d421
Compare
3 tasks
palango
reviewed
Oct 11, 2021
Since we are waiting 2x confirmations after deposit/open, this is more than enough time for the PFSCapacityUpdates to be sent, and we can remove this quite complex and bug-prone helper.
This ensures `BigNumber` constructor instance is the same across CLI and SDK, and CLI's logging `util.inspect.custom` method properly pretty print BigNumber instances in logs
if a first unconfirmed openChannel from partner came through while we were openChannelWithDeposit'ing, a channelDeposit is requested, which will fail if the former openChannel is replaced by our openChannelWithDeposit on a reorg. In this case, we must give up on the deposit request. This happens on BF7/high-concurrency scenarios.
A rare race condition (visible only on high-concurrency scenarios like BF7) could cause an openChannelWithDeposit which first succeeded (and therefore cancelled parallel deposit) to be reorged and replaced by partner's concurrent openChannel, causing deposit to never go through (and scenario's timeout). By dispatching `channelDeposit.request` in parallel unconditionally with method used (and actually using it for ensureApprovedBalance$) guarantees deposit will be performed on either condition, and is cancelled only when/if/after totalDeposit gets confirmed.
Allows users to change their minds and reject Metamask's tx prompt and still get an usable channel after that. Channel goes to closing/settling state upon respective unconfirmed success now.
andrevmatos
force-pushed
the
fix/parallel_deposit
branch
from
284d421 to
ab65dc8
Compare
palango
approved these changes
Oct 11, 2021
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.
Fixes #2963
Also fixes an issue seen in highly-concurrent scenarios like BF7
Short description
See commits messages for descriptions of what got fixed.
Definition of Done
Steps to manually test the change (dApp)