-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
wallets.csv -> wallets.json #2730
Comments
Totally... WalletConnect's website is actually close to being updated to use the registry which you can use it as well here: https://github.com/walletconnect/walletconnect-registry |
Oh nice - that json is actually already quite close to what I had in mind when opening this issue. |
However this registry only includes Dapps and Wallets that support WalletConnect so I'm not sure if you would also need to aggregate some other data of other Ethereum apps |
Ya - I don't think the data should be taken directly. But if the same format is used it is easier to share/merge/.. |
Sounds good. As you can see the data is already being served statically from this url: https://registry.walletconnect.org/data/wallets.json Logos can also be fetched by the id in three different sizes ("sm" | "md" | "lg") |
Thanks for raising this @ligi (& @pedrouid for weighing in). @ligi do you mind expanding on your reasoning here for my understanding? Why is JSON easier to update/maintain vs. CSV? I'm certainly open to switching over, just want to understand why one is better. Seems like a minor implementation detail to me - Gatsby ingests them the same & queries those data file via GraphQL. |
I see the following advantages:
and with adding more properties it will get even worse. |
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
hm - no way for the author to reopen the issue? Do I need to create a new one? |
Converted the current CSV to JSON, didn't have time to get my head around the GraphQL to take this further. |
Nice! Not sure why you need GraphQL in that context? |
The implementation makes use of GraphQL and I was struggling to get my head around all the parts, will have another look when i'm more fresh. |
So we're definitely not opposed to doing this. Definitely agree with @ligi that the change would make the experience for a contributor adding a new wallet far better. The refactor required to get this working is not insignificant and it would probably require the team to change how the wallets are tracked internally so for those reasons it isn't a huge priority for us at the moment. If anyone is happy to pick this up we'd be thrilled to take a PR for it. |
Is this still up for grabs? Wanted to know how to proceed on this one then. |
This issue is stale because it has been open 45 days with no activity. |
Is this complete @corwintines? I believe we solved as part of #6274 but LMK if I'm wrong. |
Perhaps not - we now have this TS file with wallet data: But I still see the CSV in there - can this be removed? |
I'll check this out this week and check if we are good to get rid of the csv. |
We converted to ts instead of json. I think that is ok to call this issue complete as this came from some codebase upgrades. |
I still think this is a neat idea @ligi so feel free to re-open or create a new issue if you'd like to push that discussion forward! |
I think important wallet properties can change over time (also see #2715) - having a csv storing the data makes things harder to update and maintenance quite difficult.
So I suggest changing from csv to json for more flexilibilty.
If size is a concern here the json could be postprocessed to become a csv again - but this way only algorithms need to touch it - not humans ;)
Also wondering if there could be data-sharing between https://walletconnect.org/wallets and this site. cc @pedrouid
The text was updated successfully, but these errors were encountered: