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: removed retired dev networks #1296

Merged
merged 4 commits into from
Jun 15, 2021

Conversation

emersonmacro
Copy link
Contributor

@emersonmacro emersonmacro commented Jun 12, 2021

Addresses #1294

@codecov
Copy link

codecov bot commented Jun 12, 2021

Codecov Report

Merging #1296 (227a374) into master (87a98bf) will decrease coverage by 1.90%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 86.20% <ø> (ø)
blockchain 84.42% <ø> (ø)
client 84.59% <ø> (-0.04%) ⬇️
common 88.70% <ø> (+0.45%) ⬆️
devp2p 84.00% <ø> (+0.38%) ⬆️
ethash 82.83% <ø> (ø)
tx 89.49% <ø> (-0.12%) ⬇️
vm 81.28% <ø> (-5.79%) ⬇️

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

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.

Hi there,
thanks for the PR! 😄

For the failing tx tests this is a bit laborious, but an upgrade is needed there. I also once stumbled upon this. This just needs a Common instance with the london HF activated, so this is not directly related to aleut. The thing is: once you change the network there some comparison values in the tests will also change, e.g. the hash in the hash() test case, since the hash function also takes the chain ID from the tx into account and the chain ID changes when the network changes.

So on these test cases you need to update - e.g. on the hash() test - the expected hash to the new hash produced by signed.hash() (for now you can assume that this new hash is correct).

Also note: please use a stable/lasting network (like mainnet or rinkeby) for replacement, if you use calaveras (the new dev network) e.g., the test will need some adoption again further down the line once this network is again removed. 😋

Ok, hope this helps, let me know if you have questions.

packages/common/src/genesisStates/index.ts Show resolved Hide resolved
@emersonmacro
Copy link
Contributor Author

@holgerd77 Thank you - that info was very helpful. I converted the failing tx test to use rinkeby and tests are passing now

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.

Cool, thanks for working this in so quickly, looks really good now, will merge.

@holgerd77 holgerd77 merged commit 026a54f into ethereumjs:master Jun 15, 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.

2 participants