Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add script for bulk adding tokens #324

Merged
merged 16 commits into from
Jan 10, 2024
Merged

Conversation

kathaypacific
Copy link
Contributor

@kathaypacific kathaypacific commented Jan 4, 2024

This PR adds a script to add new tokens using Coingecko as a data source. The script:

  • gets a list of X tokens by market cap for a given category (i.e. chain)
  • gets the necessary token metadata
  • gets and resizes the token image
  • saves the updated tokens info json file
  • logs warnings if the images are upscaled, stretched, or too big.

I added a new dev dependency to do the image resizing. The resizing gets us the right dimensions and png format, but of course it doesn't get us the transparent background etc. I think that will need to be manual.

Related to RET-974

@kathaypacific kathaypacific changed the title chore: first pass feat: add script for bulk adding tokens Jan 4, 2024
Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

Should we worry about the liquidity of added tokens?
I guess we're fetching from the top page, so probably not.

}

function parseArgs() {
return yargs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return yargs
return yargs
.usage('Usage: $0\n\nAdd new tokens using CoinGecko as a data source.'

Comment on lines 131 to 136
console.log('Resizing image...')
const imageFile = await Jimp.read(response.data)
const resizedImage = await imageFile
.resize(256, 256)
.quality(100)
.getBufferAsync(Jimp.MIME_PNG)
Copy link
Member

Choose a reason for hiding this comment

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

Could we show a warning message if upscaling?
Just so we don't blindly add low quality images.
Though the reviewer of the changed PR should hopefully catch that too.

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 good idea, i've added some log warnings for image stretching / upscaling

Comment on lines 109 to 120
const [symbol, name] = await Promise.all([
client.readContract({
address,
abi: erc20Abi,
functionName: 'symbol',
}),
client.readContract({
address,
abi: erc20Abi,
functionName: 'name',
}),
])
Copy link
Member

Choose a reason for hiding this comment

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

nit: doesn't really matter here, but just for the sake of getting in the habit of minimizing RPC calls

Suggested change
const [symbol, name] = await Promise.all([
client.readContract({
address,
abi: erc20Abi,
functionName: 'symbol',
}),
client.readContract({
address,
abi: erc20Abi,
functionName: 'name',
}),
])
const [symbol, name] = await client.multicall({
contracts: [
{
address,
abi: erc20Abi,
functionName: 'symbol',
},
{
address,
abi: erc20Abi,
functionName: 'name',
}
],
allowFailure: false,
})

.getBufferAsync(Jimp.MIME_PNG)

console.log('Saving image...')
const filePath = `./assets/tokens/${symbol}.png`
Copy link
Member

Choose a reason for hiding this comment

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

This could possibly overwrite existing images.
Should we log a warning if that's the case?
Though it will be clear in the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added a log warning about this, and i think it's acceptable to pick this up from the warning / git diff. doesn't seem like something that should prevent the token from being added

})

console.log('Resizing image...')
const imageFile = await Jimp.read(response.data)
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment about the limitation around transparent backgrounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't end up doing this as it's not a limitation of the script but rather the original image. there are some images with transparent backgrounds on coingecko that will work as expected, there are also images that exist already that don't have transparent backgrounds. we could take a pass through all the image assets at some point to remove backgrounds, but i also think the UI handles these relatively gracefully at the moment anyway

console.log('Resizing image...')
const imageFile = await Jimp.read(response.data)
const resizedImage = await imageFile
.resize(256, 256)
Copy link
Member

Choose a reason for hiding this comment

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

Does CoinGecko only return square images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added a warning if they are too much of a non-square, only 1 image was like that. it seems like coingecko has made the images square themselves (they're not pixel perfect square, but square enough i think)

Copy link
Member

Choose a reason for hiding this comment

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

Ok too bad jimp doesn't support a resize option to fit a square then.
That one image will become slightly distorted then, right?

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 the jimp resize will distort the image. the script logs a warning, and we can always manually download / update the image if we need to. for this particular case, the automatically resized image looks good enough to me (https://assets.coingecko.com/coins/images/913/large/LRC.png?1696502034)

const imageFile = await Jimp.read(response.data)
const resizedImage = await imageFile
.resize(256, 256)
.quality(100)
Copy link
Member

Choose a reason for hiding this comment

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

Nothing to do here, but this reminds me it would be nice to have an automated check that ensures the images we have in the repo are optimized (with max compression, for instance with ImageOptim or similar).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can take a pass through all the images with this in a follow up

fs.writeFileSync(filePath, resizedImage, 'binary')
} catch (error) {
console.warn(
`⚠️ Encountered error fetching image, skipping ${id}. ${error}`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`⚠️ Encountered error fetching image, skipping ${id}. ${error}`,
`⚠️ Encountered error fetching/resizing/writing image, skipping ${id}. ${error}`,

}
fs.writeFileSync(filePath, resizedImage, 'binary')

const maxFileSizeInBytes = 60 * 1024 // 50 KB
Copy link
Member

Choose a reason for hiding this comment

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

Should we pick 50 KB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, we can indeed pick 50!

@kathaypacific kathaypacific enabled auto-merge (squash) January 10, 2024 12:41
@kathaypacific kathaypacific merged commit 86fcabe into main Jan 10, 2024
5 checks passed
@kathaypacific kathaypacific deleted the kathy/add-tokens-script branch January 10, 2024 12:42
kathaypacific added a commit that referenced this pull request Jan 10, 2024
Fixes RET-974

Of the top 100 tokens by marketcap on coingecko for ethereum, the
following tokens could not be added automatically: fantom, astar,
xdce-crowd-sale, zilliqa. These values are the coingecko token id's, but
no token data (i.e. address) could be fetched from their endpoint.

The script in #324 was used to generate these changes - the script has
given the tokens the following default properties:

```
      name,
      symbol,
      decimals,
      address,
      imageUrl: `https://raw.githubusercontent.com/valora-inc/address-metadata/main/assets/tokens/${symbol}.png`,
      isNative: false,
      infoUrl: `https://www.coingecko.com/en/coins/${id}`,
```

These tokens are not eligible for swaps right now, it seems like we
should check the liquidity separately before enabling the ability to
swap into these tokens. Unfortunately the liquidity information is not
directly available in the requests made in #324.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants