search

decred / decrediton

Cross-platform GUI for Decred.
https://docs.decred.org/wallets/decrediton/decrediton-setup/
ISC License
195 stars 120 forks source link

[refactor] Clean up actions and reducers #1558

Open alexlyp opened 6 years ago

alexlyp commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I believe it is related with https://github.com/decred/decrediton/issues/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