TokenBalance Component #97

AlexNi245 · 2021-12-12T20:14:45Z

Closes #42

Description

This component shows the amount of a certain ERC20 token that the connected user owns. Besides displaying it has the following features:

Fetching the owners balance of the selected token using the useTokenBalance hook
Calculation of the USD value of the balance by multiplying balance * provided USD/token rate
displaying the logos logo by using the trustwallet github repo
Fetching symbol and name of the token from the according smart contract

📝 Additional Information

Anything else you'd want reviewers to know.

During the work i noticed a few things. I would love to hear your opinions on this @Dhaiwat10 @etr2460 @Ibby-devv

Are we are using some common empty / error state across the library ? Currently, the behavior of the component on error is to display the balance of 0
I decided to not use APIs such as coinmarketcap to fetch the USD value because I think most users want to provide their own exchange rate. But I would be happy to discuss this.
I also decided to just display the first 3 decimal places of the value and the balance.
I think I could make sense to pass name and symbol as a prop rather than fetch these values from the contract. This would reduce loading times (during loading name and symbol are just empty string) and would display name and symbol anyway even there are problems with the web3 Provider.

Screenshot

changeset-bot · 2021-12-13T21:23:14Z

🦋 Changeset detected

Latest commit: 8ef2478

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@web3-ui/components	Minor
@web3-ui/hooks	Minor
@web3-ui/core	Patch
example	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Dhaiwat10 · 2021-12-14T05:49:51Z

@AlexNi245 sorry for not getting back to this yet. Since this PR has quite a bit of code I just need a bit more to review everything. Thanks for your patience mate

JoviDeCroock · 2021-12-14T08:47:06Z

.changeset/config.json

