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

Audit / Improve Async State Management #236

Closed
dternyak opened this issue Sep 26, 2017 · 5 comments
Closed

Audit / Improve Async State Management #236

dternyak opened this issue Sep 26, 2017 · 5 comments
Assignees
Labels
type: enhancement Items that improve overall user experience and/or expand on existing features.

Comments

@dternyak
Copy link
Contributor

Currently, we do not keep track of all state of a network request / async action.
By keeping track of the state of a network request, we can show users spinners/give them feedback while the async action resolves.

In addition to auditing and improving state management, it would be helpful to standardize on a specific format for keeping state of a specific network.

E.g. isLoading / hasErrored / errorMessage

@dternyak dternyak added the type: enhancement Items that improve overall user experience and/or expand on existing features. label Sep 26, 2017
@dternyak
Copy link
Contributor Author

MyEtherWallet/MyEtherWallet#149 is an example of async state not being handled properly.

@wbobeirne
Copy link
Contributor

I think this one can be tricky, since many redux states require multiple loaders. For instance, the wallet reducer should have one loader for its ETH value, and separate loader(s) for the token values, and separate loader(s) for equivalent currency values. Having one and only one key for loading doesn't give us that level of granularity.

@tayvano
Copy link
Contributor

tayvano commented Sep 29, 2017

It should also be noted that token balances are loaded via parity and eth balances, transaction count, and other items necessary to send a transaction are loaded via geth. This is to prevent token balances not loading from preventing a user from sending a transaction in times of very high traffic, such as during ICOs.

@dternyak
Copy link
Contributor Author

dternyak commented Sep 29, 2017 via email

@dternyak dternyak added this to the Sprint 2 milestone Oct 17, 2017
@dternyak dternyak modified the milestones: Sprint 2, Sprint 4 Nov 15, 2017
@dternyak dternyak assigned eddiewang and unassigned james-prado Nov 15, 2017
@eddiewang
Copy link
Contributor

eddiewang commented Nov 27, 2017

I want to outline what my research has been so far and the strategy to tackle this issue will be here:

In my research dealing with the RPC calls so far it seems like there isn't we don't always handle errors currently. In issues like #462 #272 and #372 alot these errors cause certain functions to stop working on MEW and worse, sometimes they're swallowed, so we don't even know if they're there.

In my Error Auditing PR #384 I have started the preliminary work to fix this by adding JSONSchema validators to the RPC calls. Since we expect the data returned to be represented a certain way, if it deviates from that specific object format, our UI won't be able to handle it, so we should throw an error.

Now, we should look at the sagas and ensure that each error is not only properly detected, rather than just calling a showNotification on error catching, we should update the state objects in a unified manner by modifying the action creators.

Strategy:

  1. Create JsonSchema validators for all outbound api calls and make sure all error responses are thrown.
  2. Ensure no side effects are being called from action creators, or anywhere else... - all external calls should be contained within redux-saga unless there is a good reason not to do so.
  3. Create a standardized api call format (isLoading, hasErrored, errorMessage would be solid i think) for Redux... possible a wrapper function that makes this simple to wrap around current requests? Still doing research on this part....
  4. Create integration tests for the core api requests, such as wallet node calls: getBalance, getTokenBalance, etc.

@dternyak dternyak removed this from the Sprint 4 milestone Dec 1, 2017
@dternyak dternyak closed this as completed Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Items that improve overall user experience and/or expand on existing features.
Projects
None yet
Development

No branches or pull requests

5 participants