From 962531af96a8a3ed6b28462d43c69d78fa48d511 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Fri, 26 Aug 2016 19:37:10 +0100 Subject: [PATCH] Merged in changes from chriseth/payable --- libsolidity/analysis/TypeChecker.cpp | 14 +++- libsolidity/ast/Types.cpp | 31 +++++---- libsolidity/codegen/ContractCompiler.cpp | 13 +--- libsolidity/parsing/Parser.cpp | 12 ++-- test/contracts/AuctionRegistrar.cpp | 7 +- test/contracts/FixedFeeRegistrar.cpp | 2 +- test/contracts/Wallet.cpp | 2 +- test/libsolidity/SolidityEndToEndTest.cpp | 49 ++++++++++---- .../SolidityNameAndTypeResolution.cpp | 64 ++++++++++++++++++- test/libsolidity/SolidityParser.cpp | 10 +++ 10 files changed, 150 insertions(+), 54 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index bc03da01f..c17813978 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -272,6 +272,7 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr if ( overriding->visibility() != function->visibility() || overriding->isDeclaredConst() != function->isDeclaredConst() || + overriding->isPayable() != function->isPayable() || overridingType != functionType ) typeError(overriding->location(), "Override changes extended function signature."); @@ -416,6 +417,13 @@ bool TypeChecker::visit(StructDefinition const& _struct) bool TypeChecker::visit(FunctionDefinition const& _function) { bool isLibraryFunction = dynamic_cast(*_function.scope()).isLibrary(); + if (_function.isPayable()) + { + if (isLibraryFunction) + typeError(_function.location(), "Library functions cannot be payable."); + if (!_function.isConstructor() && !_function.name().empty() && !_function.isPartOfExternalInterface()) + typeError(_function.location(), "Internal functions cannot be payable."); + } for (ASTPointer const& var: _function.parameters() + _function.returnParameters()) { if (!type(*var)->canLiveOutsideStorage()) @@ -1328,14 +1336,16 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess) fatalTypeError( _memberAccess.location(), "Member \"" + memberName + "\" not found or not visible " - "after argument-dependent lookup in " + exprType->toString() + "after argument-dependent lookup in " + exprType->toString() + + (memberName == "value" ? " - did you forget the \"payable\" modifier?" : "") ); } else if (possibleMembers.size() > 1) fatalTypeError( _memberAccess.location(), "Member \"" + memberName + "\" not unique " - "after argument-dependent lookup in " + exprType->toString() + "after argument-dependent lookup in " + exprType->toString() + + (memberName == "value" ? " - did you forget the \"payable\" modifier?" : "") ); auto& annotation = _memberAccess.annotation(); diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index 938dced6b..ca8490f24 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -1890,20 +1890,23 @@ MemberList::MemberMap FunctionType::nativeMembers(ContractDefinition const*) con { MemberList::MemberMap members; if (m_location != Location::BareDelegateCall && m_location != Location::DelegateCall) - members.push_back(MemberList::Member( - "value", - make_shared( - parseElementaryTypeVector({"uint"}), - TypePointers{copyAndSetGasOrValue(false, true)}, - strings(), - strings(), - Location::SetValue, - false, - nullptr, - m_gasSet, - m_valueSet - ) - )); + { + if (!m_declaration || m_isPayable) // If this is an actual function, it has to be payable + members.push_back(MemberList::Member( + "value", + make_shared( + parseElementaryTypeVector({"uint"}), + TypePointers{copyAndSetGasOrValue(false, true)}, + strings(), + strings(), + Location::SetValue, + false, + nullptr, + m_gasSet, + m_valueSet + ) + )); + } if (m_location != Location::Creation) members.push_back(MemberList::Member( "gas", diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index eba851033..58e6847c6 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -243,14 +243,6 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac if (fallback) { eth::AssemblyItem returnTag = m_context.pushNewTag(); - - // Reject transaction if value is not accepted, but was received - if (!fallback->isPayable()) - { - m_context << Instruction::CALLVALUE; - m_context.appendConditionalJumpTo(m_context.errorTag()); - } - fallback->accept(*this); m_context << returnTag; appendReturnValuePacker(FunctionType(*fallback).returnParameterTypes(), _contract.isLibrary()); @@ -265,15 +257,14 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac CompilerContext::LocationSetter locationSetter(m_context, functionType->declaration()); m_context << callDataUnpackerEntryPoints.at(it.first); - eth::AssemblyItem returnTag = m_context.pushNewTag(); - - // Reject transaction if value is not accepted, but was received if (!functionType->isPayable()) { + // Throw if function is not payable but call contained ether. m_context << Instruction::CALLVALUE; m_context.appendConditionalJumpTo(m_context.errorTag()); } + eth::AssemblyItem returnTag = m_context.pushNewTag(); m_context << CompilerUtils::dataStartOffset; appendCalldataUnpacker(functionType->parameterTypes()); m_context.appendJumpTo(m_context.functionEntryLabel(functionType->declaration())); diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp index 1d76f80a1..0e99d1e73 100644 --- a/libsolidity/parsing/Parser.cpp +++ b/libsolidity/parsing/Parser.cpp @@ -305,6 +305,7 @@ ASTPointer Parser::parseFunctionDefinition(ASTString const* options.allowLocationSpecifier = true; ASTPointer parameters(parseParameterList(options)); bool isDeclaredConst = false; + bool isPayable = false; Declaration::Visibility visibility(Declaration::Visibility::Default); vector> modifiers; while (true) @@ -315,6 +316,11 @@ ASTPointer Parser::parseFunctionDefinition(ASTString const* isDeclaredConst = true; m_scanner->next(); } + else if (m_scanner->currentToken() == Token::Payable) + { + isPayable = true; + m_scanner->next(); + } else if (token == Token::Identifier) modifiers.push_back(parseModifierInvocation()); else if (Token::isVisibilitySpecifier(token)) @@ -326,12 +332,6 @@ ASTPointer Parser::parseFunctionDefinition(ASTString const* else break; } - bool isPayable = false; - if (m_scanner->currentToken() == Token::Payable) - { - isPayable = true; - m_scanner->next(); - } ASTPointer returnParameters; if (m_scanner->currentToken() == Token::Returns) { diff --git a/test/contracts/AuctionRegistrar.cpp b/test/contracts/AuctionRegistrar.cpp index 9161f2ce1..05da34901 100644 --- a/test/contracts/AuctionRegistrar.cpp +++ b/test/contracts/AuctionRegistrar.cpp @@ -115,11 +115,6 @@ contract GlobalRegistrar is Registrar, AuctionSystem { // TODO: Populate with hall-of-fame. } - function() { - // prevent people from just sending funds to the registrar - throw; - } - function onAuctionEnd(string _name) internal { var auction = m_auctions[_name]; var record = m_toRecord[_name]; @@ -136,7 +131,7 @@ contract GlobalRegistrar is Registrar, AuctionSystem { } } - function reserve(string _name) external { + function reserve(string _name) external payable { if (bytes(_name).length == 0) throw; bool needAuction = requiresAuction(_name); diff --git a/test/contracts/FixedFeeRegistrar.cpp b/test/contracts/FixedFeeRegistrar.cpp index c37873cf5..af8ee5950 100644 --- a/test/contracts/FixedFeeRegistrar.cpp +++ b/test/contracts/FixedFeeRegistrar.cpp @@ -73,7 +73,7 @@ contract FixedFeeRegistrar is Registrar { modifier onlyrecordowner(string _name) { if (m_record(_name).owner == msg.sender) _; } - function reserve(string _name) { + function reserve(string _name) payable { Record rec = m_record(_name); if (rec.owner == 0 && msg.value >= c_fee) { rec.owner = msg.sender; diff --git a/test/contracts/Wallet.cpp b/test/contracts/Wallet.cpp index d3e1b8d7f..b4f29a872 100644 --- a/test/contracts/Wallet.cpp +++ b/test/contracts/Wallet.cpp @@ -378,7 +378,7 @@ contract Wallet is multisig, multiowned, daylimit { } // gets called when no other function matches - function() { + function() payable { // just being sent some cash? if (msg.value > 0) Deposit(msg.sender, msg.value); diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 65f81deab..c1f1b148f 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -2085,11 +2085,11 @@ BOOST_AUTO_TEST_CASE(gas_and_value_basic) function sendAmount(uint amount) payable returns (uint256 bal) { return h.getBalance.value(amount)(); } - function outOfGas() payable returns (bool ret) { + function outOfGas() returns (bool ret) { h.setFlag.gas(2)(); // should fail due to OOG return true; } - function checkState() payable returns (bool flagAfter, uint myBal) { + function checkState() returns (bool flagAfter, uint myBal) { flagAfter = h.getFlag(); myBal = this.balance; } @@ -2098,15 +2098,15 @@ BOOST_AUTO_TEST_CASE(gas_and_value_basic) compileAndRun(sourceCode, 20); BOOST_REQUIRE(callContractFunction("sendAmount(uint256)", 5) == encodeArgs(5)); // call to helper should not succeed but amount should be transferred anyway - BOOST_REQUIRE(callContractFunction("outOfGas()", 5) == bytes()); - BOOST_REQUIRE(callContractFunction("checkState()", 5) == encodeArgs(false, 20 - 5)); + BOOST_REQUIRE(callContractFunction("outOfGas()") == bytes()); + BOOST_REQUIRE(callContractFunction("checkState()") == encodeArgs(false, 20 - 5)); } BOOST_AUTO_TEST_CASE(gas_for_builtin) { char const* sourceCode = R"( contract Contract { - function test(uint g) payable returns (bytes32 data, bool flag) { + function test(uint g) returns (bytes32 data, bool flag) { data = ripemd160.gas(g)("abc"); flag = true; } @@ -2151,7 +2151,7 @@ BOOST_AUTO_TEST_CASE(value_insane) contract test { helper h; function test() payable { h = new helper(); } - function sendAmount(uint amount) payable returns (uint256 bal) { + function sendAmount(uint amount) returns (uint256 bal) { var x1 = h.getBalance.value; var x2 = x1(amount).gas; var x3 = x2(1000).value; @@ -7090,18 +7090,17 @@ BOOST_AUTO_TEST_CASE(calling_nonexisting_contract_throws) BOOST_CHECK(callContractFunction("h()") == encodeArgs(u256(7))); } -BOOST_AUTO_TEST_CASE(payable_accept_explicit_constructor) +BOOST_AUTO_TEST_CASE(payable_constructor) { char const* sourceCode = R"( contract C { - function () payable { } + function C() payable { } } )"; compileAndRun(sourceCode, 27, "C"); } - -BOOST_AUTO_TEST_CASE(payable_accept_explicit) +BOOST_AUTO_TEST_CASE(payable_function) { char const* sourceCode = R"( contract C { @@ -7122,10 +7121,19 @@ BOOST_AUTO_TEST_CASE(non_payable_throw_constructor) { char const* sourceCode = R"( contract C { - function() { } + function C() { } + } + contract D { + function D() payable {} + function f() returns (uint) { + (new C).value(2)(); + return 2; + } } )"; - compileAndRun(sourceCode, 27, "C"); + compileAndRun(sourceCode, 27, "D"); + BOOST_CHECK(callContractFunction("f()") == encodeArgs()); + BOOST_CHECK_EQUAL(balanceAt(m_contractAddress), 27); } BOOST_AUTO_TEST_CASE(non_payable_throw) @@ -7147,6 +7155,23 @@ BOOST_AUTO_TEST_CASE(non_payable_throw) BOOST_CHECK(callContractFunctionWithValue("a()", 27) == encodeArgs()); } +BOOST_AUTO_TEST_CASE(no_nonpayable_circumvention_by_modifier) +{ + char const* sourceCode = R"( + contract C { + modifier tryCircumvent { + if (false) _ // avoid the function, we should still not accept ether + } + function f() tryCircumvent returns (uint) { + return msg.value; + } + } + )"; + compileAndRun(sourceCode); + BOOST_CHECK(callContractFunctionWithValue("f()", 27) == encodeArgs()); +} + + BOOST_AUTO_TEST_SUITE_END() } diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 8d4890d1a..e193745dd 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -3852,7 +3852,69 @@ BOOST_AUTO_TEST_CASE(modifier_without_underscore) modifier m() {} } )"; - BOOST_CHECK(expectError(text, true) == Error::Type::SyntaxError); + BOOST_CHECK(expectError(text) == Error::Type::SyntaxError); +} + +BOOST_AUTO_TEST_CASE(payable_in_library) +{ + char const* text = R"( + library test { + function f() payable {} + } + )"; + BOOST_CHECK(expectError(text) == Error::Type::TypeError); +} + +BOOST_AUTO_TEST_CASE(payable_internal) +{ + char const* text = R"( + contract test { + function f() payable internal {} + } + )"; + BOOST_CHECK(expectError(text) == Error::Type::TypeError); +} + +BOOST_AUTO_TEST_CASE(illegal_override_payable) +{ + char const* text = R"( + contract B { function f() payable {} } + contract C is B { function f() {} } + )"; + BOOST_CHECK(expectError(text) == Error::Type::TypeError); +} + +BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable) +{ + char const* text = R"( + contract B { function f() {} } + contract C is B { function f() payable {} } + )"; + BOOST_CHECK(expectError(text) == Error::Type::TypeError); +} + +BOOST_AUTO_TEST_CASE(calling_payable) +{ + char const* text = R"( + contract receiver { function pay() payable {} } + contract test { + funciton f() { (new receiver()).pay.value(10)(); } + recevier r = new receiver(); + function g() { r.pay.value(10)(); } + } + )"; + BOOST_CHECK(success(text)); +} + +BOOST_AUTO_TEST_CASE(calling_nonpayable) +{ + char const* text = R"( + contract receiver { function nopay() {} } + contract test { + function_argument_mem_to_storage f() { (new receiver()).nopay.value(10)(); } + } + )"; + BOOST_CHECK(expectError(text) == Error::Type::TypeError); } BOOST_AUTO_TEST_CASE(warn_nonpresent_pragma) diff --git a/test/libsolidity/SolidityParser.cpp b/test/libsolidity/SolidityParser.cpp index 0c0e343b2..a81a98284 100644 --- a/test/libsolidity/SolidityParser.cpp +++ b/test/libsolidity/SolidityParser.cpp @@ -1231,6 +1231,16 @@ BOOST_AUTO_TEST_CASE(invalid_fixed_conversion_leading_zeroes_check) BOOST_CHECK(!successParse(text)); } +BOOST_AUTO_TEST_CASE(payable_accessor) +{ + char const* text = R"( + contract test { + uint payable x; + } + )"; + BOOST_CHECK(!successParse(text)); +} + BOOST_AUTO_TEST_SUITE_END() }