Feature/transform cli to rest api #1636
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
|
|
Codecov Report
@@ Coverage Diff @@
## master #1636 +/- ##
==========================================
- Coverage 97.75% 95.87% -1.88%
==========================================
Files 63 146 +83
Lines 3657 5217 +1560
Branches 845 1016 +171
==========================================
+ Hits 3575 5002 +1427
- Misses 70 162 +92
- Partials 12 53 +41
Continue to review full report at Codecov.
|
kelsos
reviewed
Jun 5, 2020
kelsos
reviewed
Jun 5, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
andrevmatos
reviewed
Jun 7, 2020
There was a problem hiding this comment.
I left some comments there. It definitely already improves a lot on what we had, but some things could be improved to drive it on a more independent, reusable and explicit implementation, we can discuss if all suggestions make sense or there's any input needed. Thanks for this!
Closed
|
@andrevmatos there are some |
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
andrevmatos
reviewed
Jun 8, 2020
3 tasks
2 tasks
|
@andrevmatos I'm not sure how to solve the CI issue by |
We can ignore it. |
|
I went and manually approved but maybe we have to adjust our configuration to 5. Let's discuss it tomorrow. |
andrevmatos
reviewed
Jun 9, 2020
The whole `initRaiden` implementation has been removed, since the new implementation should primarily focus on the Raiden API support. The Raiden SDK instance has been extended into a singleton, making it easily available for all part of the app (especially the endpoint implementations). The main function become more structured, exporting the details into functions with proper naming, allowing the reader to understand the basic flow.
Parameter provided by the user must be validated. The goal was to keep the router description easy to read, while having the full power of validation. Therefore it uses describing names for validation chains, while keeping them configurable at the same time. The validation itself is imperative to simplify the middleware stack for the routes as well as make it better readable.
This includes small refactoring for the whole setup how the Raiden service gets started. Anyways, the 'ready' state as the API specification defines it not 100% correct at the moment and will need some attention.
This includes mainly the creation of new channels and retrieving open channels with different filters applied. All routes attach their channels as result to the request. A shared middleware convert their format according to the API specification and send them to the user. The format transformation might not be 100% correct at the moment due to data model differences. They must be analyzed and resolved. Furthermore has the shutdown process of the Raiden service been improved. Thereby it is able (again) to stop on receiving error events.
The intention is to become more consistent with other projects.
When the user cancels the running application, the shutdown procedure gets triggered. It stops the REST server and also stops the raiden instance. Depending on still open connections etc, this might take a while. If the user sends a second cancel signal, the whole process should be killed without waiting anymore. This was the former behavior that has been restored now.
Taking the raiden-hub express implementation as inspiration, there should be a global configuration how the application logs data. This happens in a separate module that can be imported and used everywhere. This also partially simplified the current code.
The following design decision has been made: The raiden sdk should be not wrapped and provided by a singleton. Rather it should be a global variable that becomes attached to functions by their context. Therefore each module explicitly exports a "make" function. This allows us to bind or call them with the global context, including all instances they need. Thereby the raiden instance (also the logger as another example) gets passed down up to the routes of the server. The key advantage is that it is now possible to modify the global context at any place, but take effect everywhere. Moreover does this functional approach allow to test the components easily on their own. It is so modular, that it can be easily re-used for any other use-case rather than start it from the command-line. As addition to that, it has been decided that the routes should include the main logic of the endpoints. If someone likes to checkout a simple reference implementation for the sdk for a specific case, he can simply checkout the according route. Therefore I just exported utility functions that are used across the routes, making code reusable and easier to read. The parameter validation of the re-implemented channel endpoints have been removed for now. It is affected by another major design decision, part of upcoming commits. Related to that, it is not possible to create new channels for this commit.
It has been decided that the cli should validate as less user parameters as possible. The goal is to let the sdk doing the heavy lifting, including the parameter validation. The API layer then simply parses possibly thrown errors, including wrong parameter (formats). This will become mixed with the other errors for cases where the balances is not enough and similar cases. The remaining validation is for cases where the sdk can not do the work and also should not. An example would be the filtering of channels by token and partner address. The API specification requires to send error codes for male formatted address parameter. Empty response lists are actual valid results.
Keep the code clean and put configuration into a dedicated file that can be imported.
Instead of using the global logger, a dedicated instance should be used per Cli (and server) instance. The log level modification is still set global and applies for the later created logger. The logger identifies itself by the unlocked address.
To get an overview of in- and outgoing connections to the server, the morgan logger has been added as middleware. The log format takes the light client hub server as reference, while logging all connections, not only ones with error codes.
The main target of this guide is to fix the made design designs that should be followed.
weilbith
force-pushed
the
feature/transform-cli-to-rest-api
branch
from
e38697a to
75ea635
Compare
andrevmatos
force-pushed
the
feature/transform-cli-to-rest-api
branch
from
229c41e to
2e878cf
Compare
andrevmatos
force-pushed
the
feature/transform-cli-to-rest-api
branch
from
2e878cf to
8e60d9e
Compare
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.
Related to: #173
Short description
This PR is not implementing the full Raiden API specification. Rather it is implementing the basics for the application, attempting to do the heavy lifting for future development. The idea is to get some early feedback for continuing to developing the rest of the API. This PR is already huge enough (the poor reviewer 😉).
Having this base of code should also allow us to parallelize work and set new priorities for the different endpoints (like for the train demo). Nevertheless there are already some endpoints implemented. User parameter validation is established for all endpoints of the spec.
Please watch-out for some
TODOmarkers. They refer to "issues" or lack of knowledge with/of the SDK. Feel free to simply fix them. In general I'm totally with initiations to improve the code yourself if you have a strong opinion.I'm sorry that especially my first PR in first project starts so big. But I'm happy about the solution to split this epic issue now. I hope the code is good to read, it was my primary goal. The commit history is not actually meant to get merged like this, since the story of this issue was relatively chaotic. I guess we should do a final squash to keep our history clean. Future issues should be smaller and PRs can be more focused, contributing to a clean history.
Definition of Done