Feature/dapp quick pay show additional costs #2955
Merged
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
Due to recent decisions, the user must always be able to see the full costs of his transactions before confirming them. This means that the fully automated transfer by the SDK can not be used anymore as it might end up in a mediated transfer with additional hidden costs. To solve this issue the dApp must always decide to do a direct or mediated transfer. Therefore the former transfer action got replaced by two new actions: direct transfer and mediated transfer. The direct transfer will also construct the to use direct route itself. This is necessary to ensure it will be a direct transfer. Furthermore it is fine to put this into the action itself, as it will not cause any hidden costs. This is different for the mediated transfer. Here the user must decide himself to first query a route (paying the pathfinding service) and then use this route with the additional fees. Therefore the mediated transfer action is expecting the route as parameter.
Codecov Report
@@ Coverage Diff @@
## master #2955 +/- ##
==========================================
+ Coverage 93.32% 93.45% +0.12%
==========================================
Files 207 208 +1
Lines 8830 8917 +87
Branches 1382 1394 +12
==========================================
+ Hits 8241 8333 +92
+ Misses 488 480 -8
- Partials 101 104 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Btw: I'm so unhappy with how cluttered this code is and also the recent changes to the action mixin etc, that I was working on an alternative approach in my private time. I think I have a promising approach and it works well so far. Though I just recently started to refactor this whole new approach to make the code more nice and easy for the future (same solution, different code/technique). Unfortunately I have/get no time to work on this at work atm. And I'm not always motivated to work on it in my free time. So this might take a little longer. I apologize for the code style here. This is actually not the standard I try to produce. |
|
Proposal:
|
The action components, based on the action mixin are quiet some special components. But some of them are slightly different than the others. These are those which include (also) a transfer. So far action are quite tightly coupled to the usage with action forms in may cases. Though this might become improved in future, at the moment there is the need for some actions to take more options to perform their action then the action form provides (e.g. payment identifier). These have been provided as special properties which are only defined on a couple of those actions. This lead to different interfaces of the actions, which is not nice. Especially where actions are used interchangeable like for the quick pay feature it is necessary to have a unified interface. So far this situation was accepted, because all actions that were different where different the same. And on the quick pay route only those special actions where used. But with the recent addition of direct and mediated transfer actions, this has changed. The mediated transfer now also needs to be provided with a route. This also causes issues in the quick pay route. To solve it, a new property has been added to the base action mixin. This piece of data works the same as the generic `options` argument to the `handleAction` function of the actions. It is an object that can be specified individually by each implementing action. The same goes now for the `fixedRunOptions`. They are supposed to be provided as an optional component property and can be used as any other data in the respective actions implementation. Those actions who don't need any additional options just ignore them. After all there are some downsides to this approach: type safety. Having this generic new properties does not allow for any static type checks. The action will basically fail (or not) when the respective action logic can not handle the undefined parameters which are expected. But it could also lead to unintended behavior if they can but work differently then. After all typical JavaScript issues. We accept these issues for the moment. The actions were not safe beforehand as well as this is just the same approach as we took for the direct/"flexible" action options. There are different approaches to tackle this issue to detect miss-usage early. Though probably not on a static code analysis base. Unfortunately none of these approaches is really the best and it requires more thoughts how to solve it completely with something that works for both option datasets.
This new features allows to provide a custom slot element to the display amount. The optional slot will be displayed instead of the actual amount. The intention behind this feature is to provide a very generic approach to make the component more adaptive for special cases. An example would be the case where the amount is not yet known and needs to be fetched asynchronously. Then the slot can be filled with a spinner component. If the fetching has been completed, the spinner gets removed (conditionally rendering) and the amount gets displayed as usual. This also allows to use the amount display component is cases where the amount might be undefined in the beginning, but still having this component contributing to the layout and maybe have a label to it. The slot approach keeps the component very simple without much impact and much customization for special scenarios only. So it remains a very basic component that can be used everywhere. Furthermore there were small improvements to the code itself. In the components implementation/template as well as the tests (make them more explicit). Finally it is now possible to define an empty label on purpose. Doing so allows to maintain the layout. This makes for example sense when there are multiple display components together or the label might change other time. If the label is completely missing (undefined) the pure amount will be displayed as always.
The action form component automatically focused the first input field that is editable. But this might not be always the desired behavior. Therefore a new property has been added to disable autofocus completely.
Sometimes an amount is associated with a warning. Prominent examples are when a balance is too low or a price too high. To simply visualize this, the amount display component has a new flag property to toggle the warning. This will apply special coloring to the formatted amount then.
Due to recent decisions, the user must always be able to see the full costs of his transactions before confirming them. This means that the fully automated transfer by the SDK can not be used anymore as it might end up in a mediated transfer with additional hidden costs. To solve this issue the dApp must always decide to do a direct or mediated transfer. As a result of this, the quick pay route must manually fetch the available pathfinding services and select the cheapest one. In case a direct transfer is not possible but a mediated one, the user will get displayed more payment information. This includes a new section for the mediation. Here the user sees the price of the cheapest service once it is known. Then the user can decide himself to spend the money to fetch a route. Once the possible routes are fetched, the cheapest one gets selected and the user can see the mediation fees for this route. The user can now see the complete of costs the payment will charge him. While the user has not yet asked to query the pathfinding service for a route, the quick pay view will show him the alternative payment solution. This will either be a channel deposit and direct transfer or a channel open and direct transfer. Once he has received a valid route, this will switch to a mediated transfer. So far the user can not decide against the fetched route and go back to the alternative action. As workaround he would need to reload the page and go straight for it. The whole section of mediation stuff includes some nice features like visual progress states, and the detecting if a pathfinding service is too expensive with visual effects and disabling. The logic to handle failed transfer got removed as it is not necessary anymore. It was only used to check that a mediated transfer was not possible and then decide on an alternative action. This is now build in from the very beginning and the user has the choice between both. I'm happy about the user flow we have designed here. But I'm not too happy with the code. The quick pay route has become way too big and complex. Though it is quite hard to split it in a helpful way. Somehow it would be nice to outsource the whole payment information stuff with the new mediation interactive logic. But it is to much tied with all the other stuff that it produces more overhead than actual gain. Furthermore have the latest updates shown that the combination of actions with the channel action form is not flexible enough. While a nice feature, it must become improved with a more generic form approach.
There is a special routing guard that checks if the user has any connected token. If not, it will automatically forces him to route to the screen where he can connect. There are a few exceptions to this like the account settings. This does also prevent a new user to use the quick pay feature right away. But as the quick pay works also for new users, this must be a new exception to the guard.
The autofocus features of the channel action form component were not tested yet. This has become especially important since it is possible to disable the automatic focus.
When the pathfinding servie is too expensive, the price was shown in red and the button to fetch a route was disabled. This leaves the user with no information why. To improve this, instead of the button a warning message gets displayed. This bascially makes the former 'no routes' approach more generic an allows to use it for other messages as well. The price will not be shown in red color anymore as this would be too much red then.
|
I'm optimistic and resolve the fixup commits. So we can get this PR merged soon. |
weilbith
force-pushed
the
feature/dapp-quick-pay-show-additional-costs
branch
from
b2c0d51 to
cb46032
Compare
|
Btw, as the CHANGELOG hooks is failing due to the PRs branch comes from a fork: as the quickpay feature was not yet part of a release, I think it doesn't make sense to have another entry for it in this log as it is just an improvement of this feature within the same release. |
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.
Thank you for submitting this pull request :)
Fixes #2954
Short description
I'm happy about the user flow we have designed here. But I'm not too happy with the code. The quick pay route has become way too big and complex. Though it is quite hard to split it in a helpful way. Somehow it would be nice to outsource the whole payment information stuff with the new mediation interactive logic. But it is to much tied with all the other stuff that it produces more overhead than actual gain.
Furthermore have the latest updates shown that the combination of actions with the channel action form is not flexible enough. While a nice feature, it must become improved with a more generic form approach.
Finally this issue has shown us how much important it is to have #2929 implemented.
Definition of Done
Steps to manually test the change (dApp)