Enhancement: Add even more networks to source-fetcher #5076
Conversation
haltman-at
requested review from
gnidan,
benjamincburns,
cds-amal,
fainashalts,
lsqproduction,
dongmingh and
leeftk
There was a problem hiding this comment.
Thanks @haltman-at. The addition of new networks makes me worry about the growing complexity in determineUrl and I have to ask for changes, or a reason to not worry.
| if (exceptionalTestnets.has(this.networkName)) { | ||
| //special case: certain exceptional testnets just use "testnet" | ||
| //rather than the specific testnet name |
There was a problem hiding this comment.
Is it possible to consolidate all this metadata in one data structure? There's a lot of logic to perform what seems to be a simple table lookup to resolve a scanner endpoint. It would simplify adding/removing networks if only a table has to be updated.
There was a problem hiding this comment.
Eh, OK, I'll go change it to use a lookup table.
There was a problem hiding this comment.
(The pro of the existing approach is that in many cases you don't even have to update a whole table, you can often update less than that. :) Also, you're less likely to enter something incorrectly when updating it because the things you're entering are smaller and more atomic. But yes it does raise the specter that special cases could balloon in the future...)
There was a problem hiding this comment.
OK, I've gone ahead and made this change.
So, it turns out that Sourcify and Etherscan (or rather, "Blockscan", to use the new more general name -- no, I didn't go changig our name for it) have added support for a bunch more networks; this PR updates source-fetcher to support them as well.
So, what's it do in more detail?
First off, it adds all these new networks to the big list of networks. You'll notice there's another thing it adds to the big list of networks -- Avalanche. I'd previously excluded Avalanche due to network ID / chain ID mismatches that would cause a problem for us; but as best I can tell, they've now changed their network IDs to match their chain IDs, so there's no reason to exclude them anymore.
Also, for consistency, I added POA Core to the big list, even though neither fetcher supports it. This is for the same reason I earlier added OneLedger Mainnet to the big list -- it seems off to include a testnet but not include the mainnet. I hadn't previously included POA Core for the simple reason that I didn't realize that Sokol was a testnet for it. Now that I know, I'm including it. As such I've renamed
"sokol"to"sokol-poa"for consistency with our usual convention elsewhere. I've therefore also updated the test that references it.Secondly, it adds support to the Sourcify fetcher for the networks that have been added to Sourcify (and Avalanche, since we're no longer excluding that). That part is straightforward.
Finally, it adds support to the Etherscan fetcher for the networks that have been added there (again, plus Avalanche). Since Etherscan isn't nice and uniform like Sourcify is, this takes a bit more work; it requires also adding the various URLs for its other websites. Also, the special case that was in place for Arbitrum Rinkeby has been expanded to also encompass Avalanche Fuji and BTTC Donau, with a variable holding the list of these special cases so that they don't all need to be written out individually in the code. (Yes, I've left the
determineUrlfunction in place rather than replace it by a lookup table.)I didn't add any new tests; I think the existing tests cover things well enough, we don't need tests for every specific network we include.
And that's it!