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

Add icETH and ACX token #6

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Add icETH and ACX token #6

merged 2 commits into from
Dec 7, 2022

Conversation

fleupold
Copy link
Contributor

@fleupold fleupold commented Dec 7, 2022

As discussed on slack. Not sure if we need to do something to make sure the images are long lived (e.g. add them to IPFS ourselves).

@fleupold fleupold requested a review from anxolin December 7, 2022 10:41
"address": "0x7C07F7aBe10CE8e33DC6C5aD68FE033085256A84",
"decimals": 18,
"chainId": 1,
"logoURI": "https://assets.website-files.com/60d237b6ea65be721ff4bbfa/62388a82c46d9a1dfb3dc506_icETH-token-logo.png"
Copy link

Choose a reason for hiding this comment

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

nit: All of our logos are uploaded to IPFS (which ensures availability even if they are no longer accessible at the specified URL). I think we should do the same for this here.

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 thought so too (mentioned in the PR description) however it's not true that all other token images are hosted on IPFS (a lot of them are hosted via the 1Inch list). We could also host them in this repo.

I can take the time to convert those and also write a proper readme if we agree on a process.

Copy link

Choose a reason for hiding this comment

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

Ah, sorry - I just looked at the ones immediately above the added tokens.

"address": "0x44108f0223A3C3028F5Fe7AEC7f9bb2E66beF82F",
"decimals": 18,
"chainId": 1,
"logoURI": "https://raw.githubusercontent.com/balancer-labs/assets/master/assets/0x44108f0223a3c3028f5fe7aec7f9bb2e66bef82f.png"
Copy link

Choose a reason for hiding this comment

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

Idem.

Copy link

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM.

I would update logos of the added tokens to our Pinata, or even GitHub - the important thing is we should control the file hosting. In the least so we have a backup of the asset in case it moves.

The downside of this of course is that we lose out on "automatic logo updates" that may be pushed to other sources (such as the 1Inch list).

@fleupold fleupold merged commit 83e5d3c into main Dec 7, 2022
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