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

Move initial iterateVm checks to after step hook #279

Merged
merged 1 commit into from
Feb 25, 2018

Conversation

gnidan
Copy link
Contributor

@gnidan gnidan commented Feb 21, 2018

To ensure those conditions still generate a valid step for runStepHook handler.

The initial checks otherwise prevent certain situations from generating steps (e.g. for trace output)

To ensure those conditions still generate a valid step for `runStepHook` handler.
@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage remained the same at 85.462% when pulling c37298e on gnidan:patch-1 into 688aec6 on ethereumjs:master.

@gnidan
Copy link
Contributor Author

gnidan commented Feb 21, 2018

Thanks @axic!

@axic
Copy link
Member

axic commented Feb 22, 2018

@cdetrio does tracing outputs the last opcode even if it fails?

@gnidan
Copy link
Contributor Author

gnidan commented Feb 24, 2018

@axic trace steps are structured so that the program counter refers to the opcode about to happen. so in failing transactions, I would think you'd want to see the last opcode? cause that's the one that "broke"? (Although I'm not sure how much of what I just said is debug_traceTransaction specific, vs. ethereumjs-vm behavior)

@axic
Copy link
Member

axic commented Feb 25, 2018

It should match what debug_traceTransaction needs

@axic
Copy link
Member

axic commented Feb 25, 2018

@cdetrio @jwasinger merging this now

@axic axic merged commit e863b93 into ethereumjs:master Feb 25, 2018
@axic
Copy link
Member

axic commented Feb 25, 2018

The readme also stated:

opcode - the next opcode to be ran

So this change should be close to what the readme is saying.

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.

4 participants