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

Clean up code for CREATE/CALL #179

Merged
merged 2 commits into from
Aug 17, 2017
Merged

Clean up code for CREATE/CALL #179

merged 2 commits into from
Aug 17, 2017

Conversation

axic
Copy link
Member

@axic axic commented Aug 15, 2017

No description provided.

@axic axic mentioned this pull request Aug 15, 2017
Closed
@axic
Copy link
Member Author

axic commented Aug 16, 2017

@holgerd77 @jwasinger please review this too

@axic axic requested review from wanderer and cdetrio August 16, 2017 11:06
@axic axic mentioned this pull request Aug 16, 2017
Merged
@holgerd77
Copy link
Member

What is going on here with the 1 failed test (CallToNameRegistratorTooMuchMemory2), is this intended/expected?

@axic
Copy link
Member Author

axic commented Aug 16, 2017

Need to be figured out why, see no reason for it yet.

holgerd77
holgerd77 reviewed Aug 16, 2017
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.

Had a look through the code changes, generally looks good to me but definitely second review required.

Maybe the failing test is triggered by the removed localOpts.outLength condition check? Just a guess.

jwasinger
jwasinger reviewed Aug 17, 2017
View reviewed changes
lib/opFns.js Outdated
@@ -581,7 +588,6 @@ module.exports = {
})

try {
checkCallMemCost(runState, options, localOpts)
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 where the issue is. Add this line back in to make the test pass.

Copy link
Member Author

@axic axic Aug 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It makes no sense as the localOpts are not changing at that point. Does the test expect gas deducted twice? The reason must be understood.

Update: it doesn't fix it for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Derp. It is never called in that function before. Note to self: shouldn't do these PRs late in the evening.

cdetrio
cdetrio approved these changes Aug 17, 2017
View reviewed changes
Copy link
Contributor

@cdetrio cdetrio left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@axic axic merged commit b1a3bb6 into master Aug 17, 2017
@axic axic deleted the create-cleanup branch August 17, 2017 12:42
@axic axic removed the in progress label Aug 17, 2017
holgerd77 pushed a commit that referenced this pull request Dec 1, 2020
@ryanio
Merge pull request #179 from ethereumjs/new-config-class-recreated
fb44030 
Recreated Config Class PR
holgerd77 added a commit that referenced this pull request Mar 11, 2021
@holgerd77
Merge pull request #179 from ethereumjs/const-file
1186e29 
Mv constants to own file
@Woodpile37 Woodpile37 mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwasinger jwasinger jwasinger left review comments

@holgerd77 holgerd77 holgerd77 left review comments

@cdetrio cdetrio cdetrio approved these changes

@wanderer wanderer Awaiting requested review from wanderer

Assignees
No one assigned
Labels
package: vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@axic @holgerd77 @cdetrio @jwasinger @evertonfraga