fix: Raise Etherscan client errors and stop flaky network tests - #2760
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Etherscan client: - Detect every rate limit message (per second, daily, free tier) and HTTP 429 as EtherscanRateLimitError. Before, only "Max rate limit reached" was detected and any other error returned None - Raise EtherscanHttpError for other non ok HTTP responses and EtherscanClientException for other API errors, instead of returning None. None now only means the contract is not verified - Wrap transport errors with wrap_http_exceptions as EtherscanConnectionError - _retry_request raises EtherscanRateLimitError after the last attempt instead of returning None Tests and CI: - Fix pytest.mark.flaky kwarg: delay is not valid, reruns_delay is - Add a `network` marker for tests hitting external services (third party APIs and RPC nodes). CI only runs them on Python 3.13, so the 4 matrix jobs do not share the same API keys at the same time - Etherscan network tests skip on rate limit, connection and HTTP errors, but an invalid API key still fails - Add offline unit tests for the Etherscan error handling
Contributor
|
There’s also an issue with the GitHub configuration. This is now pending: |
- Add EtherscanDailyRateLimitError, raised for the daily quota message. _retry_request raises it with no retry, as the quota only resets the next day - Move the last attempt of _retry_request out of the loop, so there is no dead `return None` - Name the test-app job `test-app (<python version>)`, so it matches the required status checks on main. With the `include:` matrix GitHub added `not network` to the job name
Uxío (Uxio0)
force-pushed
the
fix/etherscan-flaky-tests
branch
from
September 24, 2026 10:45
b72b8c2 to
3456077
Compare
test_domain_hash_to_hex_str and the mocked test_init make no request, so they run on every Python version.
Felipe Alvarado (falvaradorodriguez)
approved these changes
Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
test_etherscan_get_contract_metadatafails often in CI withAttributeError: 'NoneType' object has no attribute 'name', and passes when it is run again. Three causes:Max rate limit reachedmessage. Other rate limit messages (Max calls per sec rate limit reached (5/sec), the daily limit), HTTP 429 and any other API error returnedNone. So the test gotNoneinstead ofEtherscanRateLimitError, and theskipTestnever ran.@pytest.mark.flaky(reruns=5, delay=2):delayis not a valid kwarg for pytest-rerunfailures, it is ignored. Reruns ran with no delay, inside the same rate limit window.Changes
Etherscan client (
safe_eth/eth/clients/etherscan_client_v2.py):rate limit, and HTTP 429, raiseEtherscanRateLimitErrorEtherscanHttpError(withstatus_code)Invalid API Key) raiseEtherscanClientExceptionNoneis only returned when the contract is not verifiedwrap_http_exceptionsas the newEtherscanConnectionError(also a builtinConnectionError)_retry_requestraisesEtherscanRateLimitErrorafter the last attempt, instead of returningNoneAsyncEtherscanClientV2Tests and CI:
reruns_delaynetworkpytest marker for tests hitting external services: Etherscan, Blockscout, Sourcify, ENS, CowSwap, 4337 bundler, Transaction Service API, and the mainnet/polygon RPC node tests. CI runs them only on Python 3.13, the other jobs run-m "not network"include:. Apython-version == 3.13 && '' || 'not network'expression does not work: GitHub Actions treats''as false, so it always returns'not network'Breaking change
Callers that relied on
Nonefor Etherscan errors now get an exception. All new exceptions subclassEtherscanClientException.safe-decoder-servicecatches only(OSError, EtherscanRateLimitError)in its Etherscan -> Sourcify -> Blockscout fallback, so an HTTP error or invalid key would stop the fallback. It must be fixed before upgrading: PLA-2030.