BREAKING: Add payable modifier #665

axic · 2016-06-18T17:52:09Z

Partially implements #500 and #563. Comments welcome.

Fixes #602.

todo:

library function should not be payable
~~contract is payable should create payable fallback function (and error if there already is one)~~
calling a non-payable function with value set should report an error
accessor functions should not be able to receive ether (-> callvalue check should probably move to dispatcher)
internal functions should not allow payable modifier
overwriting a base contract function should not change payable
force constant and payable to be mutually exclusive

chriseth · 2016-06-21T15:15:46Z

This severely breaks backwards compatibility and has to be discussed thoroughly.

I think it would be better to not have this as a bulit-in compiler feature. If we want to use something like that for analysis and error reporting purposes, the modifier could be added to a standard library. Having said that, I'm not so keen on keeping the hardcoded standard libraries.

redsquirrel · 2016-06-21T15:18:22Z

@chriseth Any specific plans for the standard libraries?

axic · 2016-06-24T21:47:09Z

@chriseth I think the reason it should be built in is to support it in the interface JSON. Unless you want to introduce a feature whereby flags can be added to the interface JSON via modifiers or other language features.

axic · 2016-06-24T21:55:59Z

Alternative approach to not break backwards compatibility to have a rejectValue flag in the interface JSON. The language could have either ways, but in that case probably it is preferable to mirror the interface JSON, i.e. have a rejectValue modifier.

I do think that the idea of making backwards incompatible changes at this stage, in a coordinated effort, should not be withdrawn.

chriseth · 2016-07-08T18:24:55Z

Ok, I agree that it might be handy to know whether a function accepts ether especially with auto-generated contract interfaces in mind.

I don't especially like the name of the modifier - value is a bit too generic, although I know that we have msg.value. Also I would prefer the active form acceptsValue.

For the sake of usability, a default of rejecting ether transfer should of course be preferred. It gets more complicated at the point where we have public functions which are helper functions and are called from functions that accept value transfer - then these would throw.

axic · 2016-07-12T23:29:26Z

@chriseth the code currently has valueAccepted as a keyword and acceptvalue in the ABI JSON.

Would you rather go for acceptsValue and acceptsvalue?

Also this PR doesn't implement the actual code to reject such transactions, so I do not see any backward compatibility break. Rejection should be implemented of course and I would prefer to reject if it value as given to a non-acceptsValue method.

chriseth · 2016-07-18T17:12:22Z

I think I would prefer acceptsEther but I'm open for suggestions; and please implement the full thing in one PR.

axic · 2016-08-01T17:10:24Z

@chriseth I would probably go with acceptsValue or acceptsEther.

Regarding the latter I'm not sure how the system will look like once Ether becomes a token contract:

Will value passing be removed from normal transactions and it has to be assigned as usual via the token interface?
Or will the transaction have the token type (i.e. token contract address) passed along the value?

In case 2), acceptsEther is misleading.

chriseth · 2016-08-01T21:34:45Z

I don't think we will go with 2. If we do, we can still add a new modifier acceptsToken(Token _token)

axic · 2016-08-02T19:01:49Z

Agreed with @chriseth that we're going for the keyword payable.

chriseth · 2016-08-05T15:55:13Z

Please check (as in write a test) that this also applies to accessor functions!

axic · 2016-08-05T19:01:28Z

libsolidity/codegen/ContractCompiler.cpp

@@ -426,6 +410,13 @@ bool ContractCompiler::visit(FunctionDefinition const& _function)

 	m_context.startFunction(_function);

+	// Reject transaction if value is not accepted, but was received
+	if (!_function.isPayable())


Question is, should it be in visit(FunctionDefiniton) or appendFunctionSelector?

visit(FunctionDefinition) is also used for internal functions, so it should be in the selector or the calldata unpacker.

axic · 2016-08-05T19:12:50Z

@chriseth This works now, but a lot tests need to be fixed to include the keyword.

axic · 2016-08-08T09:29:33Z

