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.

Sign up for GitHub

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

Jump to bottom

[VM] cleanup some changes made in the TangerineWhistle PR #814

Merged
merged 13 commits into from
Aug 6, 2020
Merged

[VM] cleanup some changes made in the TangerineWhistle PR #814

merged 13 commits into from
Aug 6, 2020

Conversation

jochem-brouwer
Copy link
Member

After reviewing #807 some code quality comments were made. This PR intends to resolve these comments.

@codecov
Copy link

codecov bot commented Jul 15, 2020 •
edited
Loading

Codecov Report

Merging #814 into master will increase coverage by 0.44%.
The diff coverage is 80.00%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 80.15% <ø> (-0.03%) ⬇️
#blockchain 84.71% <ø> (+0.20%) ⬆️
#common 93.98% <ø> (-0.16%) ⬇️
#ethash ?
#tx 94.02% <ø> (?)
#vm 93.12% <80.00%> (+0.12%) ⬆️

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

@jochem-brouwer jochem-brouwer marked this pull request as ready for review July 15, 2020 16:50
@jochem-brouwer
Copy link
Member Author

Please review this PR according to the comments made in #807 by @ryanio and @s1na 😄

@jochem-brouwer
Copy link
Member Author

Why is my second commit not showing up? It is in the branch? https://github.com/ethereumjs/ethereumjs-vm/tree/cleanup-tangerine-whistle

@jochem-brouwer jochem-brouwer force-pushed the cleanup-tangerine-whistle branch from 25e50dc to 02300d6 Compare July 15, 2020 17:08
@jochem-brouwer
Copy link
Member Author

Found an issue with the TW implementation in BlockchainTests. Will push a fix soon which I'd like to have in this PR.

@jochem-brouwer
Copy link
Member Author

Ready to review.

@jochem-brouwer jochem-brouwer mentioned this pull request Jul 30, 2020
Merged
@evertonfraga evertonfraga force-pushed the cleanup-tangerine-whistle branch from 661bb4b to 654fcbf Compare July 31, 2020 14:28
@evertonfraga evertonfraga force-pushed the cleanup-tangerine-whistle branch from 654fcbf to 681418a Compare July 31, 2020 14:29
evertonfraga
evertonfraga previously requested changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

There are some additions that are not covered by tests.

Let me know if it gets tricky to test those additions. I know there are some other uncovered places as well, we could bundle them up in a code coverage sprint.

packages/vm/lib/evm/eei.ts
* @param address - Address of account
*/
async accountExists(address: Buffer): Promise<boolean> {
return this._state.accountExists(address)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently not covered by tests:
https://codecov.io/gh/ethereumjs/ethereumjs-vm/compare/2009f18c81f2fe253e846333ace57eb77625dd60...681418a97f5e27b56da0f5ca63a6fd4acb244c2b/src/packages/vm/lib/evm/eei.ts?before=packages/vm/lib/evm/eei.ts#L591

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test for this (StateManager accountExists is actually being tested; the accountExists of EEI is actually a proxy function but we should still test that)

packages/vm/lib/state/cache.ts
if (it.node) {
return it.value.deleted
}
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently not covered by tests:
https://codecov.io/gh/ethereumjs/ethereumjs-vm/compare/2009f18c81f2fe253e846333ace57eb77625dd60...681418a97f5e27b56da0f5ca63a6fd4acb244c2b/src/packages/vm/lib/state/cache.ts?before=packages/vm/lib/state/cache.ts#L64

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test for this

@jochem-brouwer
Copy link
Member Author

@evertonfraga The tests in 49f21ff should increase test coverage to your requested missing coverage 😄

holgerd77
holgerd77 approved these changes Aug 6, 2020
View reviewed changes
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 dismissed evertonfraga’s stale review August 6, 2020 16:33

Change requests addressed.

@holgerd77 holgerd77 merged commit 214f56e into master Aug 6, 2020
@holgerd77 holgerd77 deleted the cleanup-tangerine-whistle branch August 6, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanio ryanio ryanio left review comments

@holgerd77 holgerd77 holgerd77 approved these changes

@evertonfraga evertonfraga evertonfraga left review comments

@s1na s1na Awaiting requested review from s1na

Assignees
No one assigned
Labels
package: vm PR state: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jochem-brouwer @ryanio @evertonfraga @holgerd77