Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Feature) - Fetch tokens refactor#1169

Closed
Agupane wants to merge 91 commits intodevelopmentfrom
fetch-tokens-refactor
Closed

(Feature) - Fetch tokens refactor#1169
Agupane wants to merge 91 commits intodevelopmentfrom
fetch-tokens-refactor

Conversation

@Agupane
Copy link
Contributor

@Agupane Agupane commented Jul 28, 2020

Closes #1071
Closes #1068
May closes #1018
Closes #877

Description

  • Now we have TokenSymbol component that let us render a symbol for the given address
  • I have also improved the getTokenInfos, before this pr this function tried to fetch the token data from the store and then go to the blockchain. Now before going to the blockchain, it will ask the API for data, so we can fetch the symbol.
  • I have also removed all the unknown within the transactions table
  • Added types

@Agupane Agupane self-assigned this Jul 28, 2020
@github-actions
Copy link

github-actions bot commented Jul 28, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 2 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr1169--safereact.review.gnosisdev.com/app

2 similar comments
@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr1169--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr1169--safereact.review.gnosisdev.com/app

@francovenica
Copy link
Contributor

the 1068 was fixed:
image.png
image.png

@francovenica
Copy link
Contributor

Tried to find a token in rinkeby that does have a icon, but is not in the manage list yet so I can check the icon is retrieved, but I wasn't able to find any. So I'll leave that part be approved by code review and check it working when the fix gets to mainnet

@ghost
Copy link

ghost commented Jul 30, 2020

Travis automatic deployment:
https://pr1169--safereact.review.gnosisdev.com/app

@mmv08
Copy link
Contributor

mmv08 commented Sep 3, 2020

@Agupane I will work on this ticket if you're ok with it :)

@Agupane
Copy link
Contributor Author

Agupane commented Sep 3, 2020

@Agupane I will work on this ticket if you're ok with it :)

ok 👍

@mmv08 mmv08 added the on hold label Sep 9, 2020
@mmv08
Copy link
Contributor

mmv08 commented Sep 9, 2020

Putting this on hold because of more prioritized tickets for the safe apps sdk

address: string
name: string
symbol: string
decimals: number | string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use this for fetching information from the backend, then be aware that this can be null. This happens if the token you try to fetch info for is a ERC721 token.


try {
isERC721 = true
await ERC721Token.at(contractAddress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks that there is code not that the code is a ERC721 token, right? Also we are using a deprecated version of truffle-contracts, would be nice to update that to the new package (may in a different PR)


const areTxsMalformed = txs.some((t) => !isTxValid(t))

if (!ethToken) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not return already in the same place where we check isOpen? Right now we are still setting up all the functions before we realize that we should not even render the modal.

Also should we use react hooks for all the functions we create here?

<div className="section">
<Heading tag="h3">Value</Heading>
<div className="value-section">
<TokenLogo height={40} name={ethToken.name} logoUri={ethToken.logoUri as string} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it necessary to cast the logoUri to a string ... according to the type dev it should be already one, right?

const submitTx = async () => {
const web3 = getWeb3()
const txRecipient = tx.contractAddress
const txRecipient = tx.contractAddress as string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the cast here required?

onClose,
}: OwnProps): React.ReactElement | null => {
const dispatch = useDispatch()
const ethToken = useToken(ETH_ADDRESS) as Token | null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels weird to me (coming from more strongly typed languages) that we cast the return type here ... we are ignoring the NFT type and I am also not sure if undefined and null are the same :/

</Row>
<Row align="center" margin="md">
<Img alt="Ether" height={28} onError={setImageToPlaceholder} src={getEthAsToken('0').logoUri} />
<TokenLogo height={28} tokenName={token?.name} tokenLogoUri={token?.logoUri} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the other place we check if token (which I would rename to ethToken or similar, to show that it is always eth) is null here we kind of assume that it is not, I would try to be consistent

@dasanra dasanra closed this Feb 2, 2021
@dasanra dasanra deleted the fetch-tokens-refactor branch February 2, 2021 11:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor fetching token information: symbol, decimals, etc When unknown don't show "UNKNOWN" Token icon not always displayed Token values format

7 participants