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

Consider not persisting exchange rate history #6284

Closed
gnardini opened this issue Dec 17, 2020 · 1 comment · Fixed by valora-inc/wallet#255
Closed

Consider not persisting exchange rate history #6284

gnardini opened this issue Dec 17, 2020 · 1 comment · Fixed by valora-inc/wallet#255
Assignees
Labels
Component: CELO enhancement an improvement to an existing feature Priority: P1 Critical wallet

Comments

@gnardini
Copy link
Contributor

Expected Behavior

Exchange rate history is not persisted in the Redux state. See if it's cached at the Firebase lib layer.

Current Behavior

We have a bug in our code where the exchange rate history gets stored multiple times in our redux state.

Quoting from this issue: #6273

On one of my tests, I noticed that the size of the Redux store was getting bigger and bigger. Upon closer inspection, it was the history we use for the CELO/cUSD exchange rate that was growing at a rate of ~100kb sometimes when restarting the app. [...]
There are more details on what is failing on that PR, but unlimited growth of the persisted Redux state is definitely concerning.

One comment by @jeanregisser was:

Interesting, it looks like we can even go as far as not persisting the exchange rate history in Redux, as I think it's already cached at the firebase lib layer.

@tarikbellamine
Copy link
Contributor

Upping priority to P1 because we have reason to believe this is bloating redux state to the point that it's triggering app crashes

mergify bot pushed a commit that referenced this issue Jan 27, 2021
…on (#6488)

### Description

This PR enables Flipper on iOS along with `Redux Debugger` and `Reactotron` plugins.

This allows viewing / debugging the following:
- React DevTools (Components and Profiling)
- Network connections
- View hierarchy
- Redux State / Actions
- AsyncStorage
- App preferences
- Hermes
- and more ;)

It also opens the door for creating/adding our own Flipper plugins (for instance for viewing transactions 😉).

Setup instructions were added to the mobile README.

### Details

When we upgraded to React Native 0.63.x and Reanimated 2, the introduction of TurboModules broke the ability to use remote debugging with the Chrome DevTools, see [this discussion with more details](react-native-community/discussions-and-proposals#206).

As an alternative we can use Flipper to provide similar debugging functionalities.

Flipper was already working on Android, but on iOS there was a conflict between the OpenSSL lib shipped as part of `react-native-fast-crypto` and the one included by `Flipper`. To workaround this problem, we now use [a "headers" only version of `OpenSSL`](https://github.com/celo-org/OpenSSL-headers) that makes `Flipper` happy while building. 

The 2nd issue was that Flipper in React Native doesn't work with `use_frameworks!` enabled in the `Podfile`, which we need because we have Swift dependencies, which don't work without it.
I bent CocoaPods a little more so that everything Flipper related is built as static frameworks.

I've integrated both `Redux Debugger` and `Reactotron` as they provide different lenses to debug the app.
There's also a `redux-saga` plugin for `Reactotron` but it triggered some odd unhandled promise rejections, so I didn't include it.

While working on this I noticed the state in the `exchange` reducer is very big and made the `Redux Debugger` plugin quite unusable within Flipper. There's an issue open which should address this (#6284). In the meantime I've filtered out that part of the state in `Redux Debugger`. `Reactotron` can still be used to see it and is not affected.

Overall this gives us more tools to see what is happening within the app.

### Tested

- Flipper works on both iOS and Android along with their `Redux Debugger` and `Reactotron` plugins.
- iOS release builds don't include Flipper (Flipper modules are built for release builds, but NOT linked - this makes the build take more time, but unfortunately no simple way around this with CocoaPods for now, or we'd need to manage multiple `Podfile.lock` files, see facebook/flipper#1275).

<img width="1554" alt="Screenshot 2021-01-15 at 11 38 52" src="https://user-images.githubusercontent.com/57791/104722566-4bfe4800-572e-11eb-973c-63c7af58b94f.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 39 19" src="https://user-images.githubusercontent.com/57791/104722570-4f91cf00-572e-11eb-9bb7-4fe0abf4336c.png">
<img width="1554" alt="Screenshot 2021-01-15 at 12 30 47" src="https://user-images.githubusercontent.com/57791/104722693-836cf480-572e-11eb-8ef0-83e54a52dfe8.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 40 26" src="https://user-images.githubusercontent.com/57791/104722580-5587b000-572e-11eb-886e-8fff2da8cffd.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 40 48" src="https://user-images.githubusercontent.com/57791/104722594-5a4c6400-572e-11eb-86bd-873add6027b1.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 47 17" src="https://user-images.githubusercontent.com/57791/104722632-6801e980-572e-11eb-8412-d86a8a29d383.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 49 12" src="https://user-images.githubusercontent.com/57791/104722649-6df7ca80-572e-11eb-8ac5-bae41abd6488.png">
<img width="1554" alt="Screenshot 2021-01-15 at 11 49 32" src="https://user-images.githubusercontent.com/57791/104722659-7223e800-572e-11eb-9734-59512f3987f9.png">

### Related issues

- Discussed on Slack: https://celo-org.slack.com/archives/CL7BVQPHB/p1610040529072600

### Backwards compatibility

Yes
@gnardini gnardini self-assigned this Apr 19, 2021
mergify bot pushed a commit to valora-inc/wallet that referenced this issue Apr 20, 2021
### Description

We had a bug in the fetching of the exchange rates that made us fetch the whole thing every time there was an update, which is every 30 minutes. The number of elements to fetch each time depended on how much time it passed since the last use of the app, for new users and for users that didn't open the app in a couple months we would load all the history for three months.

We were also fetching the whole exchange rate history from Firebase and filtering it in memory instead of only fetching the latest three months. We tried to filter, but we were sending `1` as `startAt` which returns everything.

With the current changes, we load everything since the max between the last update and three months ago and then we listen only for new items, we don't reload everything again.

### Other changes

- Added a migration to delete the exchange rate history from Redux since it was likely that it was repeated a bunch of times.

### Tested

Mostly manually, by making changes and adding and modifying items in the RTDB to see that the behavior was the expected one.

### Related issues

- Fixes celo-org/celo-monorepo#6284

### Backwards compatibility

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CELO enhancement an improvement to an existing feature Priority: P1 Critical wallet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants