From 68a368c269ea7b6231c6e41a400e487ce81a2fef Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Thu, 13 Apr 2023 23:20:05 +0600 Subject: [PATCH 1/6] fix: capture response from raw_call --- contracts/GateSeal.vy | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/GateSeal.vy b/contracts/GateSeal.vy index 1479543..3e30bcb 100644 --- a/contracts/GateSeal.vy +++ b/contracts/GateSeal.vy @@ -159,11 +159,15 @@ def seal(_sealables: DynArray[address, MAX_SEALABLES]): assert sealable in self.sealables, "sealables: includes a non-sealable" success: bool = False + response: Bytes[32] = b"" # using `raw_call` to catch external revert and continue execution - success = raw_call( + # capturing `response` to keep the compiler from what acting out, + # for details, see https://docs.vyperlang.org/en/stable/built-in-functions.html#raw_call + success, response = raw_call( sealable, _abi_encode(SEAL_DURATION_SECONDS, method_id=method_id("pauseFor(uint256)")), + max_outsize=32, revert_on_failure=False ) From c0f1d37484d0e059e8c40be1418d45263d25a589 Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Fri, 14 Apr 2023 12:24:21 +0600 Subject: [PATCH 2/6] fix: typo --- contracts/GateSeal.vy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/GateSeal.vy b/contracts/GateSeal.vy index 3e30bcb..51f1ba9 100644 --- a/contracts/GateSeal.vy +++ b/contracts/GateSeal.vy @@ -162,7 +162,7 @@ def seal(_sealables: DynArray[address, MAX_SEALABLES]): response: Bytes[32] = b"" # using `raw_call` to catch external revert and continue execution - # capturing `response` to keep the compiler from what acting out, + # capturing `response` to keep the compiler from acting out, # for details, see https://docs.vyperlang.org/en/stable/built-in-functions.html#raw_call success, response = raw_call( sealable, From f5ffd5b7f6ec65f0be2d62280cd90e6ae3bf232f Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Fri, 14 Apr 2023 15:28:18 +0600 Subject: [PATCH 3/6] test: raw_call returns actual success --- contracts/test_helpers/SealableMock.vy | 7 +++++ tests/test_gate_seal.py | 43 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/contracts/test_helpers/SealableMock.vy b/contracts/test_helpers/SealableMock.vy index 7034c9e..547e03c 100644 --- a/contracts/test_helpers/SealableMock.vy +++ b/contracts/test_helpers/SealableMock.vy @@ -16,6 +16,13 @@ def isPaused() -> bool: return self._is_paused() +@external +def __force_pause_for(_duration: uint256): + # pause ignoring any checks + # required to simulate cases where Sealable is already paused but the seal() reverts + self.resumed_timestamp = block.timestamp + _duration + + @external def pauseFor(_duration: uint256): assert not self.reverts, "simulating revert" diff --git a/tests/test_gate_seal.py b/tests/test_gate_seal.py index 9b69639..a9986d2 100644 --- a/tests/test_gate_seal.py +++ b/tests/test_gate_seal.py @@ -428,3 +428,46 @@ def test_cannot_seal_twice_in_one_tx(gate_seal, sealables, sealing_committee): gate_seal.seal(sealables, sender=sealing_committee) with reverts("gate seal: expired"): gate_seal.seal(sealables, sender=sealing_committee) + + +def test_raw_call_response_should_be_false_when_sealable_reverts_on_pause(project, deployer, generate_sealables, sealing_committee, seal_duration_seconds, expiry_timestamp): + """ + `raw_call` without `max_outsize` and with `revert_on_failure=True` for some reason returns the boolean value of storage[0] :^) + + Which is why we specify `max_outsize=32`, even though don't actually use it. + + To test that `success` returns actual value instead of returning bool of storage[0], + we need to pause the contract before the sealing, + so that the condition `success and is_paused()` is false (i.e `False and True`), see GateSeal.vy, line 174. + + For that, we use `__force_pause_for` on SealableMock to ignore any checks and forcefully pause the contract. + After calling this function, the SealableMock is paused but the call to `pauseFor` will still revert, + thus the returned `success` should be False, the condition fails and the call reverts altogether. + + Without `max_outsize=32`, the transaction would not revert. + """ + + unpausable = False + should_revert = True + # we'll use only 1 sealable + sealables = generate_sealables(1, unpausable, should_revert) + + # deploy GateSeal + gate_seal = project.GateSeal.deploy( + sealing_committee, + seal_duration_seconds, + sealables, + expiry_timestamp, + sender=deployer, + ) + + # making sure we have the right contract + assert gate_seal.get_sealables() == sealables + + # forcefully pause the sealable before sealing + sealables[0].__force_pause_for(seal_duration_seconds, sender=deployer) + assert sealables[0].isPaused(), "should be paused now" + + # seal() should revert because `raw_call` to sealable returns `success=False`, even though isPaused() is True. + with reverts("0"): + gate_seal.seal(sealables, sender=sealing_committee) \ No newline at end of file From f2b3ac216f86d48180ebc50b4ed4f1fe68059cf2 Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Fri, 14 Apr 2023 15:30:19 +0600 Subject: [PATCH 4/6] fix: rename --- tests/test_gate_seal.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_gate_seal.py b/tests/test_gate_seal.py index a9986d2..e616d82 100644 --- a/tests/test_gate_seal.py +++ b/tests/test_gate_seal.py @@ -430,7 +430,7 @@ def test_cannot_seal_twice_in_one_tx(gate_seal, sealables, sealing_committee): gate_seal.seal(sealables, sender=sealing_committee) -def test_raw_call_response_should_be_false_when_sealable_reverts_on_pause(project, deployer, generate_sealables, sealing_committee, seal_duration_seconds, expiry_timestamp): +def test_raw_call_success_should_be_false_when_sealable_reverts_on_pause(project, deployer, generate_sealables, sealing_committee, seal_duration_seconds, expiry_timestamp): """ `raw_call` without `max_outsize` and with `revert_on_failure=True` for some reason returns the boolean value of storage[0] :^) @@ -470,4 +470,4 @@ def test_raw_call_response_should_be_false_when_sealable_reverts_on_pause(projec # seal() should revert because `raw_call` to sealable returns `success=False`, even though isPaused() is True. with reverts("0"): - gate_seal.seal(sealables, sender=sealing_committee) \ No newline at end of file + gate_seal.seal(sealables, sender=sealing_committee) From 5b29f7fea9fd6c88d7dba35753d3962caea13e3e Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Fri, 14 Apr 2023 16:40:00 +0600 Subject: [PATCH 5/6] feat: explain not checking response --- contracts/GateSeal.vy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/GateSeal.vy b/contracts/GateSeal.vy index 51f1ba9..f7668c9 100644 --- a/contracts/GateSeal.vy +++ b/contracts/GateSeal.vy @@ -162,7 +162,8 @@ def seal(_sealables: DynArray[address, MAX_SEALABLES]): response: Bytes[32] = b"" # using `raw_call` to catch external revert and continue execution - # capturing `response` to keep the compiler from acting out, + # capturing `response` to keep the compiler from acting out but will not be checking it + # as different sealables may return different values if anything at all # for details, see https://docs.vyperlang.org/en/stable/built-in-functions.html#raw_call success, response = raw_call( sealable, From 2b1243b3cd37d142cf17a3141fac2b5ccf9601fa Mon Sep 17 00:00:00 2001 From: Azat Serikov Date: Fri, 14 Apr 2023 16:52:27 +0600 Subject: [PATCH 6/6] fix: update comment --- tests/test_gate_seal.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_gate_seal.py b/tests/test_gate_seal.py index e616d82..cfc755e 100644 --- a/tests/test_gate_seal.py +++ b/tests/test_gate_seal.py @@ -432,13 +432,13 @@ def test_cannot_seal_twice_in_one_tx(gate_seal, sealables, sealing_committee): def test_raw_call_success_should_be_false_when_sealable_reverts_on_pause(project, deployer, generate_sealables, sealing_committee, seal_duration_seconds, expiry_timestamp): """ - `raw_call` without `max_outsize` and with `revert_on_failure=True` for some reason returns the boolean value of storage[0] :^) + `raw_call` without `max_outsize` and with `revert_on_failure=True` for some reason returns the boolean value of memory[0] :^) Which is why we specify `max_outsize=32`, even though don't actually use it. - To test that `success` returns actual value instead of returning bool of storage[0], + To test that `success` returns actual value instead of returning bool of memory[0], we need to pause the contract before the sealing, - so that the condition `success and is_paused()` is false (i.e `False and True`), see GateSeal.vy, line 174. + so that the condition `success and is_paused()` is false (i.e `False and True`), see GateSeal.vy::seal() For that, we use `__force_pause_for` on SealableMock to ignore any checks and forcefully pause the contract. After calling this function, the SealableMock is paused but the call to `pauseFor` will still revert,