From 6f0e1503b28793642c6d4a446cc356fe1ef1859b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 14:48:43 -0500 Subject: [PATCH] Strategies must be explicitly provided by the user, if not, no strategies will be used by default. This way users totally control strategies used and their parameterized values instead of implicit defaults which have repercussions eg. cost for speed up strategy. Update tests accordingly. --- atxm/machine.py | 20 +++------ tests/conftest.py | 15 ++++++- tests/test_machine.py | 96 +++++++++++++++++++++++++++++++++++++------ 3 files changed, 103 insertions(+), 28 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 738e741..4a612f1 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -1,5 +1,5 @@ from copy import copy, deepcopy -from typing import List, Optional, Type +from typing import List, Optional from eth_account.signers.local import LocalAccount from eth_utils import ValidationError @@ -17,11 +17,7 @@ TransactionFaulted, TransactionReverted, ) -from atxm.strategies import ( - AsyncTxStrategy, - ExponentialSpeedupStrategy, - TimeoutStrategy, -) +from atxm.strategies import AsyncTxStrategy from atxm.tracker import _TxTracker from atxm.tx import ( AsyncTx, @@ -96,11 +92,6 @@ class _Machine(StateMachine): _BLOCK_INTERVAL = 20 # ~20 blocks _BLOCK_SAMPLE_SIZE = 10_000 # blocks - STRATEGIES: List[Type[AsyncTxStrategy]] = [ - TimeoutStrategy, - ExponentialSpeedupStrategy, - ] - # max requeues/retries _MAX_REDO_ATTEMPTS = 3 @@ -124,7 +115,7 @@ def __init__( self.w3 = w3 self.signers = {} self.log = log - self._strategies = [s(w3) for s in self.STRATEGIES] + self._strategies = list() if strategies: self._strategies.extend(list(strategies)) @@ -348,8 +339,9 @@ def __strategize(self) -> None: if not params_updated: # TODO is this a potential forever wait - this is really controlled by strategies - # who can no longer do anything. if we limit the wait here then the TimeoutStrategy - # becomes useless - something to think about. #14 + # who can no longer do anything OR if there are no strategies defined. Something + # to think about. If we simply limit retry here, then what does that mean if there is + # a TimeoutStrategy? #14 log.info( f"[wait] strategies made no suggested updates to " f"pending tx #{_active_copy.id} - skipping retry round" diff --git a/tests/conftest.py b/tests/conftest.py index ec51efc..15dd679 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ from atxm import AutomaticTxMachine from atxm.logging import log +from atxm.strategies import ExponentialSpeedupStrategy, TimeoutStrategy observer = textFileLogObserver(sys.stdout) globalLogPublisher.addObserver(observer) @@ -61,9 +62,19 @@ def w3(networks): @pytest.fixture -def machine(w3): +def strategies(w3): + _strategy_classes = [ + TimeoutStrategy, + ExponentialSpeedupStrategy, + ] + _strategies = [s(w3) for s in _strategy_classes] + return _strategies + + +@pytest.fixture +def machine(w3, strategies): clock = Clock() - _machine = AutomaticTxMachine(w3=w3) + _machine = AutomaticTxMachine(w3=w3, strategies=strategies) _machine._task.clock = clock yield _machine diff --git a/tests/test_machine.py b/tests/test_machine.py index 9e4aa49..8a41933 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -1,4 +1,5 @@ import math +from typing import List, Optional import pytest @@ -14,6 +15,7 @@ Web3Exception, ) +from atxm import AutomaticTxMachine from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx from atxm.utils import _is_recoverable_send_tx_error @@ -820,16 +822,13 @@ def test_use_strategies_that_dont_make_updates( mocker, mock_wake_sleep, ): - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.execute.return_value = None strategy_2 = mocker.Mock(spec=AsyncTxStrategy) strategy_2.execute.return_value = None - machine._strategies = [strategy_1, strategy_2] + _configure_machine_strategies(machine, [strategy_1, strategy_2]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -889,6 +888,77 @@ def test_use_strategies_that_dont_make_updates( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +def test_dont_use_any_strategies( + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # strategies that don't make updates + _configure_machine_strategies(machine, None) + + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + + machine.start() + assert machine.current_state == machine._IDLE + + broadcast_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, signer=account, on_broadcast=broadcast_hook + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + broadcast_hook.assert_called_with(atx), "called with correct parameter" + + original_params = dict(atx.params) + + assert machine.current_state == machine._BUSY + + # need some cycles while tx unmined for strategies to kick in + num_cycles = 4 + for i in range(num_cycles): + yield clock.advance(1) + # params remained unchanged since strategies don't make updates + assert atx.params == original_params, "params remain unchanged" + + assert atx.params == original_params, "params remain unchanged" + assert update_spy.call_count == 0, "update never called because no retry" + + # mine tx + ethereum_tester.mine_block() + yield clock.advance(1) + + # ensure switch back to IDLE + yield clock.advance(1) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + assert atx.final + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") @pytest.mark.parametrize( @@ -917,16 +987,13 @@ def test_retry_with_errors_but_recovers( # need more freedom with redo attempts for test mocker.patch.object(machine, "_MAX_REDO_ATTEMPTS", 10) - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.name = "mock_strategy" # return non-None so retry is attempted strategy_1.execute.return_value = dict(eip1559_transaction) - machine._strategies = [strategy_1] + _configure_machine_strategies(machine, [strategy_1]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -1028,16 +1095,13 @@ def test_retry_with_errors_retries_exceeded( mocker, mock_wake_sleep, ): - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.name = "mock_strategy" # return non-None so retry is attempted strategy_1.execute.return_value = dict(eip1559_transaction) - machine._strategies = [strategy_1] + _configure_machine_strategies(machine, [strategy_1]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -1251,3 +1315,11 @@ def test_simple_state_transitions( assert machine.current_state == machine._IDLE assert wake.call_count == wake_call_count, "wake call count remains unchanged" assert sleep.call_count == sleep_call_count, "wake call count remains unchanged" + + +def _configure_machine_strategies( + machine: AutomaticTxMachine, strategies: Optional[List[AsyncTxStrategy]] = None +): + machine._strategies.clear() + if strategies: + machine._strategies.extend(strategies)