@@ -1,12 +1,9 @@
 {
  "$schema": "https://unpkg.com/@changesets/config@1.6.3/schema.json",
-  "changelog": [


Why are you removing this, this is repo config

Actually, this was not on purpose. Must have occurred during the upstream merge

I'll revert this

JoviDeCroock · 2021-12-14T08:47:46Z

package.json

+  "dependencies": {
+    "@web3-ui/hooks": "0.4.0"
+  },
+  "peerDependencies": {


It's already in the regular deps, we put it here only if we require the user to manually install the package in question

I tried to solve the failing Ci step by adding this. Unfortunately, it doesn't solve that issue. Have you an idea what to do?

Because you are importing from src

JoviDeCroock · 2021-12-14T08:49:42Z

packages/components/src/components/TokenBalance/TokenBalance.tsx

+  };
+
+  const Headline = (name: string) => (
+    <Heading as='h3' size='sm'>


Inline components can't maintain their referential identity and are subject to constant remounting. Hoisting outside of this closure will resolve that

AlexNi245 · 2021-12-14T08:51:39Z

No worries @Dhaiwat10 take your time! If there is anything I can help you with, just let me know.
Besides this, I have tried to solve the failing Ci steps. I've added "@web3-ui/hooks": "0.4.0" as a dependency and a peer-dependency but the checks are still failing. Do you know how to solve this? Code works fine on my local machine.

JoviDeCroock · 2021-12-14T08:56:24Z

packages/components/src/components/TokenBalance/TokenBalance.tsx

+import React, { useContext, useEffect, useState } from 'react';
+import { Box, Flex, Heading, Image } from '@chakra-ui/react';
+import { TokenLogo } from './components/Logo';
+import { useTokenBalance, Web3Context } from '@web3-ui/hooks/src';


@AlexNi245 cross package imports like these are not allowed in libs, reduce it to @web3-ui/hooks

Thank you for clearing this up. Wasn't aware of this.

JoviDeCroock · 2021-12-14T09:55:23Z

Your linter seems a bit goofy 😅 you will have to add the package here https://github.com/Developer-DAO/web3-ui/blob/main/packages/components/package.json#L30

JoviDeCroock · 2021-12-14T10:28:02Z

Just need to add a changeset "minor" and we're there, you can use the link here #97 (comment) or use yarn changeset at the repo root

packages/components/src/components/TokenBalance/TokenBalance.tsx

Yes, you're right I really like this pattern :) Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>

Dhaiwat10

Left some comments!

Dhaiwat10 · 2021-12-15T17:18:21Z

packages/components/src/components/TokenBalance/components/Logo.tsx

+import React from 'react';
+import { Image } from '@chakra-ui/react';
+
+export const TokenLogo = ({ tokenContractAddress }) => (
+  <Image
+    src={`https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/ethereum/assets/${tokenContractAddress}/logo.png`}
+  />
+);
+
+//i.E. Dai
+//https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/ethereum/assets/0x6B175474E89094C44Da98b954EedeAC495271d0F/logo.png


Can we move this component over to the TokenBalance.tsx? We don't need a separate file for this

You can also remove those comments

Dhaiwat10 · 2021-12-15T17:19:18Z

packages/hooks/src/Provider.tsx

@@ -89,7 +89,7 @@ export const Provider: React.FC<ProviderProps> = ({
    setConnection(connection);
    const provider = new ethers.providers.Web3Provider(connection);
    setProvider(provider);
-    const chainId = await provider.getNetwork().then(network => network.chainId);
+    const chainId = await provider.getNetwork().then((network) => network.chainId);


You should revert this since I don't think this is in the scope of this PR

Dhaiwat10 · 2021-12-15T17:19:25Z

packages/hooks/src/Provider.tsx

@@ -126,7 +126,7 @@ export const Provider: React.FC<ProviderProps> = ({
  const onAccountsChanged = async () => {
    const provider = new ethers.providers.Web3Provider(connection);
    setProvider(provider);
-    const chainId = await provider.getNetwork().then(network => network.chainId);
+    const chainId = await provider.getNetwork().then((network) => network.chainId);


Same as above!

Dhaiwat10 · 2021-12-15T17:20:02Z

packages/components/src/components/TokenBalance/components/Headline.tsx

+import React from 'react';
+import { Heading } from '@chakra-ui/react';
+
+export const Headline = (name: string) => (
+  <Heading as='h3' size='sm'>
+    {name}
+  </Heading>
+);


Can we also move this to TokenBalance.tsx?

Dhaiwat10 · 2021-12-15T17:21:55Z

packages/components/src/components/TokenBalance/TokenBalance.tsx

+  };
+
+  return (
+    <Flex borderRadius='lg' borderWidth='1px' overflow='hidden' px='4' py='2'>


Defining static styles and not letting the user set these styles if they want is not what we want. Can we accept all the props a Chakra Flex component would expect and spread them here? Our <AddressInput /> does the same for the Chakra Input component.

Dhaiwat10 · 2021-12-15T17:22:31Z

.changeset/config.json

+    {
+      "repo": "Developer-DAO/web3-ui"
+    }


Can you revert this, please?

Dhaiwat10

Can you make these small changes please and we're good to go

Dhaiwat10 · 2021-12-16T07:14:18Z

packages/components/src/components/TokenBalance/TokenBalance.tsx

+  const Headline = (name: string) => (
+    <Heading as='h3' size='sm'>
+      {name}
+    </Heading>
+  );


Like Jovi suggested:

Suggested change

const Headline = (name: string) => (

<Heading as='h3' size='sm'>

{name}

</Heading>

);

const Headline = (name: string) => {

return (

<Heading as='h3' size='sm'>

{name}

</Heading>

);

}

Dhaiwat10 · 2021-12-16T07:18:42Z

.changeset/config.json

-    "@changesets/changelog-github",
-    { "repo": "Developer-DAO/web3-ui" }
-  ],
+  "changelog": ["@changesets/changelog-github", { "repo": "Developer-DAO/web3-ui" }],


This should not be changed. Can you make sure this change goes away please?

sir, I almost lost my mind on this but I got the feeling this file was formatted by the commit hook. I have pushed it multiple times just to notice that this file still has unwanted changes.

Dhaiwat10 · 2021-12-16T07:19:09Z

packages/components/src/components/TokenBalance/erc.json

+[
+  {
+    "constant": true,
+    "inputs": [],
+    "name": "name",
+    "outputs": [
+      {
+        "name": "",
+        "type": "string"
+      }
+    ],
+    "payable": false,
+    "stateMutability": "view",
+    "type": "function"


Can you rename this file to ERC20.json instead?

Dhaiwat10 · 2021-12-16T07:19:55Z

packages/hooks/src/index.ts

+export {useWallet, useContract} from './hooks';
+export {useTokenBalance} from './hooks/useTokenBalance';
+export {Provider, Web3Context} from './Provider';
+export type {ProviderProps, Web3ContextType} from './Provider';
+export {NETWORKS, CHAIN_ID_TO_NETWORK, ERC20ABI} from './constants';


All of these lines should not be changed. Only the last line is actually being changed. Can you reset these?

This must be also the result of the post commit hook. I reverted the first few lines just to notice that they have changed again after I committed my changes.
I can use -no--verify again to restore the first 5 lines. But maybe it's intended that the commit hook applies certain formatting rules to each file even the affected file has nothing to do with the pr

I see. No worries then.

fixing index.js fix headline component

Dhaiwat10 · 2021-12-16T11:27:59Z

@AlexNi245 I think you commited some .idea files by mistake

Dhaiwat10 · 2021-12-16T12:01:26Z

packages/components/package.json

@@ -28,6 +28,7 @@
  "module": "dist/web3-ui-components.esm.js",
  "types": "dist/web3-ui-components.cjs.d.ts",
  "dependencies": {
+    "@web3-ui/hooks": "0.4.0",


One more thing that came to my mind recently is that the components package should not rely on the hooks package in any way. We want both of these packages to function well together but also independently.

This is why I feel like we shouldn't do any data fetching at all inside any of our components. Here is what I propose:

Move the current implementation to the core package.

Make the current implementation (inside components) so that it does not handle any data fetching but just accepts the data it needs to render.

Let me know if this is unclear. More than happy to discuss this in further detail since I think this is an important topic.

Do you mean this component shouldn't have any state at all and just renders given props ? Then we can remove the useTokenBalance hook from the Component.
Is that what you meant in the second point?

@AlexNi245 yes! The current verison of this component which does have some state and fetches data should be in the core package and not in the components package. This is what we did recently for #121 too.

tldr:

<TokenBalance /> in components shouldn't have any state, it should just render some data that you pass to it.

<TokenBalance /> in core should have state and should use the useTokenBalance hook

AlexNi245 · 2021-12-20T09:07:05Z

@AlexNi245 I think you commited some .idea files by mistake

Sorry for that, they are deleted now

Dhaiwat10 · 2022-01-03T15:53:12Z

@AlexNi245 just checking in. Do you need any help?

AlexNi245 · 2022-01-04T17:53:15Z

Hi @Dhaiwat10 actually I was on vacation since Christmas.I will come back home on Thursday. My apologies for not responding but I will apply your suggestions till the end of the week:)

Dhaiwat10 · 2022-01-04T22:40:46Z

No worries at all mate, enjoy!

# Conflicts: # packages/components/package.json # packages/components/src/components/index.ts # packages/hooks/src/index.ts

Dhaiwat10 · 2022-01-11T23:06:22Z

@AlexNi245 since this PR's history was extremely messed up, I copied your changes from here and opened a new PR here: #237. Closing this one

AlexNi245 added 8 commits December 3, 2021 22:32

initial skeleton of the TokenBalance component

9712a2e

first prototype of TokenBalance component

7c843ab

Merge remote-tracking branch 'origin/master' into fear/token-balance

f29734d

use useTokenBalance hook to determine token balance

065c576

add the ability to use own image by passing imageUrl

672bba0

fetch name and symbol from contract

2420626

add documentation

c650fb7

add story for invalid contract

911f8d6

AlexNi245 requested review from etr2460, Dhaiwat10 and Ibby-devv December 12, 2021 20:14

added the web-ui hooks components to dependencies

2ed6ea1

merge origin

69ec3b7

Dhaiwat10 requested review from Dhaiwat10 and removed request for Dhaiwat10 December 14, 2021 05:50

add web3-ui/hooks as a peerDependency

c2d16df

JoviDeCroock suggested changes Dec 14, 2021

View reviewed changes

JoviDeCroock reviewed Dec 14, 2021

View reviewed changes

AlexNi245 added 3 commits December 14, 2021 09:58

move Headline to its own component

ff091bd

fixed cross module imports

d527562

get rid of @web3-ui/hooks in /components/package.json

8161bd0

AlexNi245 added 2 commits December 14, 2021 11:19

Merge remote-tracking branch 'origin/master' into fear/token-balance

7c88ae1

added "@web3-ui/hooks" as a regular dependency

89e25f5

added changeset file

6df8770

JoviDeCroock reviewed Dec 14, 2021

View reviewed changes

packages/components/src/components/TokenBalance/TokenBalance.tsx Outdated Show resolved Hide resolved

Update packages/components/src/components/TokenBalance/TokenBalance.tsx

965f3f7

Yes, you're right I really like this pattern :) Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>

Dhaiwat10 requested changes Dec 15, 2021

View reviewed changes

AlexNi245 added 4 commits December 15, 2021 20:57

move logo and headline back to TokenBalance Component

e912a93

reverted accidentally committed files

9c521d5

pass props to flex component

12b1ea2

use flexprops instead of input props import

a3d9073

Dhaiwat10 requested changes Dec 16, 2021

View reviewed changes

AlexNi245 added 3 commits December 16, 2021 12:10

use flexprops instead of input props import

5b2dc8f

revert config.json without commit hook

4f8d41a

delete erc20.json

c36708c

fixing index.js fix headline component

Dhaiwat10 reviewed Dec 16, 2021

View reviewed changes

get rid of accidentally

34c4e46

Dhaiwat10 mentioned this pull request Dec 20, 2021

avatar-component #110

Closed

AlexNi245 added 2 commits January 11, 2022 10:33

Merge remote-tracking branch 'origin/master' into fear/token-balance

9b397f7

# Conflicts: # packages/components/package.json # packages/components/src/components/index.ts # packages/hooks/src/index.ts

move stateful logic of the TokenBalance Component to core directory.

8ef2478

Dhaiwat10 closed this Jan 11, 2022

TokenBalance Component #97

TokenBalance Component #97

Conversation

AlexNi245 commented Dec 12, 2021 • edited Loading

Description

📝 Additional Information

Screenshot

changeset-bot bot commented Dec 13, 2021 • edited Loading

🦋 Changeset detected

Dhaiwat10 commented Dec 14, 2021

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

AlexNi245 commented Dec 14, 2021

JoviDeCroock Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoviDeCroock commented Dec 14, 2021

JoviDeCroock commented Dec 14, 2021

Dhaiwat10 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

Dhaiwat10 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

Dhaiwat10 commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexNi245 commented Dec 20, 2021

Dhaiwat10 commented Jan 3, 2022

AlexNi245 commented Jan 4, 2022

Dhaiwat10 commented Jan 4, 2022

Dhaiwat10 commented Jan 11, 2022

AlexNi245 commented Dec 12, 2021 •

edited

Loading

changeset-bot bot commented Dec 13, 2021 •

edited

Loading

JoviDeCroock Dec 14, 2021 •

edited

Loading