Feature/dapp refactor ethereum provider #2702
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
The connect dialog does nothing anymore. It shows the first time the user connects and has the configuration option to use a subkey (which defaults to true). Besides that is just delays the connection. Also it does not add any valid information in terms of that the subkey gets regenerated each time the user connects anyways. But after all the major reason to get rid of this stuff is because of the upcoming connection manager. Thereby it will become obsolete anyway. With this step to remove it, it helps to clean the code a little to provide a more flat basement to draft on.
Due to multiple historic changes, the connection status got kinda complicated. This could be simplified by getting rid of the global loading state. Also is the status now directly part of the state and is no more a derived property. This simplified a few old test which haven't been refactored yet. In general it simplifies the usage/management and readability.
This must have been forgotten at some point in time.
The RaidenService class is closely tight to the Web3Provider and
ConfigProvider classes. At best it should not even know those exist.
Rather a higher level ("dirty") module should orchestrate the usage
between them. This is important as in future there will be different
provider options with according configurations.
To loose the components, the RaidenService now takes all necessary
values - including the ethereum provider instance - as parameters. It is
now the Home view components job to get the provider and configuration
to connect the SDK. Later it is possible to opt in as user to specify
different providers and configurations with visual components.
The whole usage of account, address, main, raiden-*, ... is quite confusing/inconsistent in the dApp. To keep the changes minimal for the moment, this just makes clear for this specific part what the parameter is actually about.
The too complex RaidenService connect function gets more and more simplified and less tight to its environment. One part it still did was to evaluate different errors of the SDK and push error states to the Vuex store. There they got picked up by the Home view component which uses another component to translate and display them to the user. Futhermore the stores state got used for the condiational flow. This is completely over the top. The Home view component calls the RaidenService plugin directly. Thereby it can get errors directly and handle them directly. The whole detour is just not necessary. Moving the error handling directly into the caller is more naturual and heavily reduces the complexity of the whole information flow and the RaidenService class. Furthermore the translation of errors was kinda cumbersome as well. Plus is was hard to extend, especially for a more generic scenario not only for connecting. Therefore this got simplified as well, allwing to strip the whole NoAccessMessage component. In future extension it probably makes sense to have a small Vue plugin or a Mixin to map the error codes to translation keys at a single place.
The dialog which handles the upload of a backup state got broken. This was causes by the recent changes of the RaidenService connect function. But actually the dialog must not call the connect function anyway. Rather the backup is stored into the Vuex store and there gets picked up by the next connection procedure. This was rather an uncontrolled behavior which eventually could cause bugs. Cleaning this up also removes the origin issue. Furthermore while working on this, I detected that the backup state never gets cleared from the store while the browser tab is active. This means is the user connects with a backup, then logs out and reconnets again, it would again use the backup state and overwrite any change that got applied in-between or causes a failure at the SDK. To fix this, the backup state gets cleared when the connection prodedure finishes.
The custom EthereumProvider type was a tool to act between the type conjunction of the SDKs interface and the actual connection methods as used by the dApp. This could be simplified to make sure that all connection methods return an JsonRpcProvider. In fact the last not unified method was the String as direct URL of an Json RPC server. Within the SDK this is a one-liner to pack it into a JsonRpcProvider object. As it (heavily) simplifies the typing in the dApp it is good to just do the packing right here. This is especially attractive when having the planned connection manager in mind.
Codecov Report
@@ Coverage Diff @@
## master #2702 +/- ##
==========================================
- Coverage 92.26% 92.22% -0.04%
==========================================
Files 176 177 +1
Lines 7224 7205 -19
Branches 1208 1199 -9
==========================================
- Hits 6665 6645 -20
- Misses 467 473 +6
+ Partials 92 87 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
taleldayekh
approved these changes
May 6, 2021
This commit does not add any new feature to the dApp, but refactors the design how the dApp gets an Ethereum connection and account to instantiate the SDK. This new approach has been developed to prepare the The new design introduces an abstract class which represents an Ethereum connection, holding the actual network/RPC provider and the account to use for signing. This abstract class gets them implemented by all the various connection methods the dApp provides. The intention is to have a nice and clean type system where the logic to instantiate the SDK and connect the dApp does not care about where the connection comes from, but just takes everything that implements this interface. Thereby the logic handling the connection process can be easily decoupled and stays static, no matter how many new connection methods get added or removed. Furthermore is must not care (not yet ready) about how the connections get configured. This allows the user to take interactively control over the configuration instead of a static file served by the web-server. After all, for the moment, the Home view component that handles the connection just gets a connection instance and passes its properties to the SDK. The selection and instantiation of the connection however is now done in the former components was well. This is the part that becomes replaced with the connection manager at a later point. Furthermore the does this approach improves the testability of the touched components. The connection process module is static and independently testable from the connection method. And each connection method can be tested individually. Mocking becomes (theoretically) easy. All by the bounding interface that tights everything together.
weilbith
force-pushed
the
feature/dapp-refactor-ethereum-provider
branch
from
a0e2f29
to
589ba42
Compare
|
I fixed the stupid linting issues (it was a missing semicolon). |
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.
Preparation for #2689 #2630 #2631 #2598
Short description
This adds no new feature and just shapes/refactors code.
I spend a lot of time to make sure everything remains working as is. Especially the end-to-end tests where quite helpful for many cases. Though the injected provider does not get tested with it and required manual work.
No CHANGELOG entry required.
I had a tough time with Jest mocking system. I'm not 100% happy about the shape of the mocks. But we are on a clock right now.
I did not test after the final rebase on master where I had to fix some conflicts. I trust the CI pipeline.
Some commits in-between had wrong formatting applied. I locally have a slightly different formatting for rare cases. Unfortunately I have not git commit hook. As I had serious issues with rebasing fixup commits and making manual edits to old commits I did not again solved those minor issues. Sorry.
PS:
@taleldayekh if you like we can review this PR together in the office to save time and keep you motivation high for such a big one.
Definition of Done
Steps to manually test the change (dApp)