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

Update available networks in network_name #2130

Closed
wants to merge 2 commits into from

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Dec 6, 2023

Description

We get the name of the current network from a list of networks based on chain id and network id.
The list is updated so that Sepolia is also included.

Changes

  • Updated command to generate the match list.
    This is done because the current command only takes the first word in the network name. So for example mainnet would be "Ethereum" instead of "Ethereum Mainnet".
    I also sorted the output, so that in the future the diff will be more meaningful.

    • Ethereum / Mainnet becomes Ethereum Mainnet (Note that it had more than one word because it was manually changed before)
    • Ethereum / Goerli becomes Goerli.
    • xDAI becomes Gnosis

    I'm not sure which effects this change is going to have, this value isn't directly needed by the services afaict. The name is used when storing instances, for example, so other parties might rely on it.

  • Added an ignored test to confirm that the test works.

How to test

New ignored test.

@fedgiac fedgiac requested a review from a team as a code owner December 6, 2023 18:41
@fedgiac fedgiac marked this pull request as draft December 6, 2023 18:47
@fedgiac fedgiac marked this pull request as ready for review December 6, 2023 18:58
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Looks good, I'm a bit nervous about the xDAI rename as we recently concluded to stick with xDAI instead of changing to Gnosis Chain everywhere, but if you say it's mainly used for instance names it's probably ok.

Otherwise, you could consider reverting the changes on existing networks) (as the main value of this PR seems to come from adding Sepolia and not changing other names).

@MartinquaXD
Copy link
Contributor

I think at least enso is using the network name in the auction metadata for some logic (#2001).
Not sure what might break on their end if we change the mainnet identifier. cc @devanoneth

@fedgiac
Copy link
Contributor Author

fedgiac commented Dec 7, 2023

At this point I'm considering getting rid of all networks that we don't use and just use this function to return the canonical, unchanging name of the network on the networks we support.

@fedgiac
Copy link
Contributor Author

fedgiac commented Dec 8, 2023

Closed in favor of #2136.

@fedgiac fedgiac closed this Dec 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants