Tokens state refactor #300

r-czajkowski · 2022-11-09T12:02:28Z

Depends on: #299

This PR moves the contract integration part from hooks to the Threshold-ts library. Thanks to this layer we can separate work between UI and contract integration. Also, it helps to unit test created services and move the "business logic" from the React hooks.

Main changes

Tokens in Threshold-ts lib

Create an interface and implementation of the ERC20 token and a Tokens class "consumer" that creates all tokens used in T dapp to fetch necessary data.

React

Move fetching token balances to the listener middleware as a one-shot listener and effect callback will be run on walletConnected action.
Clean up tokens redux state- remove the unnecessary conversionRate from a token state, only KEEP and NU has conversion rate to T but the store for tokens must be generic so it's unnecessary to define conversionRate for each token. We have a nice solution for storing the vending machine ratio in local storage.
Refactor the useERC20 hook- use the ERC20 wrapper from the threshold ts lib.
Get rid of the TokenContextProvider- this context provider is now unnecessary. We moved fetching token balances to redux listener middleware and we can get the token state from redux so there is no need for an additional context provider.

Create a wrapper for erc20 token. Also create a `Tokens` class- a wrapper for all tokens related to the threshold network.

The main change are: - remove the unnecessary `conversionRate` from a token state, only keep and nu has conversion rate to T but the store for tokens must be generic so it's unnecessary to define `conversionRate` for each token. We have nice solution for storing the vending machine ratio in local storage. - clean up the token context- fetching the token balances using the threshold-ts lib using listener middleware. It's a one-shot listener and effect callback will be run on `walletConnected` action. We will probably get rid of the `TokenContext` in a follow-up work.

Use the ERC20 wrapper from the threshold ts lib.

This context provider is now unnecessary. We moved fetching token balances to redux listener middleware and we can get the token state from redux so there is no need for additional context provider.

github-actions · 2022-11-09T12:19:38Z

Preview uploaded to https://preview.dashboard.test.threshold.network/tokens-threshold-lib/index.html.

michalsmiarowski

Left some small comments below

michalsmiarowski · 2022-11-21T05:46:12Z

src/threshold-ts/tokens/erc20/index.ts

