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

Implement REVERT (EIP140) #1661

Merged
merged 8 commits into from
Feb 13, 2017
Merged

Implement REVERT (EIP140) #1661

merged 8 commits into from
Feb 13, 2017

Conversation

axic
Copy link
Member

@axic axic commented Feb 6, 2017

@chriseth chriseth changed the title Implement REVERT (EIP140) BREAKING: Implement REVERT (EIP140) Feb 7, 2017
@chriseth
Copy link
Contributor

chriseth commented Feb 7, 2017

I guess it makes sense to make the reversion explicit (in contrast to throw), but we should probably also change throw to use the revert opcode.

@axic
Copy link
Member Author

axic commented Feb 7, 2017

I didn't intend to change throw, but I guess this is what users want and not a total consumption of gas.

@axic
Copy link
Member Author

axic commented Feb 8, 2017

As per the weekly meeting: throw would be soft deprecated. It would internally use the REVERT opcode, without any return value. We may in the future issue a warning advising the use of revert().

@axic axic force-pushed the asm-revert branch 2 times, most recently from 1c4d1c1 to 82814a5 Compare February 9, 2017 12:42
@axic
Copy link
Member Author

axic commented Feb 9, 2017 •
edited
Loading

This is actually not a breaking change, since REVERT will be an invalid instruction, effectively the behaviour is the same. Assuming that the opcode for REVERT is agreed on and no other instruction takes its place.

@chriseth chriseth changed the title BREAKING: Implement REVERT (EIP140) Implement REVERT (EIP140) Feb 9, 2017
@chriseth
Copy link
Contributor

chriseth commented Feb 9, 2017

Haha, you are right! Can you change the documentation about throw and about exceptions (we just modified that some days ago...)?

@axic axic force-pushed the asm-revert branch 2 times, most recently from f7588ae to 5707db7 Compare February 9, 2017 16:37
axic
axic commented Feb 9, 2017
View reviewed changes
docs/control-structures.rst Outdated
@@ -396,7 +396,7 @@ Currently, Solidity automatically generates a runtime exception in the following
#. If your contract receives Ether via a public getter function.
#. If you call a zero-initialized variable of internal function type.

Internally, Solidity performs an "invalid jump" when a user-provided exception is thrown. In contrast, it performs an invalid operation
Internally, Solidity performs a revert operation (instruction ``0xfd``) when a user-provided exception is thrown. In contrast, it performs an invalid operation
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we shouldn't mention that it won't consume all gas yet. Would need to update documentation after the HF.

@axic
Copy link
Member Author

axic commented Feb 9, 2017 •
edited
Loading

Work for future PR:

  • mention that revert won't consume all gas
  • support string parameter: revert("Something went wrong")

chriseth
chriseth suggested changes Feb 10, 2017
View reviewed changes
test/libsolidity/SolidityEndToEndTest.cpp Outdated
char const* sourceCode = R"(
contract C {
function f() {
revert();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please perform state changes here and check that they are not applied. Otherwise, the test would also succeed for the case where revert does nothing.

libsolidity/analysis/GlobalContext.cpp Outdated
@@ -65,7 +65,9 @@ m_magicVariables(vector<shared_ptr<MagicVariableDeclaration const>>{make_shared<
make_shared<MagicVariableDeclaration>("ecrecover",
make_shared<FunctionType>(strings{"bytes32", "uint8", "bytes32", "bytes32"}, strings{"address"}, FunctionType::Location::ECRecover)),
make_shared<MagicVariableDeclaration>("ripemd160",
make_shared<FunctionType>(strings(), strings{"bytes20"}, FunctionType::Location::RIPEMD160, true))})
make_shared<FunctionType>(strings(), strings{"bytes20"}, FunctionType::Location::RIPEMD160, true)),
make_shared<MagicVariableDeclaration>("revert",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should already provide the second overload now?

@axic
Copy link
Member Author

axic commented Feb 10, 2017

I'd prefer to merge #1678 first and then update this PR.

@chriseth chriseth merged commit 0d8a9c3 into develop Feb 13, 2017
@axic axic deleted the asm-revert branch February 13, 2017 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axic @chriseth