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

Notify when receiving transfers gets disabled #1548

Merged
merged 3 commits into from
May 22, 2020

Conversation

andrevmatos
Copy link
Contributor

@andrevmatos andrevmatos commented May 19, 2020

Fixes #1473

TODO: tests, changelog

Short description
A dialog is shown on any screen on dApp when/if SDK has Receiving transfers disabled. This can happen on startup or during runtime, for mainly a couple of reasons:

  • rateToSvt config mapping is empty (it is by default, so receiving is disabled until it gets populated)
  • monitoringReward config isn't set (it is by default, as 5SVT)
  • udcBalance is not enough to pay monitoring reward (which can happen at startup or at runtime by a withdraw from service or user I want to withdraw tokens from the UDC contract #1421).
  • explictly disabled in user config with Raiden.updateConfig({ caps: { noReceive: true } }), which has priority

image

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)

  1. On dApp, acquire raiden SDK instance and enable receiving with:
raiden.updateConfig({
  rateToSvt: { '<token_addr>': '100000000000000000000' },
  caps: { noReceive: false },
 })
  1. Reload, check no dialog is shown
  2. Disable receiving explicity while connected:
raiden.updateConfig({ caps: { noReceive: true } })
  1. See dialog shown which says receiving is disabled
  2. One can also trigger dialog shown, even if caps['noReceive'] is unset (auto), if UDC balance is emptied, if one sets monitoringReward config to something higher than UDC desposit or rateToSvt is emptied

@@ -111,6 +110,11 @@ export class Raiden {
*/
public readonly transfers$: Observable<RaidenTransfer>;

/** RaidenConfig object */
public config!: RaidenConfig;
Copy link
Contributor

@nephix nephix May 20, 2020

Choose a reason for hiding this comment

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

Should we provide a callback'ish way as well? I guess we haven't decided yet in #357

It's not multi step but same thing IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking about it, I really want to standardize these interfaces, but didn't want to make this PR more complex, so just used the same pattern we already have for state$, channels$, transfers$ and so.. I'd say we should keep it like that for now, but I'm thinking on something like making Raiden an EventEmitter derived class and emitting these as events in a more interoperable way.

Copy link
Contributor

@nephix nephix left a comment

Choose a reason for hiding this comment

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

Looks plausible to me overall

@andrevmatos andrevmatos force-pushed the feature/notify_receiving_disabled branch from 5814141 to 39f7eec Compare May 20, 2020 18:03
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #1548 into master will decrease coverage by 0.00%.
The diff coverage is 98.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
- Coverage   96.11%   96.10%   -0.01%     
==========================================
  Files         141      142       +1     
  Lines        5117     5138      +21     
  Branches     1004      954      -50     
==========================================
+ Hits         4918     4938      +20     
  Misses        155      155              
- Partials       44       45       +1     
Flag Coverage Δ
#dapp 92.37% <100.00%> (+0.07%) ⬆️
#sdk 97.63% <98.03%> (-0.03%) ⬇️
Impacted Files Coverage Δ
raiden-ts/src/types.ts 100.00% <ø> (ø)
raiden-ts/src/epics.ts 96.15% <95.00%> (-1.29%) ⬇️
raiden-dapp/src/App.vue 100.00% <100.00%> (ø)
...src/components/dialogs/ReceivingDisabledDialog.vue 100.00% <100.00%> (ø)
raiden-dapp/src/services/raiden-service.ts 88.96% <100.00%> (+0.14%) ⬆️
raiden-dapp/src/store/index.ts 100.00% <100.00%> (ø)
raiden-ts/src/raiden.ts 94.61% <100.00%> (+0.01%) ⬆️
raiden-ts/src/services/epics.ts 99.08% <100.00%> (ø)
raiden-ts/src/transfers/epics/init.ts 100.00% <100.00%> (ø)
raiden-ts/src/transfers/epics/locked.ts 98.20% <100.00%> (ø)
... and 7 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 4058afe...56dff7d. Read the comment docs.

@andrevmatos andrevmatos marked this pull request as ready for review May 20, 2020 18:16
@andrevmatos andrevmatos force-pushed the feature/notify_receiving_disabled branch from 39f7eec to 56dff7d Compare May 21, 2020 12:34
Copy link
Contributor

@taleldayekh taleldayekh left a comment

Choose a reason for hiding this comment

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

Works like a charm, good job and thank you!

@andrevmatos andrevmatos merged commit d5758d9 into master May 22, 2020
@andrevmatos andrevmatos deleted the feature/notify_receiving_disabled branch May 22, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I want to get receiving deactivated if the SVT in the UDC is not high enough to monitor my transfers
3 participants