React redux implementation, fetching current price of BTC

[ioay]

Closes #236 #121

---

Initialize Redux support to store state

Show values in USD in Ledger Live dApp

---

What was done:

• Added packages: react-redux, ver. ^9.1.0 | @reduxjs/toolkit", ver: ^2.2.0
• Disabled eslint no-param-reassign rule for slice files. Redux Toolkit uses Immer under the hood to write simpler immutable update logic using "mutating" syntax.
• Updated asyncWrapper to logPromiseFailure method to catch promises.
• Added useCurrencyConversion hook to calculate values from BTC to USD. It should be updated to handle other currencies if appears in the redux store.
• Added useFetchBTCPriceUSD hook to get bitcoin price from Coingecko API.
• redux dev tools are enabled for all environments, excluding PROD env.

---

Preview:

[kkosiorowska]

I think in this PR we could create our first slice and check that the state update as well as the selectors work properly, instead of just creating a slice template. As I mentioned below I think we could do it for a state of turn off/on USD value. This is a simple true/false value. What do you think?

[ioay]

PR is ready for review.

[kkosiorowska]

I've left some comments. I also noticed that to close #345 we need to add support for our toggle of showing prices in USD.

[kkosiorowska]

I wonder if it's a good idea to wrap it up in try catch block. We could then handle situations when we receive some error from an external server.

[ioay]

We use asyncWrapper to handle this thunk in useInitApp.

[kkosiorowska]

Yes, but when something fails we do not log the error. That's why I'm wondering about it. We can also control the returned value when something fails.

[ioay]

It's exactly what asyncWrapper does here:

useEffect(() => {
    asyncWrapper(dispatch(fetchBTCPriceUSD()))
  }, [dispatch])

[Shadowfiend]

A quick (and, as ever, nonblocking) note: let's document what asyncWrapper is actually meant to do alongside the function declaration… And probably consider renaming it accordingly as well. It's extremely unclear what its purpose is right now, and reading the history of why it was added it seems like it was… Strictly to evade an eslint rule? If so, we should treat that as an anti-pattern.

If I understand correctly, what Karolina is asking here is exactly what that eslint rule that triggered adding asyncWrapper is meant to catch: if there's an error, it's just swallowed by the thunk, and with asyncWrapper it's also swallowed when it's invoked. Additionally, because we've attached a .catch in asyncWrapper, we won't get the default UnhandledPromiseRejection in the console/on error reporting endpoints like Sentry (via unhandledrejection)—and all of this escapes React's error boundary handling as well (vs e.g. using the external useErrorBoundary hook).

[ioay]

A quick (and, as ever, nonblocking) note: let's document what asyncWrapper is actually meant to do alongside the function declaration… And probably consider renaming it accordingly as well. It's extremely unclear what its purpose is right now, and reading the history of why it was added it seems like it was… Strictly to evade an eslint rule? If so, we should treat that as an anti-pattern.

If I understand correctly, what Karolina is asking here is exactly what that eslint rule that triggered adding asyncWrapper is meant to catch: if there's an error, it's just swallowed by the thunk, and with asyncWrapper it's also swallowed when it's invoked. Additionally, because we've attached a .catch in asyncWrapper, we won't get the default UnhandledPromiseRejection in the console/on error reporting endpoints like Sentry (via unhandledrejection)—and all of this escapes React's error boundary handling as well (vs e.g. using the external useErrorBoundary hook).

Agreed, we should deal with asyncWrapper or just find another solution to handle promises in the app, it was created to solve errors reported by eslint (#179 (comment)). Initially, we handled it separately for each Promise.

At the moment I've just added try/catch block to handle thunk error: 3f07d13

Added task to deal with it: #260

[Shadowfiend]

Fwiw I think you can get a big improvement just by making asyncWrapper something like logPromiseFailure:

/**
 * If the promise fails, log the underlying error but maintain the failed
 * promise.
 *
 * Does nothing to successful promises.
 */
function logPromiseFailure<T>(promise: Promise<T>): Promise<T> {
  return promise.catch((error) => {
    console.error(error)
    throw error
  })
}

This doesn't solve the problem of “what do we do with errors”, but it makes sure that errors aren't swallowed wholesale, makes that decision explicitly visible to anyone reading the code, and leaves you with a clear hook to plug into Sentry or some other error management system.

[ioay]

2426636

[kkosiorowska]

I just noticed this task #237. So we can close this task without it. I left a few more comments.

[kkosiorowska]

Yes, but when something fails we do not log the error. That's why I'm wondering about it. We can also control the returned value when something fails.

[kkosiorowska]

Non-blocking comment
One thing wonders to me. Is there any possibility of previewing the state of redux when we open our dApp in ledger live app?

[r-czajkowski]

Non-blocking comment.

Maybe too late but I would consider using TanStack Query instead of redux at least for this feature. We can cache API response and we can set a time when the data should be re-fetched etc see https://tanstack.com/query/latest/docs/framework/react/reference/useQuery especially: staleTime and gcTime options. My point is that if we add something else to btc store we need to duplicate the logic for loading and errors indicator and we have a lot of boilerplate to create a simple feature such as fetching USD price.

We could use redux to store data that should be updated based on the emitted on-chain events but it should probably be possible with this lib as well.

[ioay]

At first glance, it looks good, but I wonder if instead of TanStack Query we should think about using of addon to the already installed redux-toolkit package: https://redux-toolkit.js.org/tutorials/rtk-query. What do you think?

[r-czajkowski]

Looks good too. But I would go with TanStack Query - it looks cool and powerful, is well-documented, there are a lot of examples, and is popular and well-maintained (38k stars on GitHub, only 30 issues and 18 opened pull requests). We can achieve all the stuff we need with TanStack Query w/o redux and we can easily plugin GraphQl which will be needed once we create our subgraph for Acre network.

[ioay]

Added a task to deal with TanStack Query: #277

[ioay]

After removing throw error we no longer meet typescript requirements, so I've decided to add return 0 instead of returning undefined, because if we do not retrieve the data, it is better to display the value 0.

In my opinion, in the near future, we should consider keeping certain states in localStorage or caching this data when TanStack Query will be added, and in case of an error: the previous value is displayed to the user. What do you think? @kkosiorowska @r-czajkowski @nkuba @Shadowfiend

[netlify]

‼️ Deploy request for acre-dapp-testnet rejected.

Learn more in the Netlify docs →

Name	Link
🔨 Latest commit	fee4422

[kkosiorowska]

Actually, we are ready to merge it. 🚀 I have only one doubt, I left in the comment. I don't want to block the PR by this. So if this part needs improvement we will fix it next one.

I have one more question. Is there any possibility of previewing the state of redux when we open our dApp in ledger live app?

[kkosiorowska]

I wonder if this is the correct approach in this case. 🤔 It seems to me that we should pass here only what we need. Without that useCurrencyConversion has access to other properties anyway.

cc @r-czajkowski

[ioay]

In general, this is a classic approach and there is nothing wrong with it as long as the object contains the necessary data of a certain type. Objects in JavaScript are passed by reference, so in terms of performance, there is nothing wrong with it either.

[ioay]

Actually, we are ready to merge it. 🚀 I have only one doubt, I left in the comment. I don't want to block the PR by this. So if this part needs improvement we will fix it next one.

I have one more question. Is there any possibility of previewing the state of redux when we open our dApp in ledger live app?

Good question, Ledger uses the Electron browser under the hood, so redux dev tools will not be immediately available from the console in Ledger Live. I'll create a separate story to figure out a solution.

#298