diff --git a/src/safe_cli/ethereum_client.py b/src/safe_cli/ethereum_client.py new file mode 100644 index 00000000..3c3f2da6 --- /dev/null +++ b/src/safe_cli/ethereum_client.py @@ -0,0 +1,35 @@ +from safe_eth.eth import EthereumClient, TxSpeed +from web3 import Web3 +from web3.types import TxParams, Wei + +# safe-cli refuses a cap above this many times the node's gas price. A cap that high is a typo more often than a +# choice: extra zeros, or wei typed as gwei. +MAX_FEE_PER_GAS_LIMIT = 10 + + +class SafeCliEthereumClient(EthereumClient): + """ + ``EthereumClient`` that sends EIP-1559 transactions with the ``maxFeePerGas`` the user gave in + ``--max-fee-per-gas``. Without the option it behaves as ``EthereumClient``. + """ + + # The user's cap in wei, or None + max_fee_per_gas: int | None = None + + def set_eip1559_fees( + self, tx: TxParams, tx_speed: TxSpeed = TxSpeed.NORMAL + ) -> TxParams: + tx = super().set_eip1559_fees(tx, tx_speed) + if self.max_fee_per_gas is None: + return tx + node_price = self.w3.eth.gas_price + if node_price and self.max_fee_per_gas > MAX_FEE_PER_GAS_LIMIT * node_price: + raise ValueError( + f"--max-fee-per-gas: {Web3.from_wei(self.max_fee_per_gas, 'gwei')} gwei is more than " + f"{MAX_FEE_PER_GAS_LIMIT} times the node's gas price ({Web3.from_wei(node_price, 'gwei')} gwei). " + "safe-cli sent nothing." + ) + tx["maxFeePerGas"] = Wei(self.max_fee_per_gas) + # A node rejects a tip above the cap + tx["maxPriorityFeePerGas"] = min(tx["maxPriorityFeePerGas"], tx["maxFeePerGas"]) + return tx diff --git a/src/safe_cli/main.py b/src/safe_cli/main.py index 84cbeb54..edb681e7 100644 --- a/src/safe_cli/main.py +++ b/src/safe_cli/main.py @@ -15,6 +15,7 @@ from . import VERSION from .argparse_validators import check_hex_str +from .ethereum_client import SafeCliEthereumClient from .operators import SafeOperator from .safe_cli import SafeCli from .tx_builder.exceptions import SoliditySyntaxError, TxBuilderEncodingError @@ -29,6 +30,31 @@ app = typer.Typer(name="Safe CLI") +MAX_FEE_PER_GAS_OPTION = "--max-fee-per-gas" + + +@app.callback() +def global_options( + max_fee_per_gas: Annotated[ + float | None, + typer.Option( + MAX_FEE_PER_GAS_OPTION, + envvar="SAFE_CLI_MAX_FEE_PER_GAS", + help="maxFeePerGas in gwei for each transaction safe-cli sends. Without it safe-cli estimates the " + "fee from the fee history. Limit: 10 times the node's gas price. Put it before the command or the " + "Safe address.", + show_default=False, + ), + ] = None, +): + if max_fee_per_gas is not None and not max_fee_per_gas > 0: + raise typer.BadParameter( + "expected a positive number of gwei", param_hint=MAX_FEE_PER_GAS_OPTION + ) + SafeCliEthereumClient.max_fee_per_gas = ( + None if max_fee_per_gas is None else Web3.to_wei(max_fee_per_gas, "gwei") + ) + def _build_safe_operator_and_load_keys( safe_address: ChecksumAddress, @@ -401,7 +427,11 @@ def _is_safe_cli_default_command(arguments: list[str]) -> bool: def main(): + # --max-fee-per-gas and its value come before the command + first = 1 + if len(sys.argv) > 1 and sys.argv[1].startswith(MAX_FEE_PER_GAS_OPTION): + first = 2 if "=" in sys.argv[1] else 3 # By default, the attended mode is initialised. Otherwise, the required command must be specified. - if _is_safe_cli_default_command(sys.argv): - sys.argv.insert(1, "attended-mode") + if _is_safe_cli_default_command(sys.argv[:1] + sys.argv[first:]): + sys.argv.insert(first, "attended-mode") app() diff --git a/src/safe_cli/operators/safe_operator.py b/src/safe_cli/operators/safe_operator.py index 0c94779b..f3e20699 100644 --- a/src/safe_cli/operators/safe_operator.py +++ b/src/safe_cli/operators/safe_operator.py @@ -38,6 +38,7 @@ from web3.contract import Contract from web3.exceptions import BadFunctionCallOutput +from safe_cli.ethereum_client import SafeCliEthereumClient from safe_cli.ethereum_hd_wallet import get_account_from_words from safe_cli.operators.exceptions import ( AccountNotLoadedException, @@ -162,7 +163,7 @@ def __init__( ): self.address = address self.node_url = node_url - self.ethereum_client = EthereumClient(self.node_url) + self.ethereum_client = SafeCliEthereumClient(self.node_url) self.ens = ENS.from_web3(self.ethereum_client.w3) self.network: EthereumNetwork = self.ethereum_client.get_network() try: diff --git a/tests/test_ethereum_client.py b/tests/test_ethereum_client.py new file mode 100644 index 00000000..0be179c5 --- /dev/null +++ b/tests/test_ethereum_client.py @@ -0,0 +1,184 @@ +import unittest +from unittest import mock + +from eth_account import Account +from safe_eth.eth import EthereumClient +from typer.testing import CliRunner +from web3 import Web3 + +from safe_cli.ethereum_client import MAX_FEE_PER_GAS_LIMIT, SafeCliEthereumClient +from safe_cli.main import app, main +from safe_cli.operators.hw_wallets.hw_wallet_manager import HwWalletManager + +from .safe_cli_test_case_mixin import SafeCliTestCaseMixin + +GWEI = 10**9 +runner = CliRunner() + + +class TestSafeCliEthereumClient(SafeCliTestCaseMixin, unittest.TestCase): + def tearDown(self): + SafeCliEthereumClient.max_fee_per_gas = None + + def _fees(self, base_fee: int, tip: int, node_price: int) -> dict: + ethereum_client = SafeCliEthereumClient(self.ethereum_node_url) + with ( + mock.patch.object( + EthereumClient, "estimate_fee_eip1559", return_value=(base_fee, tip) + ), + mock.patch.object( + type(ethereum_client.w3.eth), + "gas_price", + new_callable=mock.PropertyMock, + return_value=node_price, + ), + ): + return ethereum_client.set_eip1559_fees({"gasPrice": 1}) + + def test_without_the_option_nothing_changes(self): + fees = self._fees(20 * GWEI, 2 * GWEI, 25 * GWEI) + self.assertEqual(fees["maxFeePerGas"], 22 * GWEI) + self.assertEqual(fees["maxPriorityFeePerGas"], 2 * GWEI) + + def test_max_fee_per_gas(self): + SafeCliEthereumClient.max_fee_per_gas = 715_000 * GWEI + # A node that forecasts 625,625 gwei for the next block and charges 715,000 + fees = self._fees(625_625 * GWEI, 0, 715_000 * GWEI) + self.assertEqual(fees["maxFeePerGas"], 715_000 * GWEI) + self.assertEqual(fees["maxPriorityFeePerGas"], 0) + self.assertNotIn("gasPrice", fees) + + # A cap below the estimated tip lowers the tip to the cap + SafeCliEthereumClient.max_fee_per_gas = 1 * GWEI + fees = self._fees(20 * GWEI, 2 * GWEI, 22 * GWEI) + self.assertEqual(fees["maxFeePerGas"], 1 * GWEI) + self.assertEqual(fees["maxPriorityFeePerGas"], 1 * GWEI) + + def test_limit(self): + SafeCliEthereumClient.max_fee_per_gas = MAX_FEE_PER_GAS_LIMIT * 22 * GWEI + fees = self._fees(20 * GWEI, 2 * GWEI, 22 * GWEI) + self.assertEqual(fees["maxFeePerGas"], MAX_FEE_PER_GAS_LIMIT * 22 * GWEI) + + SafeCliEthereumClient.max_fee_per_gas += 1 + with self.assertRaisesRegex(ValueError, "more than 10 times"): + self._fees(20 * GWEI, 2 * GWEI, 22 * GWEI) + + # Some development chains report a gas price of 0: no limit applies + self._fees(20 * GWEI, 2 * GWEI, 0) + + def test_command_line_option(self): + result = runner.invoke(app, ["--max-fee-per-gas", "31.5", "version"]) + self.assertEqual(result.exit_code, 0) + self.assertEqual(SafeCliEthereumClient.max_fee_per_gas, 31_500_000_000) + + result = runner.invoke(app, ["version"]) + self.assertEqual(result.exit_code, 0) + self.assertIsNone(SafeCliEthereumClient.max_fee_per_gas) + + for value in ("0", "-1", "nan", "fast"): + result = runner.invoke(app, [f"--max-fee-per-gas={value}", "version"]) + self.assertEqual(result.exit_code, 2, value) + + with mock.patch.dict("os.environ", {"SAFE_CLI_MAX_FEE_PER_GAS": "40"}): + runner.invoke(app, ["version"]) + self.assertEqual(SafeCliEthereumClient.max_fee_per_gas, 40 * GWEI) + # The command line overrides the environment + runner.invoke(app, ["--max-fee-per-gas=41", "version"]) + self.assertEqual(SafeCliEthereumClient.max_fee_per_gas, 41 * GWEI) + + @mock.patch.object(type(app), "__call__") + def test_attended_mode_after_the_option(self, mock_app: mock.MagicMock): + safe, node = Account.create().address, "http://localhost:8545" + for arguments, expected in ( + ([safe, node], ["attended-mode", safe, node]), + ( + ["--max-fee-per-gas", "30", safe, node], + ["--max-fee-per-gas", "30", "attended-mode", safe, node], + ), + ( + ["--max-fee-per-gas=30", safe, node], + ["--max-fee-per-gas=30", "attended-mode", safe, node], + ), + ( + ["--max-fee-per-gas", "30", "send-ether", safe, node], + ["--max-fee-per-gas", "30", "send-ether", safe, node], + ), + ): + with mock.patch("sys.argv", ["safe-cli", *arguments]) as argv: + main() + self.assertEqual(argv[1:], expected) + + def _sent_to(self, address: str, since_block: int) -> list: + return [ + transaction + for number in range(since_block + 1, self.w3.eth.block_number + 1) + for transaction in self.w3.eth.get_block(number, True)["transactions"] + if transaction["to"] == address + ] + + def test_every_way_of_sending_uses_it(self): + safe_operator = self.setup_operator() + owner = self.ethereum_test_account + safe_operator.accounts.add(owner) + safe_operator.default_sender = owner + self.w3.eth.send_transaction( + {"from": owner.address, "to": safe_operator.address, "value": 10} + ) + # The fee history forecasts 0.6 x the base fee the chain charges, so the node refuses the estimate + forecast = self.w3.eth.get_block("latest")["baseFeePerGas"] * 6 // 10 + cap = self.w3.eth.gas_price * 3 + SafeCliEthereumClient.max_fee_per_gas = cap + with mock.patch.object( + EthereumClient, "estimate_fee_eip1559", return_value=(forecast, 0) + ): + # Executing a Safe transaction with a local key + block = self.w3.eth.block_number + self.assertTrue(safe_operator.send_ether(Account.create().address, 1)) + (transaction,) = self._sent_to(safe_operator.address, block) + self.assertEqual( + (transaction["type"], transaction["maxFeePerGas"]), (2, cap) + ) + + # approve_hash + block = self.w3.eth.block_number + self.assertTrue( + safe_operator.approve_hash(Web3.keccak(text="fee cap"), owner.address) + ) + (transaction,) = self._sent_to(safe_operator.address, block) + self.assertEqual( + (transaction["type"], transaction["maxFeePerGas"]), (2, cap) + ) + + # Executing a Safe transaction with a hardware wallet + sender = mock.Mock() + sender.address = owner.address + sender.get_signed_raw_transaction.side_effect = lambda tx, chain_id: ( + self.w3.eth.account.sign_transaction(tx, owner.key).raw_transaction + ) + safe_operator.default_sender = None + safe_operator.hw_wallet_manager = HwWalletManager() + safe_operator.hw_wallet_manager.sender = sender + block = self.w3.eth.block_number + self.assertTrue(safe_operator.send_ether(Account.create().address, 1)) + (transaction,) = self._sent_to(safe_operator.address, block) + self.assertEqual( + (transaction["type"], transaction["maxFeePerGas"]), (2, cap) + ) + + # Without the option the node refuses: the wording differs between Ganache and anvil + SafeCliEthereumClient.max_fee_per_gas = None + safe_operator.default_sender = owner + forecast = self.w3.eth.get_block("latest")["baseFeePerGas"] * 6 // 10 + with ( + mock.patch.object( + EthereumClient, "estimate_fee_eip1559", return_value=(forecast, 0) + ), + self.assertRaisesRegex( + Exception, r"(?i)max ?fee ?per ?gas.*less than.*base ?fee" + ), + ): + safe_operator.send_ether(Account.create().address, 1) + + +if __name__ == "__main__": + unittest.main()