diff --git a/autonomy/chain/tx.py b/autonomy/chain/tx.py index 2e63b9c44b..c6f1c6644d 100644 --- a/autonomy/chain/tx.py +++ b/autonomy/chain/tx.py @@ -48,6 +48,7 @@ DEFAULT_ON_CHAIN_INTERACT_TIMEOUT = 60.0 DEFAULT_ON_CHAIN_INTERACT_RETRIES = 5 DEFAULT_ON_CHAIN_INTERACT_SLEEP = 3.0 +DEFAULT_GAS_PRICE_MULTIPLIER = 1.0 DEFAULT_GAS_ESTIMATE_MULTIPLIER = 1.0 DEFAULT_MISSING_EVENT_EXCEPTION = Exception( "Could not verify transaction. Event not found." @@ -113,6 +114,7 @@ def __init__( timeout: Optional[float] = None, retries: Optional[int] = None, sleep: Optional[float] = None, + gas_price_multiplier: Optional[float] = None, gas_estimate_multiplier: Optional[float] = None, ) -> None: """Initialize object.""" @@ -124,13 +126,22 @@ def __init__( self.retries = retries or DEFAULT_ON_CHAIN_INTERACT_RETRIES self.sleep = sleep or DEFAULT_ON_CHAIN_INTERACT_SLEEP + if gas_price_multiplier is not None: + if gas_price_multiplier > 1.5: + logger.warning(f"{gas_price_multiplier=} is unusually high.") + if gas_price_multiplier <= 0: + raise ValueError(f"{gas_price_multiplier=} must be positive.") + if gas_estimate_multiplier is not None: - if gas_estimate_multiplier > 2.5: + if gas_estimate_multiplier > 1.5: logger.warning(f"{gas_estimate_multiplier=} is unusually high.") if gas_estimate_multiplier <= 0: raise ValueError(f"{gas_estimate_multiplier=} must be positive.") - self.gas_multiplier = gas_estimate_multiplier or DEFAULT_GAS_ESTIMATE_MULTIPLIER + self.gas_price_multiplier = gas_price_multiplier or DEFAULT_GAS_PRICE_MULTIPLIER + self.gas_estimate_multiplier = ( + gas_estimate_multiplier or DEFAULT_GAS_ESTIMATE_MULTIPLIER + ) def _get_preferred_gas_price_strategy(self, tx_dict: dict) -> str | None: """Get the preferred gas price strategy based on tx_dict fields.""" @@ -142,30 +153,44 @@ def _get_preferred_gas_price_strategy(self, tx_dict: dict) -> str | None: return None - def _reprice(self, tx_dict: Dict) -> Optional[Dict]: - """Reprice transaction.""" - gas_price_strategy = self._get_preferred_gas_price_strategy(tx_dict) - if gas_price_strategy == "eip1559": - old_price = { - "maxFeePerGas": tx_dict["maxFeePerGas"], - "maxPriorityFeePerGas": tx_dict["maxPriorityFeePerGas"], - } - elif gas_price_strategy == "gas_station": - old_price = {"gasPrice": tx_dict["gasPrice"]} - else: - # This means something went wrong when building the transaction - # returning a None value to the main loop will tell the main loop - # to rebuild the transaction - return None - - tx_dict.update( - self.ledger_api.try_get_gas_pricing( - gas_price_strategy=gas_price_strategy, - old_price=old_price, + def _apply_gas_multipliers(self, tx_dict: Dict) -> Dict: + """Apply gas price and estimate multipliers separately.""" + # Apply gas price multiplier to EIP-1559 prices + if "maxFeePerGas" in tx_dict: + tx_dict["maxFeePerGas"] = int( + tx_dict["maxFeePerGas"] * self.gas_price_multiplier ) - ) + if "maxPriorityFeePerGas" in tx_dict: + tx_dict["maxPriorityFeePerGas"] = int( + tx_dict["maxPriorityFeePerGas"] * self.gas_price_multiplier + ) + # Apply gas price multiplier to legacy gas price + if "gasPrice" in tx_dict: + tx_dict["gasPrice"] = int(tx_dict["gasPrice"] * self.gas_price_multiplier) + + # Apply gas estimate multiplier + gas_estimated = int(tx_dict.get("gas", 0)) + tx_dict["gas"] = math.ceil(gas_estimated * self.gas_estimate_multiplier) + return tx_dict + def _update_gas_and_price(self, tx_dict: Dict) -> Dict: + """Update gas price and estimate, then apply multipliers.""" + # Get gas price + gas_price = self.ledger_api.try_get_gas_pricing( + gas_price_strategy=self._get_preferred_gas_price_strategy(tx_dict), + ) + if gas_price is not None: + tx_dict.update(gas_price) + + # Get gas estimate + tx_dict = self.ledger_api.update_with_gas_estimate( + {**tx_dict, "gas": tx_dict.get("gas", 0)} + ) + + # Apply both gas price and estimate multipliers + return self._apply_gas_multipliers(tx_dict) + def transact(self, dry_run: bool = False) -> "TxSettler": """Make a transaction and return a receipt""" retries = 0 @@ -180,21 +205,8 @@ def transact(self, dry_run: bool = False) -> "TxSettler": if dry_run: # Return with only the transaction dict on dry-run return self - gas_price = self.ledger_api.try_get_gas_pricing( - gas_price_strategy=self._get_preferred_gas_price_strategy( - self.tx_dict - ), - ) - if gas_price is not None: - self.tx_dict.update(gas_price) - self.tx_dict = self.ledger_api.update_with_gas_estimate( - { - **self.tx_dict, - "gas": self.tx_dict.get("gas", 0), - } - ) - gas_estimated = int(self.tx_dict.get("gas", 0)) - self.tx_dict["gas"] = math.ceil(gas_estimated * self.gas_multiplier) + # Update gas pricing and estimate with multipliers applied + self.tx_dict = self._update_gas_and_price(self.tx_dict) tx_signed = self.crypto.sign_transaction(transaction=self.tx_dict) self.tx_hash = self.ledger_api.send_signed_transaction( tx_signed=tx_signed, @@ -217,7 +229,7 @@ def transact(self, dry_run: bool = False) -> "TxSettler": if should_reprice(error): logger.warning(f"Low gas error: {e}; Repricing the transaction...") self.tx_hash = None - self.tx_dict = self._reprice(self.tx_dict or {}) # type: ignore + time.sleep(self.sleep) continue self.tx_dict = None diff --git a/docs/api/chain/tx.md b/docs/api/chain/tx.md index a3223596da..ecae77889d 100644 --- a/docs/api/chain/tx.md +++ b/docs/api/chain/tx.md @@ -56,6 +56,7 @@ def __init__(ledger_api: LedgerApi, timeout: Optional[float] = None, retries: Optional[int] = None, sleep: Optional[float] = None, + gas_price_multiplier: Optional[float] = None, gas_estimate_multiplier: Optional[float] = None) -> None ``` diff --git a/tests/test_autonomy/test_chain/test_tx.py b/tests/test_autonomy/test_chain/test_tx.py index ebf67c710f..65abbbe1c1 100644 --- a/tests/test_autonomy/test_chain/test_tx.py +++ b/tests/test_autonomy/test_chain/test_tx.py @@ -21,7 +21,7 @@ from logging import Logger from time import sleep -from typing import Any, Callable +from typing import Any, Callable, Dict from unittest import mock import pytest @@ -600,94 +600,299 @@ def _update_with_gas_estimate_mock(tx: dict) -> dict: in mock_logger.warning.call_args[0][0] ) assert state["try_update_with_gas_estimate_call_count"] == 2 - assert state["try_get_gas_pricing_call_count"] == 3 + assert state["try_get_gas_pricing_call_count"] == 2 assert settler.tx_hash is not None settler.settle() assert settler.tx_receipt is not None -def test_gas_estimate_multiplier() -> None: - """Test gas_estimate_multiplier parameter.""" +class TestGasMultipliers: + """Test TxSettler gas multipliers functionality.""" - def _tx_builder() -> dict: - return { - "from": "0x123", - "to": "0x456", - "value": 100, - "gas": 100, - } + def test_default_gas_multipliers(self) -> None: + """Test default gas multipliers are set correctly.""" + settler = TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + ) + assert settler.gas_price_multiplier == 1.0 + assert settler.gas_estimate_multiplier == 1.0 - # Test default multiplier (should be 1.0) - settler = TxSettler( - ledger_api=mock.Mock( - try_get_gas_pricing=lambda **kwargs: { - "maxFeePerGas": 100, - "maxPriorityFeePerGas": 100, - }, - update_with_gas_estimate=lambda tx: {**tx, "gas": 100}, - ), - crypto=mock.Mock(sign_transaction=lambda transaction: transaction), - chain_type=ChainType.LOCAL, - tx_builder=_tx_builder, - ) - assert settler.gas_multiplier == 1.0 + def test_custom_gas_price_multiplier(self) -> None: + """Test custom gas price multiplier initialization.""" + settler = TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + gas_price_multiplier=1.5, + ) + assert settler.gas_price_multiplier == 1.5 + assert settler.gas_estimate_multiplier == 1.0 - # Test custom multiplier - settler_with_multiplier = TxSettler( - ledger_api=mock.Mock( - try_get_gas_pricing=lambda **kwargs: { - "maxFeePerGas": 100, - "maxPriorityFeePerGas": 100, - }, - update_with_gas_estimate=lambda tx: {**tx, "gas": 100}, - ), - crypto=mock.Mock(sign_transaction=lambda transaction: transaction), - chain_type=ChainType.LOCAL, - tx_builder=_tx_builder, - gas_estimate_multiplier=1.5, - ) - assert settler_with_multiplier.gas_multiplier == 1.5 + def test_custom_gas_estimate_multiplier(self) -> None: + """Test custom gas estimate multiplier initialization.""" + settler = TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + gas_estimate_multiplier=1.5, + ) + assert settler.gas_price_multiplier == 1.0 + assert settler.gas_estimate_multiplier == 1.5 - # Test that gas is multiplied correctly in transaction - settler_with_multiplier.transact(dry_run=False) - assert settler_with_multiplier.tx_dict is not None - assert settler_with_multiplier.tx_dict["gas"] == 150 # 100 * 1.5 + def test_custom_both_multipliers(self) -> None: + """Test custom both gas multipliers initialization.""" + settler = TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + gas_price_multiplier=1.2, + gas_estimate_multiplier=1.3, + ) + assert settler.gas_price_multiplier == 1.2 + assert settler.gas_estimate_multiplier == 1.3 + + def test_gas_price_multiplier_validation_negative(self) -> None: + """Test gas price multiplier must be positive (negative value).""" + with pytest.raises( + ValueError, match="gas_price_multiplier=.* must be positive" + ): + TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + gas_price_multiplier=-0.5, + ) + + def test_gas_estimate_multiplier_validation_negative(self) -> None: + """Test gas estimate multiplier must be positive (negative value).""" + with pytest.raises( + ValueError, match="gas_estimate_multiplier=.* must be positive" + ): + TxSettler( + ledger_api=mock.Mock(), + crypto=mock.Mock(), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {}, + gas_estimate_multiplier=-0.5, + ) - # Test invalid multiplier (must be positive) - with pytest.raises(ValueError, match="gas_estimate_multiplier.*must be positive"): + @mock.patch("autonomy.chain.tx.logger") + def test_gas_price_multiplier_warning_unusually_high( + self, mock_logger: mock.Mock + ) -> None: + """Test warning when gas price multiplier is unusually high.""" TxSettler( ledger_api=mock.Mock(), crypto=mock.Mock(), chain_type=ChainType.LOCAL, - tx_builder=_tx_builder, - gas_estimate_multiplier=0, + tx_builder=lambda: {}, + gas_price_multiplier=1.6, ) + mock_logger.warning.assert_called_once() + assert "gas_price_multiplier=" in mock_logger.warning.call_args[0][0] + assert "unusually high" in mock_logger.warning.call_args[0][0] - with pytest.raises(ValueError, match="gas_estimate_multiplier.*must be positive"): + @mock.patch("autonomy.chain.tx.logger") + def test_gas_estimate_multiplier_warning_unusually_high( + self, mock_logger: mock.Mock + ) -> None: + """Test warning when gas estimate multiplier is unusually high.""" TxSettler( ledger_api=mock.Mock(), crypto=mock.Mock(), chain_type=ChainType.LOCAL, - tx_builder=_tx_builder, - gas_estimate_multiplier=-1.0, + tx_builder=lambda: {}, + gas_estimate_multiplier=1.6, + ) + mock_logger.warning.assert_called_once() + assert "gas_estimate_multiplier=" in mock_logger.warning.call_args[0][0] + assert "unusually high" in mock_logger.warning.call_args[0][0] + + def test_gas_multipliers_applied_eip1559_on_initial_transact(self) -> None: + """Test gas multipliers are applied to EIP-1559 prices on initial transact.""" + settler = TxSettler( + ledger_api=mock.Mock( + try_get_gas_pricing=lambda **kwargs: { + "maxFeePerGas": 100, + "maxPriorityFeePerGas": 50, + }, + update_with_gas_estimate=lambda tx: {**tx, "gas": 21000}, + send_signed_transaction=mock.Mock(return_value="0x123"), + api=mock.Mock( + eth=mock.Mock( + block_number=1, + wait_for_transaction_receipt=mock.Mock( + return_value=mock.Mock(blockNumber=1) + ), + ) + ), + ), + crypto=mock.Mock(sign_transaction=lambda transaction: transaction), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {"from": "0x123"}, + gas_price_multiplier=1.5, + gas_estimate_multiplier=1.2, ) + settler.transact() + # Verify multipliers were applied + assert settler.tx_dict is not None + assert settler.tx_dict["maxFeePerGas"] == 150 # 100 * 1.5 + assert settler.tx_dict["maxPriorityFeePerGas"] == 75 # 50 * 1.5 + assert settler.tx_dict["gas"] == 25200 # ceil(21000 * 1.2) -@mock.patch("autonomy.chain.tx.logger") -def test_gas_estimate_multiplier_warning(logger: mock.Mock) -> None: - """Test that gas_estimate_multiplier warns when > 2.5.""" + def test_gas_multipliers_applied_legacy_on_initial_transact(self) -> None: + """Test gas multipliers are applied to legacy gasPrice on initial transact.""" + settler = TxSettler( + ledger_api=mock.Mock( + try_get_gas_pricing=lambda **kwargs: {"gasPrice": 100}, + update_with_gas_estimate=lambda tx: {**tx, "gas": 21000}, + send_signed_transaction=mock.Mock(return_value="0x123"), + api=mock.Mock( + eth=mock.Mock( + block_number=1, + wait_for_transaction_receipt=mock.Mock( + return_value=mock.Mock(blockNumber=1) + ), + ) + ), + ), + crypto=mock.Mock(sign_transaction=lambda transaction: transaction), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {"from": "0x123"}, + gas_price_multiplier=2.0, + gas_estimate_multiplier=1.5, + ) + settler.transact() - def _tx_builder() -> dict: - return {"from": "0x123", "to": "0x456", "value": 100} + assert settler.tx_dict is not None + assert settler.tx_dict["gasPrice"] == 200 # 100 * 2.0 + assert settler.tx_dict["gas"] == 31500 # ceil(21000 * 1.5) - TxSettler( - ledger_api=mock.Mock(), - crypto=mock.Mock(), - chain_type=ChainType.LOCAL, - tx_builder=_tx_builder, - gas_estimate_multiplier=3.0, - ) + def test_gas_multipliers_applied_on_reprice(self) -> None: + """Test gas price multiplier is applied when repricing transaction.""" + call_count = {"reprice": 0, "estimate": 0} + + def mock_try_get_gas_pricing(**kwargs: Any) -> Dict[str, int]: + call_count["reprice"] += 1 + if call_count["reprice"] == 1: + # Initial pricing + return {"maxFeePerGas": 100, "maxPriorityFeePerGas": 50} + else: + # Repricing with bump + return {"maxFeePerGas": 150, "maxPriorityFeePerGas": 75} + + def mock_update_with_gas_estimate(tx: Dict[str, Any]) -> Dict[str, Any]: + call_count["estimate"] += 1 + return {**tx, "gas": 21000} + + def mock_send_signed_transaction(**kwargs: Any) -> str: + if call_count["estimate"] == 1: + raise ValueError("gas too low") + return "0x123" + + settler = TxSettler( + ledger_api=mock.Mock( + try_get_gas_pricing=mock_try_get_gas_pricing, + update_with_gas_estimate=mock_update_with_gas_estimate, + send_signed_transaction=mock_send_signed_transaction, + api=mock.Mock( + eth=mock.Mock( + block_number=1, + wait_for_transaction_receipt=mock.Mock( + return_value=mock.Mock(blockNumber=1) + ), + ) + ), + ), + crypto=mock.Mock(sign_transaction=lambda transaction: transaction), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {"from": "0x123"}, + gas_price_multiplier=1.5, + gas_estimate_multiplier=1.2, + ) + settler.transact() + + # After reprice with multiplier, prices should be bumped and multiplied + # Initial reprice: 150 * 1.5 = 225, 75 * 1.5 = 112.5 + # Then _update_gas_and_price is called again: same calculation since mock returns same bump + assert settler.tx_dict is not None + assert settler.tx_dict["maxFeePerGas"] == 225 # 150 * 1.5 + assert ( + settler.tx_dict["maxPriorityFeePerGas"] == 112 + ) # 75 * 1.5 (int truncates) + assert settler.tx_dict["gas"] == 25200 # ceil(21000 * 1.2) + + def test_gas_estimate_alone_multiplier(self) -> None: + """Test only gas estimate multiplier is applied without price multiplier.""" + settler = TxSettler( + ledger_api=mock.Mock( + try_get_gas_pricing=lambda **kwargs: { + "maxFeePerGas": 100, + "maxPriorityFeePerGas": 50, + }, + update_with_gas_estimate=lambda tx: {**tx, "gas": 21000}, + send_signed_transaction=mock.Mock(return_value="0x123"), + api=mock.Mock( + eth=mock.Mock( + block_number=1, + wait_for_transaction_receipt=mock.Mock( + return_value=mock.Mock(blockNumber=1) + ), + ) + ), + ), + crypto=mock.Mock(sign_transaction=lambda transaction: transaction), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {"from": "0x123"}, + gas_estimate_multiplier=2.0, + ) + settler.transact() + + # Price should not be multiplied (using default 1.0) + assert settler.tx_dict is not None + assert settler.tx_dict["maxFeePerGas"] == 100 + assert settler.tx_dict["maxPriorityFeePerGas"] == 50 + # Gas estimate should be multiplied + assert settler.tx_dict["gas"] == 42000 # ceil(21000 * 2.0) + + def test_gas_price_alone_multiplier(self) -> None: + """Test only gas price multiplier is applied without estimate multiplier.""" + settler = TxSettler( + ledger_api=mock.Mock( + try_get_gas_pricing=lambda **kwargs: { + "maxFeePerGas": 100, + "maxPriorityFeePerGas": 50, + }, + update_with_gas_estimate=lambda tx: {**tx, "gas": 21000}, + send_signed_transaction=mock.Mock(return_value="0x123"), + api=mock.Mock( + eth=mock.Mock( + block_number=1, + wait_for_transaction_receipt=mock.Mock( + return_value=mock.Mock(blockNumber=1) + ), + ) + ), + ), + crypto=mock.Mock(sign_transaction=lambda transaction: transaction), + chain_type=ChainType.LOCAL, + tx_builder=lambda: {"from": "0x123"}, + gas_price_multiplier=2.0, + ) + settler.transact() - logger.warning.assert_called_once() # type: ignore[attr-defined] - assert "unusually high" in logger.warning.call_args[0][0] # type: ignore[attr-defined] + # Price should be multiplied + assert settler.tx_dict is not None + assert settler.tx_dict["maxFeePerGas"] == 200 # 100 * 2.0 + assert settler.tx_dict["maxPriorityFeePerGas"] == 100 # 50 * 2.0 + # Gas estimate should not be multiplied (using default 1.0) + assert settler.tx_dict["gas"] == 21000