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

wip: Vm opcodes REVERT (2nd try) #17

Open
wants to merge 4 commits into
base: vm-opcodes-returns
Choose a base branch
from

Conversation

whilei
Copy link
Owner

@whilei whilei commented Aug 22, 2018

#16 was failing tests, I think because I refactored the EVM.Run iterator wrongly.


This change is Reviewable

@whilei whilei changed the title wip: Vm opcodes revert2 (2nd try) wip: Vm opcodes REVERT (2nd try) Aug 22, 2018
Copy link
Collaborator

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @whilei)


core/execution.go, line 221 at r1 (raw file):

	ret, err = evm.Run(contract, input)
	if err != nil {
		if err != nil {

Wait, what?! ;)

I think you meant if err != vm.ErrExecutionReverted.


core/vm/jump_table.go, line 27 at r1 (raw file):

	// returns determines whether the operation sets the return data
	returns bool
	reverts bool // determines whether the operation reverts state (implicitly halts)

Once again, I don't like this approach - we can have just a simple predicate function.

Copy link
Owner Author

@whilei whilei left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @whilei and @tzdybal)


core/execution.go, line 221 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Wait, what?! ;)

I think you meant if err != vm.ErrExecutionReverted.

Done.


core/vm/jump_table.go, line 27 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Once again, I don't like this approach - we can have just a simple predicate function.

Doing.

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.

None yet

2 participants