It may be a good time to review some of those EndToEnd tests whether they actually require a value to be sent at them. Some looked like they may only have it as an oversight.

chriseth · 2016-08-08T13:41:22Z

test/libsolidity/SolidityEndToEndTest.cpp

@@ -2045,7 +2045,7 @@ BOOST_AUTO_TEST_CASE(contracts_as_addresses)
 		}
 		contract test {
 			helper h;
-			function test() { h = new helper(); h.send(5); }
+			function test() payable { h = new helper(); h.send(5); }


Do you actually handle the constructor?

Yes. I've only added the keyword to the tests where it was failing without the keyword and I've reviewed that the code actually needs it.

chriseth · 2016-08-08T13:44:39Z

I think you added payable to a lot of end to end tests where this is not needed.

chriseth · 2016-08-25T16:28:48Z

We could add another feature: contract a is payable { ... } results in contract a receiving an empty fallback function and errors if it already has a fallback function.

ethernomad · 2016-08-26T02:20:44Z

Will this mean that getters generated for public variables will throw if they are called with a value transfer?

chriseth · 2016-08-26T13:42:14Z

@ethernomad yes

axic · 2016-08-31T00:28:28Z

@chriseth I think constant and payable modifiers should be mutually exclusive.

constant means that the state cannot be modified, while receiving value is modifying the balance of the state. (Albeit it is not done by the code running on EVM.)

chriseth · 2016-09-01T08:49:12Z

@axic ok, that makes sense.

chriseth · 2016-09-05T11:37:36Z

Removed the contract A is payable feature again as it is probably of limited use. We can still add it without breaking backwards-compatibility.

pirapira · 2016-09-05T16:35:34Z

test/libsolidity/SolidityEndToEndTest.cpp

+	char const* sourceCode = R"(
+		contract C {
+			modifier tryCircumvent {
+				if (false) _ // avoid the function, we should still not accept ether


Now a semicolon after _ is necessary after #1005.

…difier.

axic · 2016-09-06T00:15:08Z

libsolidity/analysis/TypeChecker.cpp

+		if (!_function.isConstructor() && !_function.name().empty() && !_function.isPartOfExternalInterface())
+			typeError(_function.location(), "Internal functions cannot be payable.");
+	}
+	if (_function.isPayable() && _function.isDeclaredConst())


This can be merged into the above if.

axic · 2016-09-06T00:20:43Z

Two tests needed:

ABI test for payable fallback
payable private method should be disallow (similar to how payable internal is)

chriseth · 2016-09-06T08:59:42Z

@axic please review again

axic · 2016-09-06T10:10:50Z

@chriseth looks good

chriseth added the in progress label Jun 18, 2016

axic changed the title ~~Add valueAccepted modifier~~ Add valueAccepted modifier [WIP] Jun 20, 2016

axic changed the title ~~Add valueAccepted modifier [WIP]~~ Add payable modifier [WIP] Aug 2, 2016

axic reviewed Aug 5, 2016
View reviewed changes

chriseth added this to the 2-breaking-safety-changes milestone Aug 8, 2016

chriseth reviewed Aug 8, 2016
View reviewed changes

axic mentioned this pull request Aug 8, 2016

Reserved keywords update #833

Merged

chriseth changed the title ~~Add payable modifier [WIP]~~ BREAKING: Add payable modifier Aug 11, 2016

chriseth added the nextrelease label Aug 12, 2016

chriseth added needs review labels Sep 5, 2016

chriseth mentioned this pull request Sep 5, 2016

Reject constant modifier on the fallback function #963

Closed

pirapira reviewed Sep 5, 2016
View reviewed changes

axic and others added 9 commits September 5, 2016 21:28

Support payable keyword for functions

a34f2f1

Include ABI JSON test for payable keyword

a7794b1

Do not include the payable keyword for constructors

75d556a

Include EndToEnd test for payable keyword

34a6afb

Mark every other test payable where neccesary in EndToEndTest

680b83b

Merged in changes from chriseth/payable

962531a

Change function type to include and propagate payable and constant mo…

9c64edf

…difier.

Make constant and payable mutually exclusive.

1eb7ddb

Change placeholder style.

ff11aa1

axic reviewed Sep 6, 2016
View reviewed changes

chriseth added 2 commits September 6, 2016 10:58

Tests for payable / private combination.

384f189

Test and fixes for payable fallback in ABI.

dff9633

chriseth merged commit f687635 into ethereum:develop Sep 6, 2016

chriseth removed in progress labels Sep 6, 2016

This was referenced Sep 6, 2016

Payable as a parameter in the ABI interface JSON #500

Closed

Strict transaction value handling #563

Closed

axic deleted the feature/accept-ether branch October 25, 2016 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Add payable modifier #665

BREAKING: Add payable modifier #665

axic commented Jun 18, 2016 •

edited by chriseth

Loading

chriseth commented Jun 21, 2016

redsquirrel commented Jun 21, 2016

axic commented Jun 24, 2016

axic commented Jun 24, 2016

chriseth commented Jul 8, 2016

axic commented Jul 12, 2016 •

edited

Loading

chriseth commented Jul 18, 2016 •

edited

Loading

axic commented Aug 1, 2016

chriseth commented Aug 1, 2016

axic commented Aug 2, 2016

chriseth commented Aug 5, 2016

axic Aug 5, 2016

chriseth Aug 8, 2016 •

edited

Loading

axic commented Aug 5, 2016 •

edited

Loading

axic commented Aug 8, 2016

chriseth Aug 8, 2016

axic Aug 8, 2016

chriseth commented Aug 8, 2016

chriseth commented Aug 25, 2016 •

edited

Loading

ethernomad commented Aug 26, 2016

chriseth commented Aug 26, 2016

axic commented Aug 31, 2016

chriseth commented Sep 1, 2016

chriseth commented Sep 5, 2016

pirapira Sep 5, 2016 •

edited

Loading

axic Sep 6, 2016

axic commented Sep 6, 2016 •

edited

Loading

chriseth commented Sep 6, 2016

axic commented Sep 6, 2016

BREAKING: Add payable modifier #665

BREAKING: Add payable modifier #665

Conversation

axic commented Jun 18, 2016 • edited by chriseth Loading

chriseth commented Jun 21, 2016

redsquirrel commented Jun 21, 2016

axic commented Jun 24, 2016

axic commented Jun 24, 2016

chriseth commented Jul 8, 2016

axic commented Jul 12, 2016 • edited Loading

chriseth commented Jul 18, 2016 • edited Loading

axic commented Aug 1, 2016

chriseth commented Aug 1, 2016

axic commented Aug 2, 2016

chriseth commented Aug 5, 2016

axic Aug 5, 2016

Choose a reason for hiding this comment

chriseth Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

axic commented Aug 5, 2016 • edited Loading

axic commented Aug 8, 2016

chriseth Aug 8, 2016

Choose a reason for hiding this comment

axic Aug 8, 2016

Choose a reason for hiding this comment

chriseth commented Aug 8, 2016

chriseth commented Aug 25, 2016 • edited Loading

ethernomad commented Aug 26, 2016

chriseth commented Aug 26, 2016

axic commented Aug 31, 2016

chriseth commented Sep 1, 2016

chriseth commented Sep 5, 2016

pirapira Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

axic Sep 6, 2016

Choose a reason for hiding this comment

axic commented Sep 6, 2016 • edited Loading

chriseth commented Sep 6, 2016

axic commented Sep 6, 2016

axic commented Jun 18, 2016 •

edited by chriseth

Loading

axic commented Jul 12, 2016 •

edited

Loading

chriseth commented Jul 18, 2016 •

edited

Loading

chriseth Aug 8, 2016 •

edited

Loading

axic commented Aug 5, 2016 •

edited

Loading

chriseth commented Aug 25, 2016 •

edited

Loading

pirapira Sep 5, 2016 •

edited

Loading

axic commented Sep 6, 2016 •

edited

Loading