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

(Feature) [xDai] - Fetch collectibles from Gnosis services#1460

Merged
fernandomg merged 18 commits intofeature/#1353-xDai-compatibilityfrom
feature/#1408-fetch-collectibles-from-tx-service
Oct 14, 2020
Merged

(Feature) [xDai] - Fetch collectibles from Gnosis services#1460
fernandomg merged 18 commits intofeature/#1353-xDai-compatibilityfrom
feature/#1408-fetch-collectibles-from-tx-service

Conversation

@fernandomg
Copy link
Contributor

This PR closes #1408, by:

Also, there was a tiny yet annoying issue with the infura URL: https://github.com/gnosis/safe-react/blob/feature/%231408-fetch-collectibles-from-tx-service/src/config/index.ts#L68 (RPC URL wasn't well constructed)

Finally, this is how it looks by consuming Gnosis service on Rinkeby. There are some missing information (images and names in some cases).

collectibles-from-gnosis

@github-actions
Copy link

github-actions bot commented Oct 9, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@ghost
Copy link

ghost commented Oct 9, 2020

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

@github-actions
Copy link

github-actions bot commented Oct 9, 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 2 0
Ignored 1 N/A
  • Result: ✅ success

  • Annotations: 2 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


Report generated by eslint-plus-action


// eslint-disable-next-line react/display-name
const TokenRow = memo(({ classes, data, index, style }: any) => {
const TokenRow = memo(({ data, index, style }: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Types are missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,6 @@
import NFTIcon from 'src/routes/safe/components/Balances/assets/nft_icon.png'

export const setCollectibleImageToPlaceholder = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible to type this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

this._rateLimit = RateLimit(options.rps, { timeUnit: 60 * 1000, uniformDistribution: true })
}

static extractAssets(assets: TokenResult[], nftTokens: NFTTokens): NFTAssets {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name does not represent the functionality. I was thinking is groupNFTPerAddress or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's a naming mess.

NTFAssets aims to be the NFT Family, while the NFTToken aims to be the token itself (which is, actually, the asset).

So, I'll work on that.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm desisting from doing this change here.

As this comes from the Collectibles first implementation and I'm in the middle of the refactor from nftAssets to nftCollections (and to be honest, I don't even like that name either); and this is getting bigger than I thought and really prone to error.

image

Personally, it will affect us as developers but won't interfere with the UX. Thus, I'll create a tech-debt task and work that there.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dasanra
Copy link
Collaborator

dasanra commented Oct 13, 2020

Remember to solve the merge conflicts

@ghost
Copy link

ghost commented Oct 13, 2020

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

@fernandomg
Copy link
Contributor Author

Remember to solve the merge conflicts

@dasanra, done

@fernandomg fernandomg merged commit bdcffa7 into feature/#1353-xDai-compatibility Oct 14, 2020
@fernandomg fernandomg deleted the feature/#1408-fetch-collectibles-from-tx-service branch October 14, 2020 13:43
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants