From 7ed6f6be185e9b55de544b9ea7985d70a1be63f3 Mon Sep 17 00:00:00 2001 From: primata Date: Sat, 25 Jul 2026 16:01:28 -0300 Subject: [PATCH 1/2] fix: Read transaction service API key from the environment at call time `TransactionServiceApi.__init__` exposed `SAFE_TRANSACTION_SERVICE_API_KEY` and `SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT` as default argument values. That has two consequences: 1. Default arguments are evaluated once, when the module is imported, so a value placed in the environment afterwards (a `.env` loader, a test fixture, a notebook) is never seen. 2. `SafeBaseAPI.from_ethereum_client` forwards `api_key=None` explicitly, and an explicit `None` overrides a default. Every caller that builds the client through `from_ethereum_client` therefore gets `api_key=None` regardless of the environment. `safe-cli` uses `from_ethereum_client`, so it sends no `Authorization` header and warns "you must set the following environment variable with your API key" even when the variable is set. Requests silently fall back to the unauthenticated rate limit. Resolving both values inside the constructor body fixes all three cases while keeping an explicitly passed argument authoritative. Note one intentional behaviour change: passing `api_key=None` while the environment variable is set now yields an authenticated client. That is what `from_ethereum_client` needs, and it matches how an optional argument usually reads, but it does mean `None` can no longer be used to force an anonymous client when the variable is present. Tests are added in a `SimpleTestCase`, since they need neither the database nor network access, and they fail on `main`: FAILED test_api_key_read_from_environment_at_call_time FAILED test_api_key_from_environment_with_explicit_none FAILED test_request_timeout_read_from_environment_at_call_time --- .../transaction_service_api.py | 18 +++-- .../tests/api/test_transaction_service_api.py | 78 ++++++++++++++++++- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/safe_eth/safe/api/transaction_service_api/transaction_service_api.py b/safe_eth/safe/api/transaction_service_api/transaction_service_api.py index a7cf0c8a8..959f18271 100644 --- a/safe_eth/safe/api/transaction_service_api/transaction_service_api.py +++ b/safe_eth/safe/api/transaction_service_api/transaction_service_api.py @@ -63,12 +63,20 @@ def __init__( network: EthereumNetwork, ethereum_client: Optional[EthereumClient] = None, base_url: Optional[str] = None, - api_key: Optional[str] = os.environ.get("SAFE_TRANSACTION_SERVICE_API_KEY"), - request_timeout: int = int( - os.environ.get("SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT", 10) - ), + api_key: Optional[str] = None, + request_timeout: Optional[int] = None, ): - super().__init__(network, ethereum_client, base_url, api_key, request_timeout) + # Resolved here rather than as default argument values, so that the + # environment is read at call time and an explicit `None` (as passed by + # `SafeBaseAPI.from_ethereum_client`) still falls back to it. + super().__init__( + network, + ethereum_client, + base_url, + api_key or os.environ.get("SAFE_TRANSACTION_SERVICE_API_KEY"), + request_timeout + or int(os.environ.get("SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT", 10)), + ) def _get_url_by_network(self, network: EthereumNetwork) -> Optional[str]: network_short_name = self.NETWORK_SHORTNAME.get(network) diff --git a/safe_eth/safe/tests/api/test_transaction_service_api.py b/safe_eth/safe/tests/api/test_transaction_service_api.py index cd9e53c7b..3d1318fb9 100644 --- a/safe_eth/safe/tests/api/test_transaction_service_api.py +++ b/safe_eth/safe/tests/api/test_transaction_service_api.py @@ -3,7 +3,7 @@ from unittest import mock from unittest.mock import MagicMock, PropertyMock, patch -from django.test import TestCase +from django.test import SimpleTestCase, TestCase import pytest from eth_account import Account @@ -354,3 +354,79 @@ def test_decode_data(self): "Cannot decode tx data:", str(context.exception), ) + + +class TestTransactionServiceApiApiKey(SimpleTestCase): + """ + The API key and request timeout are read from the environment. These tests + do not need network access or a real key, so they are kept out of + `TestTransactionServiceAPI`, which skips unless a key is configured. + """ + + ENV_API_KEY = "SAFE_TRANSACTION_SERVICE_API_KEY" + ENV_TIMEOUT = "SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT" + + @staticmethod + def _ethereum_client_mock() -> MagicMock: + ethereum_client = MagicMock(spec=EthereumClient) + ethereum_client.get_network.return_value = EthereumNetwork.MAINNET + return ethereum_client + + def test_api_key_read_from_environment_at_call_time(self): + # Set after the module was imported: a default argument value would + # have been evaluated at import time and would miss this. + with mock.patch.dict(os.environ, {self.ENV_API_KEY: "env-api-key"}, clear=True): + self.assertEqual( + TransactionServiceApi(EthereumNetwork.MAINNET).api_key, "env-api-key" + ) + + def test_api_key_from_environment_with_explicit_none(self): + # `SafeBaseAPI.from_ethereum_client` forwards `api_key=None`, which must + # not discard the environment value. + with mock.patch.dict(os.environ, {self.ENV_API_KEY: "env-api-key"}, clear=True): + self.assertEqual( + TransactionServiceApi(EthereumNetwork.MAINNET, api_key=None).api_key, + "env-api-key", + ) + self.assertEqual( + TransactionServiceApi.from_ethereum_client( + self._ethereum_client_mock() + ).api_key, + "env-api-key", + ) + + def test_explicit_api_key_takes_precedence(self): + with mock.patch.dict(os.environ, {self.ENV_API_KEY: "env-api-key"}, clear=True): + self.assertEqual( + TransactionServiceApi( + EthereumNetwork.MAINNET, api_key="explicit-api-key" + ).api_key, + "explicit-api-key", + ) + self.assertEqual( + TransactionServiceApi.from_ethereum_client( + self._ethereum_client_mock(), api_key="explicit-api-key" + ).api_key, + "explicit-api-key", + ) + + def test_api_key_absent(self): + with mock.patch.dict(os.environ, {}, clear=True): + self.assertIsNone(TransactionServiceApi(EthereumNetwork.MAINNET).api_key) + + def test_request_timeout_read_from_environment_at_call_time(self): + with mock.patch.dict(os.environ, {self.ENV_TIMEOUT: "42"}, clear=True): + self.assertEqual( + TransactionServiceApi(EthereumNetwork.MAINNET).request_timeout, 42 + ) + + with mock.patch.dict(os.environ, {}, clear=True): + self.assertEqual( + TransactionServiceApi(EthereumNetwork.MAINNET).request_timeout, 10 + ) + self.assertEqual( + TransactionServiceApi( + EthereumNetwork.MAINNET, request_timeout=5 + ).request_timeout, + 5, + ) From 10cca8fdc162cbaa3e960e3cee0ac210886a5e35 Mon Sep 17 00:00:00 2001 From: primata Date: Sun, 26 Jul 2026 17:45:01 -0300 Subject: [PATCH 2/2] fix: let an explicitly passed api_key or timeout win over the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: `api_key or os.environ.get(...)` also swallows falsy values a caller passed deliberately. `api_key=""` means "send no Authorization header" and `request_timeout=0` is a value, not an absence, but both fell through to the environment. Checking `is None` keeps the behaviour this PR is about — the explicit `None` that `SafeBaseAPI.from_ethereum_client` forwards still resolves from the environment — while leaving every other value alone. Also renames the test class, which had a duplicated "Api" segment, and covers both falsy cases. They fail on the previous revision: FAILED test_empty_api_key_forces_anonymous FAILED test_request_timeout_read_from_environment_at_call_time --- .../transaction_service_api.py | 19 ++++++++------ .../tests/api/test_transaction_service_api.py | 26 ++++++++++++++++++- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/safe_eth/safe/api/transaction_service_api/transaction_service_api.py b/safe_eth/safe/api/transaction_service_api/transaction_service_api.py index 959f18271..8a5485378 100644 --- a/safe_eth/safe/api/transaction_service_api/transaction_service_api.py +++ b/safe_eth/safe/api/transaction_service_api/transaction_service_api.py @@ -69,14 +69,17 @@ def __init__( # Resolved here rather than as default argument values, so that the # environment is read at call time and an explicit `None` (as passed by # `SafeBaseAPI.from_ethereum_client`) still falls back to it. - super().__init__( - network, - ethereum_client, - base_url, - api_key or os.environ.get("SAFE_TRANSACTION_SERVICE_API_KEY"), - request_timeout - or int(os.environ.get("SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT", 10)), - ) + # + # The check is `is None` rather than a truthiness test so that any value + # the caller actually passed wins: `api_key=""` forces an anonymous + # client even when the environment variable is set. + if api_key is None: + api_key = os.environ.get("SAFE_TRANSACTION_SERVICE_API_KEY") + if request_timeout is None: + request_timeout = int( + os.environ.get("SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT", 10) + ) + super().__init__(network, ethereum_client, base_url, api_key, request_timeout) def _get_url_by_network(self, network: EthereumNetwork) -> Optional[str]: network_short_name = self.NETWORK_SHORTNAME.get(network) diff --git a/safe_eth/safe/tests/api/test_transaction_service_api.py b/safe_eth/safe/tests/api/test_transaction_service_api.py index 3d1318fb9..48449b1d1 100644 --- a/safe_eth/safe/tests/api/test_transaction_service_api.py +++ b/safe_eth/safe/tests/api/test_transaction_service_api.py @@ -356,7 +356,7 @@ def test_decode_data(self): ) -class TestTransactionServiceApiApiKey(SimpleTestCase): +class TestTransactionServiceApiEnvConfig(SimpleTestCase): """ The API key and request timeout are read from the environment. These tests do not need network access or a real key, so they are kept out of @@ -414,6 +414,21 @@ def test_api_key_absent(self): with mock.patch.dict(os.environ, {}, clear=True): self.assertIsNone(TransactionServiceApi(EthereumNetwork.MAINNET).api_key) + def test_empty_api_key_forces_anonymous(self): + # Only `None` means "not supplied". An empty string is a deliberate + # choice to send no Authorization header, and must survive a set + # environment variable. + with mock.patch.dict(os.environ, {self.ENV_API_KEY: "env-api-key"}, clear=True): + self.assertEqual( + TransactionServiceApi(EthereumNetwork.MAINNET, api_key="").api_key, "" + ) + self.assertEqual( + TransactionServiceApi.from_ethereum_client( + self._ethereum_client_mock(), api_key="" + ).api_key, + "", + ) + def test_request_timeout_read_from_environment_at_call_time(self): with mock.patch.dict(os.environ, {self.ENV_TIMEOUT: "42"}, clear=True): self.assertEqual( @@ -430,3 +445,12 @@ def test_request_timeout_read_from_environment_at_call_time(self): ).request_timeout, 5, ) + + # Same reasoning as the empty API key: 0 is a value, not an absence. + with mock.patch.dict(os.environ, {self.ENV_TIMEOUT: "42"}, clear=True): + self.assertEqual( + TransactionServiceApi( + EthereumNetwork.MAINNET, request_timeout=0 + ).request_timeout, + 0, + )