Skip to content

fix: Read transaction service API key from the environment at call time - #2596

Open
Primata wants to merge 2 commits into
safe-global:mainfrom
Primata:fix-api-key-from-environment
Open

fix: Read transaction service API key from the environment at call time#2596
Primata wants to merge 2 commits into
safe-global:mainfrom
Primata:fix-api-key-from-environment

Conversation

@Primata

@Primata Primata commented Jul 25, 2026

Copy link
Copy Markdown

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

@Primata
Primata requested a review from a team as a code owner July 25, 2026 19:06
Copilot AI review requested due to automatic review settings July 25, 2026 19:06
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@datadog-official

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes TransactionServiceApi initialization so the API key and request timeout are resolved from environment variables at constructor call time (instead of import time), and so from_ethereum_client(..., api_key=None) no longer unintentionally disables env-based authentication. It also adds unit tests to lock in the intended precedence rules.

Changes:

  • Resolve SAFE_TRANSACTION_SERVICE_API_KEY and SAFE_TRANSACTION_SERVICE_REQUEST_TIMEOUT inside TransactionServiceApi.__init__ instead of as default argument values.
  • Add SimpleTestCase tests covering env-at-call-time behavior, explicit-argument precedence, and the from_ethereum_client forwarding case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
safe_eth/safe/api/transaction_service_api/transaction_service_api.py Moves env resolution into the constructor to avoid import-time default evaluation and api_key=None overriding env.
safe_eth/safe/tests/api/test_transaction_service_api.py Adds SimpleTestCase tests validating API key/timeout env resolution and precedence behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread safe_eth/safe/api/transaction_service_api/transaction_service_api.py Outdated
Comment thread safe_eth/safe/tests/api/test_transaction_service_api.py Outdated
`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
@Primata
Primata force-pushed the fix-api-key-from-environment branch from 2afc15a to 7ed6f6b Compare July 25, 2026 19:11
…ment

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
Copilot AI review requested due to automatic review settings July 26, 2026 20:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants