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

dApp: blockchain progress for opening a channel #2136

Merged
merged 13 commits into from
Sep 22, 2020

Conversation

taleldayekh
Copy link
Contributor

@taleldayekh taleldayekh commented Sep 15, 2020

Fixes #1941

Short description
Introduces notification for opening a channel with a block counter that counts until channel gets opened.

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. Open a channel. Snackbar should appear.
  2. Open the notification panel, a notification for the open channel event should be shown with a counter.

This event will be needed for keeping track of when a channel gets opened.
@github-actions
Copy link

github-actions bot commented Sep 15, 2020

You modified raiden-ts/src,
Please remember to add a change log entry at raiden-ts/CHANGELOG.md if necessary.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #2136 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2136      +/-   ##
==========================================
- Coverage   95.72%   95.64%   -0.08%     
==========================================
  Files         160      159       -1     
  Lines        5967     5977      +10     
  Branches     1047     1052       +5     
==========================================
+ Hits         5712     5717       +5     
- Misses        194      197       +3     
- Partials       61       63       +2     
Flag Coverage Δ
#dapp 92.44% <ø> (-0.25%) ⬇️
#sdk 96.98% <ø> (ø)
#sdk_e2e 65.59% <ø> (ø)
#sdk_unit 85.93% <ø> (ø)

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

Impacted Files Coverage Δ
raiden-dapp/src/App.vue 100.00% <ø> (ø)
...components/notification-panel/NotificationCard.vue 77.77% <ø> (-22.23%) ⬇️
...onents/notification-panel/NotificationSnackbar.vue 87.50% <ø> (ø)
raiden-dapp/src/services/raiden-service.ts 87.50% <ø> (-0.38%) ⬇️
raiden-dapp/src/store/notifications/getters.ts 62.50% <ø> (+2.50%) ⬆️
raiden-dapp/src/store/notifications/index.ts 100.00% <ø> (ø)
raiden-dapp/src/store/notifications/mutations.ts 71.42% <ø> (-28.58%) ⬇️
raiden-dapp/src/store/notifications/state.ts 100.00% <ø> (ø)
raiden-dapp/src/views/NotificationPanel.vue 100.00% <ø> (ø)
raiden-ts/src/actions.ts 100.00% <ø> (ø)
... and 6 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 d1db7ae...f55439c. Read the comment docs.

… notification

Gets the values necessary for counting blocks from the channel open success event and passes them to a helper function for dispatching a notification whenever a channel open success event is registered.
One mutation for adding or updating a notification based on whether a notification is dispatched with an id that already exists in the notifications array.
Notification success now also takes in the values which were added when refactoring the store to enable updating of notifications.
Calling the getter for incrementing the id inside of the actions caused the id to be incremented even when new notifications were dispatched with the same id. To solve this we had to refactor and move everything to the mutations.
The NotificationCard component now displays the block count if the txConfirmationBlock is defined. Also, the NotificationArea component has been renamed to NotificationSnackbar since we are always refering to it as a snackbar.
Apart from fixing the broken tests I started with the tests for the newly added channel open notificaion, however it is currently not passing so I commented it out for now while working on somethin else.
@taleldayekh taleldayekh marked this pull request as ready for review September 21, 2020 15:18
Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

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

We have done a lot of this PR together. At least the basic "design" and a lot of debugging. So I have not reviewed the code manually again. We agreed that all code parts that are not too nice here, are based on the current code structure which will become target of upcoming issues that should clean this up.

Thanks for all your hard work. 💪
I think this feature showed perfectly, that sometimes a feature might look small, but the current code structure does not support it. So first the structure needs to be adopted, to then make such feature easy to integrate. Though all the trouble we had for days with this feature reminded us of some parts of the code that are hard to work with and will need some attention.

@weilbith
Copy link
Contributor

PS: We agreed that this PR won't care about the code coverage. There are so far no tests for the notifications module of the Vuex store. And since this PR includes already a lot of refactorings, we need to stop at some point. Therefore the coverage will be target of a new PR that @taleldayekh will start to work on. This PR will introduce these missing tests, testing the already existing features, as well as some new parts introduced here.

@taleldayekh
Copy link
Contributor Author

Great and thank you @weilbith for your assistance in debugging and code design.

@taleldayekh taleldayekh merged commit db74068 into master Sep 22, 2020
@taleldayekh taleldayekh deleted the channel-blockchain-progress branch September 22, 2020 08:00
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.

Show blockchain transaction progress for "Open Channel"
2 participants