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

Common: Remove calaveras ephemeral testnet #1430

Merged
merged 10 commits into from
Sep 1, 2021

Conversation

Zachinquarantine
Copy link
Contributor

The Calaveras testnet was only used for testing the London upgrade AFAIK, and since the London upgrade has come and gone, this PR removes support for Calaveras.

@ryanio
Copy link
Contributor

ryanio commented Aug 26, 2021

hey @Zachinquarantine, thanks for opening! This is a good idea for some maintenance cleanup :)

Doing a search in the repo for calaveras, I found a few more places we can remove:

packages/client/README.md: L58
packages/common/README.md: L118
packages/common/src/index.ts: remove from enum Chain in L45 and case in L909

@Zachinquarantine
Copy link
Contributor Author

Doing a search in the repo for calaveras, I found a few more places we can remove:

packages/client/README.md: L58
packages/common/README.md: L118
packages/common/src/index.ts: remove from enum Chain in L45 and case in L909

Done!

@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #1430 (ad779b6) into master (327d2d4) will decrease coverage by 9.53%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 86.76% <ø> (?)
blockchain 83.56% <ø> (?)
client 82.31% <ø> (?)
common 94.20% <ø> (-0.04%) ⬇️
devp2p 82.48% <ø> (?)
ethash 86.56% <ø> (ø)
tx 88.24% <ø> (?)
vm 79.33% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

@Zachinquarantine hi there, thanks for the PR! 😄

There are still some tests in Common using the Calaveras network (in chain.spec.ts), these also needs to be updated since we need the tests to pass to be able to merge here.

@ryanio Just as some general reflection point: there is a slight risk on doing these kind of clean-ups that people have actually used or have referenced these testnets in their code (for whatever reason) and we will break something for them. Just as some general thought point, I know we have removed these networks (like yolov3 e.g.) before ourselves and we can therefore also do here for Calaveras. Maybe we want to at least do some precautions here on the next ephemeral testnet addition and e.g. make clearer doc comments to advice users to not integrate these kind of networks in their production code.

Another possibility would be that we completely omit adding in the first place and solely use the new custom chain parameters instead. Not sure if this is so convenient though for us, I actually do enjoy just using --network=calaveras in the client and do not have to bother around custom chain file paths and the like.

@holgerd77
Copy link
Member

@Zachinquarantine not sure if you stumbled upon this already, but also make sure to always run npm run lint:fix before pushing to have your code formatted properly (this otherwise also breaks CI runs). 😄

@Zachinquarantine
Copy link
Contributor Author

but also make sure to always run npm run lint:fix before pushing to have your code formatted properly

I get:
npm ERR! Missing script: "lint:fix"
npm ERR!
npm ERR! To see a list of scripts, run:
npm ERR! npm run
What should I do?

@ryanio
Copy link
Contributor

ryanio commented Aug 28, 2021

but also make sure to always run npm run lint:fix before pushing to have your code formatted properly

I get:

npm ERR! Missing script: "lint:fix"

npm ERR!

npm ERR! To see a list of scripts, run:

npm ERR! npm run

What should I do?

If on root run with --workspaces to run the cmd in all packages.

So: npm run lint:fix --workspaces

@Zachinquarantine
Copy link
Contributor Author

but also make sure to always run npm run lint:fix before pushing to have your code formatted properly

I get:
npm ERR! Missing script: "lint:fix"
npm ERR!
npm ERR! To see a list of scripts, run:
npm ERR! npm run
What should I do?

If on root run with --workspaces to run the cmd in all packages.

So: npm run lint:fix --workspaces

I don't think I have lint installed at all, how can I install it?

@holgerd77
Copy link
Member

@Zachinquarantine you need to one-time run npm i from the root of your monorepo to install all dependencies and dev dependencies and build all the packages (takes a couple of minutes).

@acolytec3
Copy link
Contributor

This looks good! Looks like everything is cleaned up and tests are all passing. @holgerd77 I think this is technically a breaking change on common so will defer to your judgement on when/how to merge.

@holgerd77
Copy link
Member

@acolytec3 after sleeping on this one night I am actually even more confident that we just can merge this into master and actually also continue with our current practice on adding (and removing) these testnets to Common.

People very well know that these testnets are just temporary and I guess the chance that someone is hardcoding a reference to these testnets into a production library is extremely rare.

So @Zachinquarantine thanks again for the contribution, will merge.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 changed the title Remove calaveras ephemerial testnet Common: Remove calaveras ephemerial testnet Sep 1, 2021
@holgerd77 holgerd77 merged commit 6d80656 into ethereumjs:master Sep 1, 2021
@Zachinquarantine Zachinquarantine deleted the remove-calaveras branch September 1, 2021 16:14
@holgerd77 holgerd77 changed the title Common: Remove calaveras ephemerial testnet Common: Remove calaveras ephemeral testnet Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants