Skip to content

Retry withdrawal if it's still valid and possible - #4766

Merged
tomasmik merged 1 commit into
masterfrom
retry-withdrawal
Feb 11, 2022
Merged

Retry withdrawal if it's still valid and possible#4766
tomasmik merged 1 commit into
masterfrom
retry-withdrawal

Conversation

@tomasmik

Copy link
Copy Markdown
Contributor

Rely on transactor response codes being correct and retry
a withdrawal if it failed but the request itself was valid.

While retrying a few times for 5 minutes, doesn't completely solve our
issue, this and the previous withdrawal PR should remedy 90% or more of
the withdrawal problems being reported.

Solving this completely is rather difficult because money is actually
first taken from a provider and a payment promise is issued which has to be
settled with transactor we would have to somehow track this promise
and make sure we're resending it, but as it's mentioned in the comment
block in code, sending a settlement after 5 minutes will produce different
results then shown in the UI as fees will change and sometimes they will change dramatically.

Small issue which was not resolved yet: Every other withdrawal might
fail with no money being transferred to beneficiary or taken from provider. That is because we've stopped
lying to the UI and actually report failures now. This is not a huge deal as no real value transfer occurs.

Closes: #4631

Rely on transactor response codes being correct and retry
a withdrawal if it failed but the request itself was valid.

While retrying a few times for 5 minutes, doesn't completely solve our
issue, this and the previous withdrawal PR should remedy 90% or more of
the withdrawal problems being reported.

Solving this completely is rather difficult because we would have to
somehow track this promise and make sure we're resending it, but as it's
mentioned in the comment block in code, sending a settlement after 5
minutes will produce different results then shown in the UI as fees will
change and sometimes they will change dramatically.

Small issue which was not resolved yet: Every other withdrawal might
fail with no money being transferred. That is because we've stopped
lying to the UI and actually report failures now.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #4766 (f71822a) into master (093022c) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4766      +/-   ##
==========================================
- Coverage   39.33%   39.20%   -0.14%     
==========================================
  Files         356      356              
  Lines       19048    19084      +36     
==========================================
- Hits         7492     7481      -11     
- Misses      10837    10884      +47     
  Partials      719      719              
Impacted Files Coverage Δ
session/pingpong/hermes_promise_settler.go 33.55% <0.00%> (-2.15%) ⬇️
nat/traversal/pinger.go 75.19% <0.00%> (-4.66%) ⬇️
requests/dialer_swarm.go 72.18% <0.00%> (-3.76%) ⬇️
p2p/channel.go 68.38% <0.00%> (+2.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 093022c...f71822a. Read the comment docs.

@soffokl soffokl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏 👏 👏

@tomasmik
tomasmik merged commit 84c5af6 into master Feb 11, 2022
@tomasmik
tomasmik deleted the retry-withdrawal branch February 11, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Withdrawals still disappear sometimes

5 participants