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

[refactor] Clean up actions and reducers #1558

Open
alexlyp opened this issue Jul 27, 2018 · 4 comments
Open

[refactor] Clean up actions and reducers #1558

alexlyp opened this issue Jul 27, 2018 · 4 comments

Comments

@alexlyp
Copy link
Member

alexlyp commented Jul 27, 2018

Now that 1.3.0 is nearing completion I'd like to open up discussion for some possible places to begin cleaning and refactoring various rough spots of the codebase.

I would like to streamline the actions and reducers somewhat. Currently all actions (that don't open streams) have roughly the same form: XXXX_ATTEMPT, XXXX_FAILED, XXXX_SUCCESS. This seems like it would lend itself to reusing of a shared type for them all.

And while we are updating that, it seems reasonable to begin to fix the similar situation we have for the state of these actions as well. Roughly speaking we should be able to determine if an action is in flight, its request contents (if necessary), any errors, and the response received. Again, this should be able to be reduced down into a shared type that can be reused by most of the actions.

I'd also like to possibly do an audit of our state actions currently to confirm we are conforming to immutability requirements. I believe we are, but worth looking into to confirm this is the case.

@matheusd
Copy link
Member

I would like to streamline the actions and reducers somewhat. Currently all actions (that don't open streams) have roughly the same form: XXXX_ATTEMPT, XXXX_FAILED, XXXX_SUCCESS. This seems like it would lend itself to reusing of a shared type for them all.

Agree they can be streamlined a bit. I only see two possible gotchas with this:

  1. The solution can't be so generic that it ends up doing "nothing" and requiring reimplementation of logic, just at a deeper abstraction level (ie: just moving the XXXX into a dispatched variable that still requires testing everywhere)

  2. It needs to account for having multiple concurrent requests. Some of those XXX_ATTEMPT flags are used to prevent doing the same requests twice (eg getTicketsInfoAttempt()). so there is still some need to know which requests are in flight.

I'd also like to possibly do an audit of our state actions currently to confirm we are conforming to immutability requirements. I believe we are, but worth looking into to confirm this is the case.

Agreed there.

While we're at it, let's remove the last direct references to grpc structures in the client and control actions. Everything should be referencing the wallet module. Eyeballing it, there seems to be only 2 requests left in ControlActions (construct tx and rescan).

@alexlyp
Copy link
Member Author

alexlyp commented Jul 30, 2018

Yah the rescan, and other upcoming SpvSync, use response streams which I was unsure how to handle to put into the wallet module. But agree they should be made into the same form as well.

@alexlyp
Copy link
Member Author

alexlyp commented Jul 30, 2018

Yah I was mostly thinking about code consistency and cleanliness when thinking about streamlining the actions. Currently seems pretty messy, and while easy for us that have been around the codebase a while to add new actions, I could see it being difficult for newcomers to understand.

@vctt94
Copy link
Member

vctt94 commented Aug 7, 2018

I believe it is related with #799.

Have you guys see how the requests are made at politeiagui? I believe we can do something similar with that architecture, as it make very simple to add new features and it is pretty generic, I think.

Here is an example: https://github.com/decred/politeiagui/blob/master/src/reducers/api.js

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

No branches or pull requests

3 participants