From f3d7d68c6d87918fb1909175e56d077dc4822bf0 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Fri, 22 Oct 2021 20:28:43 -0400 Subject: [PATCH 01/11] allow arbitrary revert reason strings --- tests/parser/functions/test_slice.py | 2 +- vyper/old_codegen/stmt.py | 50 ++++++++++++++++++++-------- vyper/semantics/validation/local.py | 4 +-- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/tests/parser/functions/test_slice.py b/tests/parser/functions/test_slice.py index 725428e804..bfce521505 100644 --- a/tests/parser/functions/test_slice.py +++ b/tests/parser/functions/test_slice.py @@ -100,6 +100,7 @@ def ret10_slice() -> Bytes[10]: c = get_contract(code) assert c.ret10_slice() == b"A" + def test_slice_expr(get_contract): # test slice of a complex expression code = """ @@ -112,7 +113,6 @@ def ret10_slice() -> Bytes[10]: assert c.ret10_slice() == b"A" - code_bytes32 = [ """ foo: bytes32 diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index b2e9979966..d1703c333f 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -2,13 +2,19 @@ import vyper.utils as util from vyper import ast as vy_ast from vyper.builtin_functions import STMT_DISPATCH_TABLE -from vyper.exceptions import StructureException, TypeCheckFailure +from vyper.exceptions import CompilerPanic, StructureException, TypeCheckFailure from vyper.old_codegen import external_call, self_call -from vyper.old_codegen.context import Context +from vyper.old_codegen.context import Constancy, Context from vyper.old_codegen.expr import Expr -from vyper.old_codegen.parser_utils import LLLnode, getpos, make_setter, unwrap_location +from vyper.old_codegen.parser_utils import ( + LLLnode, + getpos, + make_byte_array_copier, + make_setter, + unwrap_location, +) from vyper.old_codegen.return_ import make_return_stmt -from vyper.old_codegen.types import BaseType, ByteArrayType, ListType, get_size_of_type, parse_type +from vyper.old_codegen.types import BaseType, ByteArrayType, ListType, parse_type class Stmt: @@ -142,23 +148,39 @@ def _assert_reason(self, test_expr, msg): if isinstance(msg, vy_ast.Name) and msg.id == "UNREACHABLE": return LLLnode.from_list(["assert_unreachable", test_expr], typ=None, pos=getpos(msg)) - reason_str_type = ByteArrayType(len(msg.value.strip())) + # set constant so that revert reason str is well behaved + self.context.constancy = Constancy.Constant + msg_lll = Expr(msg, self.context).lll_node - # abi encode the reason string - sig_placeholder = self.context.new_internal_variable(BaseType(32)) - # offset of bytes in (bytes,) - arg_placeholder = self.context.new_internal_variable(BaseType(32)) - placeholder_bytes = Expr(msg, self.context).lll_node + def _get_last(lll): + if len(lll.args) == 0: + return lll.value + return _get_last(lll.args[-1]) + + if msg_lll.location != "memory": + buf = self.context.new_internal_variable(msg_lll.typ) + instantiate_msg = make_byte_array_copier(buf, msg_lll) + else: + buf = _get_last(msg_lll) + if not isinstance(buf, int): + print(type(buf)) + raise CompilerPanic(f"invalid bytestring {buf}\n{self}") + instantiate_msg = msg_lll + # offset of bytes in (bytes,) method_id = util.abi_method_id("Error(string)") # abi encode method_id + bytestring + assert buf >= 36, "invalid buffer" + # we don't mind overwriting other memory because we are + # getting out of here anyway. + _runtime_length = ["mload", buf] revert_seq = [ "seq", - ["mstore", sig_placeholder, method_id], - ["mstore", arg_placeholder, 32], - placeholder_bytes, - ["revert", sig_placeholder + 28, int(32 + 4 + get_size_of_type(reason_str_type) * 32)], + instantiate_msg, + ["mstore", buf - 36, method_id], + ["mstore", buf - 32, 0x20], + ["revert", buf - 36, ["add", 36, _runtime_length]], ] if test_expr: lll_node = ["if", ["iszero", test_expr], revert_seq] diff --git a/vyper/semantics/validation/local.py b/vyper/semantics/validation/local.py index 383dbb0070..909adf2cb7 100644 --- a/vyper/semantics/validation/local.py +++ b/vyper/semantics/validation/local.py @@ -30,6 +30,7 @@ from vyper.semantics.types.user.event import Event from vyper.semantics.types.user.struct import StructDefinition from vyper.semantics.types.utils import get_type_from_annotation +from vyper.semantics.types.value.array_value import StringDefinition from vyper.semantics.types.value.boolean import BoolDefinition from vyper.semantics.types.value.numeric import Uint256Definition from vyper.semantics.validation.annotation import StatementAnnotationVisitor @@ -103,8 +104,7 @@ def _validate_revert_reason(msg_node: vy_ast.VyperNode) -> None: if isinstance(msg_node, vy_ast.Str): if not msg_node.value.strip(): raise StructureException("Reason string cannot be empty", msg_node) - elif not (isinstance(msg_node, vy_ast.Name) and msg_node.id == "UNREACHABLE"): - raise InvalidType("Reason must UNREACHABLE or a string literal", msg_node) + validate_expected_type(msg_node, StringDefinition(64)) class FunctionNodeVisitor(VyperNodeVisitorBase): From af983790164e1d19ff2f6ad186e079460e64bb39 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 00:43:34 +0000 Subject: [PATCH 02/11] fix buffer offset/length --- vyper/old_codegen/stmt.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index d1703c333f..8f173658b4 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -152,6 +152,7 @@ def _assert_reason(self, test_expr, msg): self.context.constancy = Constancy.Constant msg_lll = Expr(msg, self.context).lll_node + # TODO this is probably useful in parser_utils def _get_last(lll): if len(lll.args) == 0: return lll.value @@ -163,7 +164,6 @@ def _get_last(lll): else: buf = _get_last(msg_lll) if not isinstance(buf, int): - print(type(buf)) raise CompilerPanic(f"invalid bytestring {buf}\n{self}") instantiate_msg = msg_lll @@ -178,9 +178,9 @@ def _get_last(lll): revert_seq = [ "seq", instantiate_msg, - ["mstore", buf - 36, method_id], + ["mstore", buf - 64, method_id], ["mstore", buf - 32, 0x20], - ["revert", buf - 36, ["add", 36, _runtime_length]], + ["revert", buf - 36, ["add", 4 + 32 + 32, _runtime_length]], ] if test_expr: lll_node = ["if", ["iszero", test_expr], revert_seq] From 8cdba361947fa5d7ed21146f0f8c4b670ed5a624 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 01:01:29 +0000 Subject: [PATCH 03/11] spruce up a couple error messages --- vyper/old_codegen/stmt.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index 8f173658b4..41634f8225 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -416,19 +416,21 @@ def parse_Return(self): return make_return_stmt(lll_val, self.stmt, self.context) def _get_target(self, target): + _dbg_expr = target + if isinstance(target, vy_ast.Name) and target.id in self.context.forvars: - raise TypeCheckFailure("Failed for-loop constancy check") + raise TypeCheckFailure(f"Failed constancy check\n{_dbg_expr}") if isinstance(target, vy_ast.Tuple): target = Expr(target, self.context).lll_node for node in target.args: if (node.location == "storage" and self.context.is_constant()) or not node.mutable: - raise TypeCheckFailure("Failed for-loop constancy check") + raise TypeCheckFailure(f"Failed constancy check\n{_dbg_expr}") return target target = Expr.parse_variable_location(target, self.context) if (target.location == "storage" and self.context.is_constant()) or not target.mutable: - raise TypeCheckFailure("Failed for-loop constancy check") + raise TypeCheckFailure(f"Failed constancy check\n{_dbg_expr}") return target From 02a51e65dc30f95cea2bb88ab4e3a5d8f77fdb6a Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 01:02:52 +0000 Subject: [PATCH 04/11] fix constancy scoping --- vyper/old_codegen/stmt.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index 41634f8225..52b1d9b94a 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -149,8 +149,12 @@ def _assert_reason(self, test_expr, msg): return LLLnode.from_list(["assert_unreachable", test_expr], typ=None, pos=getpos(msg)) # set constant so that revert reason str is well behaved - self.context.constancy = Constancy.Constant - msg_lll = Expr(msg, self.context).lll_node + try: + tmp = self.context.constancy + self.context.constancy = Constancy.Constant + msg_lll = Expr(msg, self.context).lll_node + finally: + self.context.constancy = tmp # TODO this is probably useful in parser_utils def _get_last(lll): From 7b44283b69d906baf719750f8a3298c12db08236 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 01:12:08 +0000 Subject: [PATCH 05/11] fix revert reason typecheck --- vyper/semantics/validation/local.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vyper/semantics/validation/local.py b/vyper/semantics/validation/local.py index 909adf2cb7..5dcb5c357c 100644 --- a/vyper/semantics/validation/local.py +++ b/vyper/semantics/validation/local.py @@ -104,7 +104,11 @@ def _validate_revert_reason(msg_node: vy_ast.VyperNode) -> None: if isinstance(msg_node, vy_ast.Str): if not msg_node.value.strip(): raise StructureException("Reason string cannot be empty", msg_node) - validate_expected_type(msg_node, StringDefinition(64)) + elif not (isinstance(msg_node, vy_ast.Name) and msg_node.id == "UNREACHABLE"): + try: + validate_expected_type(msg_node, StringDefinition(64)) + except TypeMismatch as e: + raise InvalidType("revert reason must be String[64]") from e class FunctionNodeVisitor(VyperNodeVisitorBase): From b4ad9351857bbda602a9e8cc9aea711aaf5f6017 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 01:12:26 +0000 Subject: [PATCH 06/11] fix a test - minter was invalid name --- tests/parser/exceptions/test_invalid_type_exception.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/parser/exceptions/test_invalid_type_exception.py b/tests/parser/exceptions/test_invalid_type_exception.py index d5641b78ee..f2f9a45720 100644 --- a/tests/parser/exceptions/test_invalid_type_exception.py +++ b/tests/parser/exceptions/test_invalid_type_exception.py @@ -42,7 +42,7 @@ def foo(): """ @external def mint(_to: address, _value: uint256): - assert msg.sender == self,minter + assert msg.sender == self,msg.sender """, # Raise reason must be string """ From 88c5f7b3e3d958ab75626f115224908386143580 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 01:23:33 +0000 Subject: [PATCH 07/11] fix revert reason zero pad --- vyper/old_codegen/stmt.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index 52b1d9b94a..0dc8dfa1a9 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -12,6 +12,7 @@ make_byte_array_copier, make_setter, unwrap_location, + mzero, ) from vyper.old_codegen.return_ import make_return_stmt from vyper.old_codegen.types import BaseType, ByteArrayType, ListType, parse_type @@ -181,10 +182,12 @@ def _get_last(lll): _runtime_length = ["mload", buf] revert_seq = [ "seq", + # abi_encode uses zero pad, this uses less space. + mzero(buf, msg_lll.typ.maxlen), instantiate_msg, ["mstore", buf - 64, method_id], ["mstore", buf - 32, 0x20], - ["revert", buf - 36, ["add", 4 + 32 + 32, _runtime_length]], + ["revert", buf - 36, ["add", 4 + 32 + 32, ["ceil32", _runtime_length]]], ] if test_expr: lll_node = ["if", ["iszero", test_expr], revert_seq] From cf5522c6881d11de3b3fbbaf4080e29c9dd637db Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 10:41:39 -0700 Subject: [PATCH 08/11] fix lark grammar for assert/raise reason --- tests/grammar/vyper.lark | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/grammar/vyper.lark b/tests/grammar/vyper.lark index 5a354ade10..2b9783ac9e 100644 --- a/tests/grammar/vyper.lark +++ b/tests/grammar/vyper.lark @@ -139,10 +139,10 @@ log_stmt: _LOG NAME "(" [arguments] ")" return_stmt: _RETURN [_expr ("," _expr)*] _UNREACHABLE: "UNREACHABLE" raise_stmt: _RAISE -> raise - | _RAISE STRING -> raise_with_reason + | _RAISE _expr -> raise_with_reason | _RAISE _UNREACHABLE -> raise_unreachable assert_stmt: _ASSERT _expr -> assert - | _ASSERT _expr "," STRING -> assert_with_reason + | _ASSERT _expr "," _expr -> assert_with_reason | _ASSERT _expr "," _UNREACHABLE -> assert_unreachable body: _NEWLINE _INDENT ([COMMENT] _NEWLINE | _stmt)+ _DEDENT From f81d581a256864202850cccbd4271ff741a0c4ea Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 10:42:09 -0700 Subject: [PATCH 09/11] clarify an _assert_reason optimization --- vyper/old_codegen/stmt.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index 0dc8dfa1a9..23ee353d69 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -189,7 +189,8 @@ def _get_last(lll): ["mstore", buf - 32, 0x20], ["revert", buf - 36, ["add", 4 + 32 + 32, ["ceil32", _runtime_length]]], ] - if test_expr: + + if test_expr is not None: lll_node = ["if", ["iszero", test_expr], revert_seq] else: lll_node = revert_seq @@ -212,7 +213,7 @@ def parse_Assert(self): def parse_Raise(self): if self.stmt.exc: - return self._assert_reason(0, self.stmt.exc) + return self._assert_reason(None, self.stmt.exc) else: return LLLnode.from_list(["revert", 0, 0], typ=None, pos=getpos(self.stmt)) From 2a4972ffb9f012f3d303e1181b4c91379690e8df Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 10:51:33 -0700 Subject: [PATCH 10/11] add tests for variable revert reasons --- tests/parser/features/test_assert.py | 18 +++++++++--------- vyper/old_codegen/stmt.py | 5 ++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/parser/features/test_assert.py b/tests/parser/features/test_assert.py index adad5e57af..d31edc74a4 100644 --- a/tests/parser/features/test_assert.py +++ b/tests/parser/features/test_assert.py @@ -29,15 +29,15 @@ def test(a: int128) -> int128: return 1 + a @external -def test2(a: int128, b: int128) -> int128: +def test2(a: int128, b: int128, extra_reason: String[32]) -> int128: c: int128 = 11 assert a > 1, "a is not large enough" - assert b == 1, "b may only be 1" + assert b == 1, concat("b may only be 1", extra_reason) return a + b + c @external -def test3() : - raise "An exception" +def test3(reason_str: String[32]): + raise reason_str """ c = get_contract_with_gas_estimation(code) @@ -48,17 +48,17 @@ def test3() : assert e_info.value.args[0] == "larger than one please" # a = 0, b = 1 with pytest.raises(TransactionFailed) as e_info: - c.test2(0, 1) + c.test2(0, 1, "") assert e_info.value.args[0] == "a is not large enough" # a = 1, b = 0 with pytest.raises(TransactionFailed) as e_info: - c.test2(2, 2) - assert e_info.value.args[0] == "b may only be 1" + c.test2(2, 2, " because I said so") + assert e_info.value.args[0] == "b may only be 1" + " because I said so" # return correct value - assert c.test2(5, 1) == 17 + assert c.test2(5, 1, "") == 17 with pytest.raises(TransactionFailed) as e_info: - c.test3() + c.test3("An exception") assert e_info.value.args[0] == "An exception" diff --git a/vyper/old_codegen/stmt.py b/vyper/old_codegen/stmt.py index 23ee353d69..ff2fca79ce 100644 --- a/vyper/old_codegen/stmt.py +++ b/vyper/old_codegen/stmt.py @@ -12,7 +12,7 @@ make_byte_array_copier, make_setter, unwrap_location, - mzero, + zero_pad, ) from vyper.old_codegen.return_ import make_return_stmt from vyper.old_codegen.types import BaseType, ByteArrayType, ListType, parse_type @@ -182,9 +182,8 @@ def _get_last(lll): _runtime_length = ["mload", buf] revert_seq = [ "seq", - # abi_encode uses zero pad, this uses less space. - mzero(buf, msg_lll.typ.maxlen), instantiate_msg, + zero_pad(buf), ["mstore", buf - 64, method_id], ["mstore", buf - 32, 0x20], ["revert", buf - 36, ["add", 4 + 32 + 32, ["ceil32", _runtime_length]]], From 373fa5c0d9d7d0fece044ee52dfb83d5a8e9da2b Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Sun, 24 Oct 2021 20:09:51 +0000 Subject: [PATCH 11/11] chore: fix tests for upgrade to web3==5.21.0 --- setup.py | 2 +- tests/parser/features/test_assert.py | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/setup.py b/setup.py index 3231e310d5..44f27fa37f 100644 --- a/setup.py +++ b/setup.py @@ -15,7 +15,7 @@ "pytest-xdist>=1.32,<2.0", "eth-tester[py-evm]>=0.5.0b1,<0.6", "py-evm==0.4.0a4", # NOTE: temporarily pinned until we have support for py-evm 0.5.0a0+ - "web3==5.12.3", + "web3==5.21.0", "tox>=3.15,<4.0", "lark-parser==0.10.0", "hypothesis[lark]>=5.37.1,<6.0", diff --git a/tests/parser/features/test_assert.py b/tests/parser/features/test_assert.py index d31edc74a4..de4b7359f7 100644 --- a/tests/parser/features/test_assert.py +++ b/tests/parser/features/test_assert.py @@ -3,6 +3,11 @@ from eth_tester.exceptions import TransactionFailed +# web3 returns f"execution reverted: {err_str}" +def _fixup_err_str(s): + return s.replace("execution reverted: ", "") + + def test_assert_refund(w3, get_contract_with_gas_estimation, assert_tx_failed): code = """ @external @@ -45,21 +50,21 @@ def test3(reason_str: String[32]): with pytest.raises(TransactionFailed) as e_info: c.test(0) - assert e_info.value.args[0] == "larger than one please" + assert _fixup_err_str(e_info.value.args[0]) == "larger than one please" # a = 0, b = 1 with pytest.raises(TransactionFailed) as e_info: c.test2(0, 1, "") - assert e_info.value.args[0] == "a is not large enough" + assert _fixup_err_str(e_info.value.args[0]) == "a is not large enough" # a = 1, b = 0 with pytest.raises(TransactionFailed) as e_info: c.test2(2, 2, " because I said so") - assert e_info.value.args[0] == "b may only be 1" + " because I said so" + assert _fixup_err_str(e_info.value.args[0]) == "b may only be 1" + " because I said so" # return correct value assert c.test2(5, 1, "") == 17 with pytest.raises(TransactionFailed) as e_info: c.test3("An exception") - assert e_info.value.args[0] == "An exception" + assert _fixup_err_str(e_info.value.args[0]) == "An exception" invalid_code = [ @@ -183,7 +188,7 @@ def test(x: uint256[3]) -> bool: assert_tx_failed(lambda: c.test([1, 3, 5])) -def test_assest_reason_revert_length(w3, get_contract, memory_mocker): +def test_assert_reason_revert_length(w3, get_contract, memory_mocker): code = """ @external def test() -> int128: @@ -194,7 +199,7 @@ def test() -> int128: w3.manager.provider.ethereum_tester.backend.is_eip838_error = lambda err: False with pytest.raises(TransactionFailed) as e_info: c.test() - error_bytes = eval(e_info.value.args[0]) + error_bytes = eval(_fixup_err_str(e_info.value.args[0])) assert len(error_bytes) == 100 msg = decode_single("string", error_bytes[36:]) assert msg == "oops"