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

Monorepo: make _* methods protected / _common -> common #2857

Merged
merged 17 commits into from
Jul 4, 2023

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Jul 4, 2023

Addresses #2852

This is a first test on the above issue to get a feeling on the extent of the task, will start with the Common library. 

This also refactors the EventEmitter structure from Common aligning this with EVM and client by separating the EventEmitter from the main class and make this an own property to allow for a cleaner API (code completion).

Update: this is now taking on all the libraries. This is on purpose a "rough" update, primarily targeting the main classes where the API is most visible. Everything else would just take too much time and effort at this point.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #2857 (de7a5e4) into master (d7131f5) will decrease coverage by 0.01%.
The diff coverage is 80.05%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.16% <87.50%> (ø)
blockchain 92.56% <82.35%> (ø)
client 87.08% <89.65%> (ø)
common 97.07% <100.00%> (+<0.01%) ⬆️
devp2p 86.95% <ø> (ø)
ethash ∅ <ø> (∅)
evm 67.01% <64.76%> (-0.02%) ⬇️
rlp ∅ <ø> (?)
statemanager 86.27% <78.00%> (-0.18%) ⬇️
trie 90.04% <100.00%> (ø)
tx 95.96% <100.00%> (ø)
util 86.57% <ø> (ø)
vm 77.28% <82.69%> (ø)

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

@holgerd77 holgerd77 changed the title Monorepo/Common: First-round PR to make _* methods protected Monorepo: make _* methods protected / _common -> common Jul 4, 2023
@holgerd77 holgerd77 force-pushed the make-non-public-members-protected branch from 852c5ce to de7a5e4 Compare July 4, 2023 13:45
@holgerd77
Copy link
Member Author

Rebased this via UI

@@ -97,7 +97,7 @@ describe('Istanbul: EIP-152', () => {
const res = precompile09({
data: hexToBytes('0x' + testCase.input),
gasLimit: BigInt(20),
_common: common,
common: common,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
common: common,
common,

Super nit but we can now do this

Copy link
Member Author

Choose a reason for hiding this comment

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

Will ignore this for now, we should really activate linter for test files though, this would then be caught and fixed.

@@ -108,7 +108,7 @@ describe('Istanbul: EIP-152', () => {
const res = precompile09({
data: hexToBytes('0x' + testCase.input),
gasLimit: BigInt(10000000),
_common: common,
common: common,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
common: common,
common,

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Two super-nit comments, but will approve anyways since it is just a style-ish comment. LGTM!

@holgerd77 holgerd77 merged commit 5f8552f into master Jul 4, 2023
@holgerd77 holgerd77 deleted the make-non-public-members-protected branch July 4, 2023 15:17
@holgerd77 holgerd77 mentioned this pull request Jul 13, 2023
2 tasks
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