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

Fix mismatch between UDC totalDeposit and effectiveBalance #2277

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

andrevmatos
Copy link
Contributor

Thank you for submitting this pull request :)

Fixes #2275

Short description
udcDeposit actions contained totalDeposit as sole meta property, but it actually was the effectiveBalance value, which raised issues when calculating the new totalDeposit when making a deposit after a withdraw was performed.
This PR fixes it by using actual totalDeposit as meta value (for linking udcDeposit requests & success/failure responses), and added balance to payload, to be used as UDC balance, instead of the deposit.
Integration tests were extended to assert behavior when depositing after withdraw, which works as regression test for this action.

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

Copy link
Contributor

@palango palango left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -459,13 +459,16 @@ export function monitorUdcBalanceEpic(
* etc), but merged on the top-level observable, therefore connectivity issues can cause
* exceptions which would shutdown the SDK. Let's swallow the error here, since this will be
* retried on next block, which should only be emitted after connectivity is reestablished */
defer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding line 455: We should open an issue for the contracts to add events.

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #2277 into master will increase coverage by 36.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2277       +/-   ##
===========================================
+ Coverage   58.12%   94.80%   +36.67%     
===========================================
  Files          61      156       +95     
  Lines        4461     6271     +1810     
  Branches      939     1163      +224     
===========================================
+ Hits         2593     5945     +3352     
+ Misses       1854      261     -1593     
- Partials       14       65       +51     
Flag Coverage Δ
#dapp 91.85% <100.00%> (?)
#sdk 95.99% <100.00%> (?)
#sdk_e2e ?
#sdk_integration 68.63% <100.00%> (?)
#sdk_unit 85.49% <61.11%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiden-ts/src/epics.ts 98.27% <ø> (+6.89%) ⬆️
raiden-ts/src/services/actions.ts 100.00% <ø> (ø)
raiden-dapp/src/components/account/Withdrawal.vue 92.85% <100.00%> (ø)
raiden-dapp/src/services/raiden-service.ts 87.01% <100.00%> (ø)
raiden-ts/src/raiden.ts 89.17% <100.00%> (+28.32%) ⬆️
raiden-ts/src/services/epics.ts 98.58% <100.00%> (+36.47%) ⬆️
raiden-ts/src/utils/actions.ts 96.47% <100.00%> (+1.34%) ⬆️
raiden-ts/src/polyfills.ts 68.42% <0.00%> (-31.58%) ⬇️
raiden-dapp/src/components/transfer/FindRoutes.vue 92.00% <0.00%> (ø)
...-dapp/src/components/dialogs/OpenChannelDialog.vue 100.00% <0.00%> (ø)
... and 134 more

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 b121047...e49c217. Read the comment docs.

@andrevmatos andrevmatos merged commit c6ae454 into master Oct 20, 2020
@andrevmatos andrevmatos deleted the fix/udc_deposit branch October 20, 2020 16:41
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.

MetaMask - RPC Error: execution reverted: deposit not increasing
2 participants