+  }
+  balanceOf = (account: string): Promise<BigNumber> => {


Let's add new line here (after 31st line)

michalsmiarowski · 2022-11-21T06:02:01Z

src/threshold-ts/tokens/erc20/index.ts

+  approve = (spender: string, amount: string): Promise<ContractTransaction> => {
+    return this._contract.approve(spender, amount)
+  }


Don't we need async/await here?

michalsmiarowski · 2022-11-21T06:07:14Z

src/threshold-ts/index.ts

@@ -9,13 +10,15 @@ export class Threshold {
  staking!: IStaking
  multiAppStaking!: MultiAppStaking
  vendingMachines!: IVendingMachines
+  tokens!: ITokens


I would prefer to keep it as singular instead of plurar so we can call specific token as:

threshold.token.tbtc instead of threshold.tokens.tbtc.

What do you think?

michalsmiarowski · 2022-11-21T06:07:48Z

src/threshold-ts/tokens/index.ts

+  tbtc: IERC20WithApproveAndCall
+}
+
+export class Tokens implements ITokens {


If you agree with above then maybe we could also rename those to Token and IToken?

Hmm I agree with the above but imo it doesn't fit here because Token sounds like an abstract interface for all tokens eg erc20 or erc721. Maybe TokensWrapper here? I would like to point out here that it's a wrapper for all tokens that we use/are important in Threshold network.

michalsmiarowski · 2022-11-21T06:30:54Z

src/store/tokens/effects.ts

+    { token: tbtcv1, name: Token.TBTC },
+    { token: tbtc, name: Token.TBTCV2 },


Can we change the names in Token enum?

Token.TBTC -> Token.TBTCV1
Token.TBTCV2 -> Token.TBTC

I think that we had a discussion about this and we decided to name tbtc-v2 occurences in dApp as tbtc and old tbtc as tbtc-v1

I think that we had a discussion about this and we decided to name tbtc-v2 occurences in dApp as tbtc and old tbtc as tbtc-v1

yep, comments here: #178 (comment)

michalsmiarowski · 2022-11-21T06:46:28Z

src/store/tokens/tokenSlice.ts

+    setTokenBalanceError: (
      state,
-      action: PayloadAction<SetTokenConversionRateActionPayload>
+      action: PayloadAction<SetTokenBalanceErrorActionPayload>
    ) => {
-      const { token, conversionRate } = action.payload
-
-      const formattedConversionRate = numeral(
-        +conversionRate / 10 ** 15
-      ).format("0.0000")
-
-      state[token].conversionRate = formattedConversionRate
+      const { token } = action.payload
+      state[token].loading = false


Can you explain why we need that action? It doesn't set any error in the store so the name of it could be misleading. I think we could refactor the names:

setTokenLoading -> tokenLoading
setTokenBalanceError -> tokenLoaded

if we want to just keep the true/false assignment to loading property in separate actions

michalsmiarowski · 2022-11-21T06:55:05Z

src/web3/hooks/useERC20.ts

      }
    },
-    [account, contract]
+    [token, tokenName, setTokenLoading, setTokenBalanceError]


I think we should also add setTokenBalance to the dependency arrray

michalsmiarowski

Small issue with tests

michalsmiarowski · 2022-11-21T06:59:06Z

src/contexts/TokenContext.tsx

-  contract: Contract | null
-}
-
-export const TokenContext = createContext<{


useFetchTvl test fails because it can't find Token Context

We agree that we prefer to keep it as singular instead of plural so we can call specific token as: `threshold.token.tbtc` instead of `threshold.tokens.tbtc`.

`Token.TBTC` -> `Token.TBTCV1` `Token.TBTCV2` -> `Token.TBTC` In terms of threshold staking applications, we have only tbtc v2 app so I think from code's/T dapp's perspective is ok to use tbtc w/o v-2 because it's obvious that only tbtc-v2 works on threshold network. We only added the tbtc v1 token to get the token price in usd and calculate tvl. Ref: #178 (comment)

Trying to treat actions more as "describing events that occurred", rather than "setters" as recommended in official redux docs.

github-actions · 2022-12-01T13:53:27Z

Preview uploaded to https://preview.dashboard.test.threshold.network/tokens-threshold-lib/index.html.

r-czajkowski added 4 commits November 7, 2022 15:49

Create the erc20 token wrapper

3ec80ee

Create a wrapper for erc20 token. Also create a `Tokens` class- a wrapper for all tokens related to the threshold network.

Refactor token hooks

3b1b8a4

Use the ERC20 wrapper from the threshold ts lib.

Remove the Token Context Provider

4f7cf47

This context provider is now unnecessary. We moved fetching token balances to redux listener middleware and we can get the token state from redux so there is no need for additional context provider.

r-czajkowski requested review from georgeweiler and michalsmiarowski November 9, 2022 12:02

r-czajkowski mentioned this pull request Nov 10, 2022

Staking interface updates #302

Open

r-czajkowski added the 📜 contract integration label Nov 14, 2022

r-czajkowski marked this pull request as ready for review November 18, 2022 08:54

Base automatically changed from threshold-lib-updates to main November 18, 2022 12:46

michalsmiarowski reviewed Nov 21, 2022

View reviewed changes

michalsmiarowski requested changes Nov 21, 2022

View reviewed changes

michalsmiarowski assigned r-czajkowski Nov 22, 2022

r-czajkowski added 6 commits November 28, 2022 13:58

Use async/await in ERC20 class methods

4220a1b

Rename wrapper for all tokens in Threshold lib

9973b5b

We agree that we prefer to keep it as singular instead of plural so we can call specific token as: `threshold.token.tbtc` instead of `threshold.tokens.tbtc`.

Update token slice action names

e48d027

Trying to treat actions more as "describing events that occurred", rather than "setters" as recommended in official redux docs.

Add missing value to the hook deps array

7ea246e

Fix useFetchTvl hook unit test.

9d1f351

r-czajkowski requested a review from michalsmiarowski November 28, 2022 15:06

Merge branch 'main' into tokens-threshold-lib

f5e8a61

r-czajkowski mentioned this pull request Apr 14, 2023

tBTC stats on overview page #490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tokens state refactor #300

Tokens state refactor #300

r-czajkowski commented Nov 9, 2022 •

edited

Loading

github-actions bot commented Nov 9, 2022

michalsmiarowski left a comment

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

michalsmiarowski left a comment

michalsmiarowski Nov 21, 2022

r-czajkowski Nov 28, 2022

github-actions bot commented Dec 1, 2022

		{ token: tbtcv1, name: Token.TBTC },
		{ token: tbtc, name: Token.TBTCV2 },

Tokens state refactor #300

Are you sure you want to change the base?

Tokens state refactor #300

Conversation

r-czajkowski commented Nov 9, 2022 • edited Loading

Main changes

Tokens in Threshold-ts lib

React

github-actions bot commented Nov 9, 2022

michalsmiarowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsmiarowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2022

r-czajkowski commented Nov 9, 2022 •

edited

Loading