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

Moved all chains to individual json #52

Merged
merged 8 commits into from May 8, 2019
Merged

Moved all chains to individual json #52

merged 8 commits into from May 8, 2019

Conversation

bmann
Copy link
Contributor

@bmann bmann commented Apr 11, 2019

I've implemented #40

This is done as individual JSON files. I've updated the various loops so this should just work now. Yay for long plane rides!

This should work cleanly, other than some additions may need to be re-added. But, we might also consider doing this as "native" Jekyll posts -- one Markdown file per chain, all the info in YAML in the header. We can open a new thread to discuss this.

@bmann bmann requested a review from ligi April 11, 2019 19:20
Copy link
Member

@ligi ligi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR - but I really think the filename should be <chainID>.json as stated here #52 and not <chainName>.json - this makes access easier and also duplicate names and variations of names

@bmann
Copy link
Contributor Author

bmann commented Apr 11, 2019

I’m confused @ligi - you wrote .json not .json ;) Will need some more clarification.

All networks have separate json files in the data/chains folder

I compile them into one chains.json

I can also just make the json files publish separately too, but am unclear on the use case.

@ligi
Copy link
Member

ligi commented Apr 11, 2019

ah - github did not display the <> without me quoting - should be clearer now ;-)

@bmann
Copy link
Contributor Author

bmann commented Apr 12, 2019

Got it. So I disagree. A folder full of numbers is less useful and harder to use. As well, it solves a problem of coming up with a “slug” or single non-conflicting short name.

Duplicate names will need to agree on a non-conflicting slug.

I just released this fixes the generated chainlink names I had to create which are super ugly. I can update this pull to fix that too.

@ligi
Copy link
Member

ligi commented Apr 12, 2019

Got it. So I disagree. A folder full of numbers is less useful and harder to use.

I disagree here. a folder of numbers is more useful and easier to use as you can directly download the json file just by knowing the chainID - with this solution you would need to download all files first. Also it enforces the unique constraint for the chainID. A lot of reasons to have chainID as filename - also doing the same for ethereum-lists/tokens where the name is the address.

@bmann
Copy link
Contributor Author

bmann commented Apr 24, 2019

Ah. To be clear - depends on the user. I can generate published file names that are whatever. So it’s more about how we want people to SUBMIT the info.

I have a new branch that I can PR where the input format is Markdown files and chainid json files are generated.

I don’t think “if you know the chainid you can just get one JSON” is particularly valid because just getting the entire chains.json is likely more useful.

But happy to do either. I’ll hook up Netlify to the branch so you can see what it generates as.

@ligi
Copy link
Member

ligi commented Apr 24, 2019

I don’t think “if you know the chainid you can just get one JSON” is particularly valid because just getting the entire chains.json is likely more useful.

I see a lot of reasons to just get one of them. E.g. an app wants to switch to a chain via WalletConenect - then the wallet needs to get information about the chain (e.g. the RPC endpoints). The wallet does not need to know about all chains - just about the requested one. Less data stored/transferred. Especially important in emerging economies without good internet and phones.

But happy to do either. I’ll hook up Netlify to the branch so you can see what it generates as.

still strongly feel <chainID>.json is the way to go

@pedrouid
Copy link
Collaborator

The primary goal of this repo IMO is to have a source of truth for chainIds for existing EVM chains. I think that are more advantages to naming the files by chainId since this whole repo revolves around the fact that EIP-155 establishes that chainId are non-conflicting constants therefore this makes it even easier to maintain this list. It removes the ability to have two chains with same chainId.

This PR was not in sync with master so I had to manually go through new submissions and fields that were changed. The naming did not help since it made harder for me to find which file was which where some chains have very similar names like EtherSocial and Ethereum Social Network. Also having the names on the files breaks links for developers. For example: Rootstock rebranded themselves to just RSK (similar to how Ripple rebranded to XRP) and that in itself would require the file name to change.

Finally because the file name is the chainId, I can always query from any app or website, the latest file by just using the .../<chain_id>.json. For example:

https://raw.githubusercontent.com/ethereum-lists/chains/single-json-per-chainid/_data/chains/100.json

I've branched out and created a PR to make this change but if this looks ok to you @bmann, I think we are going to go with this change unless there is technical difficulty on the jekyll said merging the files for the website.

@pedrouid
Copy link
Collaborator

New PR: #56

@bmann
Copy link
Contributor Author

bmann commented Apr 24, 2019

Yes. Please look at the other branch that I have not yet PR’d. It does this already. In Jekyll. Which can then generate any combination of JSON file namings.

@pedrouid
Copy link
Collaborator

Is it the markdown-chains branch? Can you point to the logic where it generates any combination of JSON file namings?

@pedrouid pedrouid merged commit 7cf958e into master May 8, 2019
@pedrouid pedrouid deleted the single-json branch May 8, 2019 17:10
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.

None yet

3 participants