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

develop: small fixes #1924

Merged
merged 8 commits into from
Jun 3, 2022
Merged

develop: small fixes #1924

merged 8 commits into from
Jun 3, 2022

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented May 31, 2022

This PR:

  • Simplifies setting blockchain genesis parameters from common for future extensibility of additional fields
  • Moves vm's EOF file one folder up (couldn't find a reason it needed to be in vm/opcodes), export default rather than wildcard import
  • Removes last assert usages from evm/precompiles, devp2p
    • (no assert usage left across the monorepo, woo!)

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #1924 (fba80d4) into develop (e0a659d) will decrease coverage by 0.20%.
The diff coverage is 54.05%.

Impacted file tree graph

Flag Coverage Δ
block 86.15% <ø> (ø)
blockchain 82.42% <66.66%> (+1.12%) ⬆️
client 77.64% <100.00%> (-0.09%) ⬇️
common 95.01% <ø> (ø)
devp2p 82.50% <65.21%> (-0.34%) ⬇️
ethash 90.81% <ø> (ø)
statemanager 84.87% <ø> (ø)
trie 80.19% <ø> (-0.50%) ⬇️
tx 92.13% <ø> (ø)
util 87.29% <ø> (ø)
vm 80.38% <37.50%> (-0.72%) ⬇️

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

@ryanio ryanio marked this pull request as ready for review May 31, 2022 20:42
Comment on lines -218 to -222
// Throw on chain or hardfork options removed in latest major release to
// prevent implicit chain setup on a wrong chain
if ('chain' in opts || 'hardfork' in opts) {
throw new Error('Chain/hardfork options are not allowed any more on initialization')
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this since it would be two major releases ago for develop, which I think is far enough to remove the legacy check, and TypeScript should guard downstream users against these options being used.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@ryanio ryanio requested a review from holgerd77 June 1, 2022 22:38
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.

Yes, all fine! 🙂 👍

@@ -22,7 +22,7 @@ import EEI from './eei'
import { ERROR, VmError } from '../exceptions'
import { default as Interpreter, InterpreterOpts, RunState } from './interpreter'
import Message, { MessageWithTo } from './message'
import * as eof from './opcodes/eof'
import EOF from './eof'
Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, that's better. I was actually me who introduced the structure if I remember correctly.

@@ -73,7 +70,7 @@ export class ENR {
obj.secp256k1
)

assert(isVerified, 'Unable to verify ENR signature')
if (!isVerified) throw new Error('Unable to verify ENR signature')
Copy link
Member

Choose a reason for hiding this comment

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

Ah, also nice here.

@@ -371,7 +371,7 @@ async function startClient(config: Config, customGenesisState?: GenesisState) {
validateBlocks: true,
validateConsensus,
})
setCommonForkHashes(config.chainCommon, blockchain.genesisBlock().hash())
setCommonForkHashes(config.chainCommon, blockchain.genesisBlock.hash())
Copy link
Member

Choose a reason for hiding this comment

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

Yes, also nicer. 🙂

Comment on lines -218 to -222
// Throw on chain or hardfork options removed in latest major release to
// prevent implicit chain setup on a wrong chain
if ('chain' in opts || 'hardfork' in opts) {
throw new Error('Chain/hardfork options are not allowed any more on initialization')
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@holgerd77 holgerd77 merged commit d62b7f4 into develop Jun 3, 2022
@holgerd77 holgerd77 deleted the develop-small-fixes branch June 3, 2022 09:07
g11tech pushed a commit that referenced this pull request Jun 3, 2022
* simplify setting blockchan genesis parameters for future extensibility, make timestamp optional instead of `| null`

* move EOF one folder up, export default rather than wildcard import

* remove `assert` usage from vm/precompiles, devp2p

* eof: reinstate individual imports

* make genesisBlock a property and add a separate createGenesisBlock method

* fix mock, remove commented out code (double checked, geth does not return the eth protocol versions in admin_nodeInfo, so this is safely removable)

* remove old chain/hardfork check

* simplify errors, use increment operator, use for..of rather than forEach
holgerd77 pushed a commit that referenced this pull request Jun 8, 2022
* simplify setting blockchan genesis parameters for future extensibility, make timestamp optional instead of `| null`

* move EOF one folder up, export default rather than wildcard import

* remove `assert` usage from vm/precompiles, devp2p

* eof: reinstate individual imports

* make genesisBlock a property and add a separate createGenesisBlock method

* fix mock, remove commented out code (double checked, geth does not return the eth protocol versions in admin_nodeInfo, so this is safely removable)

* remove old chain/hardfork check

* simplify errors, use increment operator, use for..of rather than forEach
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