Conversation
…TenacityRetryAdapter - Implemented tests for HttpxClientAdapter covering initialization, context management, GET/POST requests, error handling, and HTTP/2 support. - Added tests for new API methods in Method enum, ensuring their existence and correct specifications in EtherscanV2 and EtherscanLikeScanner. - Developed tests for TenacityRetryAdapter, verifying retry behavior, exception handling, exponential backoff timing, and callback functionality. - Included tests for concurrent retries and integration with real exceptions from aiochainscan.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56f7a2f533
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| from typing import TYPE_CHECKING, Any, ClassVar | ||
|
|
||
| import aiohttp |
There was a problem hiding this comment.
This scanner has a hard top-level dependency on aiohttp, but the v0.4 dependency set no longer includes aiohttp; in a clean install, importing ChainscanClient loads aiochainscan.scanners and then blockscout_v2, which raises ModuleNotFoundError before any request is made. This makes the primary client API unusable unless users happen to have aiohttp installed transitively.
Useful? React with 👍 / 👎.
| api_kind=api_kind, # Use mapped api_kind for UrlBuilder compatibility | ||
| network=network_str, # Use string version of network | ||
| api_key=client_config['api_key'], | ||
| network=network_str, # Use original network string for client property |
There was a problem hiding this comment.
The from_config path computes scanner-specific aliases (including blockscout_v2: main -> ethereum) but then constructs the client with network_str instead of the normalized value, so ChainscanClient.from_config('blockscout_v2', 'main') still initializes the scanner with main and fails network validation. This breaks the common/default main network input despite the alias table explicitly intending to support it.
Useful? React with 👍 / 👎.
| { | ||
| 'symbol': token_info.get('symbol', ''), | ||
| 'name': token_info.get('name', ''), | ||
| 'contract_address': token_info.get('address', ''), |
There was a problem hiding this comment.
The DataFrame export maps contract_address from token.address, but BlockScout V2 token payloads use address_hash (as documented in this commit’s scanner parser comments), so exported token portfolios lose contract addresses and produce empty values for that column. This silently corrupts downstream analytics/join workflows that rely on contract addresses.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…aiohttp handling; apply code formatting fixes across multiple files.
- Added import tests to catch circular dependencies and import blockers early. - Updated CI/CD workflow to run import tests before other checks. - Enhanced pre-commit configuration to include import tests on every commit. - Created setup script for developers to install dependencies and validate setup. - Documented development setup and quality gates in README and CONTRIBUTING.md. - Removed outdated PRE_COMMIT_FIXES.md and added detailed quality gates documentation.
- Change exception handling from ModuleNotFoundError to ImportError in aiohttp_client import. - Add validation for max_size in InMemoryCache to ensure it is greater than 0. - Modify eviction logic in InMemoryCache to prevent eviction when updating existing keys. - Update orjson_parser to encode raw_str as UTF-8 before loading. - Refactor ChainscanClient to use normalized network for scanner compatibility and improve pagination handling for BlockScout V2. - Enhance error handling in transaction decoding to catch additional exceptions. - Update analytics service to handle both Etherscan and BlockScout V2 contract address formats. - Improve documentation in QUALITY_GATES.md and QUALITY_IMPROVEMENTS.md regarding pre-commit checks. - Add eth-hash[pycryptodome] as a development dependency in pyproject.toml. - Mock aiohttp imports in tests to avoid circular dependency issues. - Introduce comprehensive tests for InMemoryCache to verify eviction logic and max_size validation.
| ) as response, | ||
| ): | ||
| response.raise_for_status() | ||
| raw_response = await response.json() |
There was a problem hiding this comment.
New HTTP session created per page in pagination loop
High Severity
In iter_transactions, the BlockScout V2 pagination loop creates a new aiohttp.ClientSession() on every iteration of the while True loop. Each page of results opens a fresh TCP connection, destroying connection pooling. For wallets with many transactions, this means dozens of unnecessary TCP handshakes. The session also bypasses the client's configured self._network (with its rate limiting, retry logic, and httpx-based connection pool), using raw aiohttp instead.
| if parsed is not None: | ||
| # Etherscan returns Gwei, convert to wei | ||
| return parsed * 10**9 | ||
| return None |
There was a problem hiding this comment.
GasOracleDTO silently drops decimal gas price values
High Severity
The _parse_gwei_to_wei validator uses parse_hex_or_int, which calls int() on the raw string. Etherscan's gas oracle API returns all gas prices as decimal Gwei strings (e.g., "0.496839934"). Since int("0.496839934") raises ValueError, parse_hex_or_int returns None, causing all four gas price fields (safe_gas_price, propose_gas_price, fast_gas_price, suggest_base_fee) to silently become None — a complete data loss.
Additional Locations (1)
| ) | ||
| @classmethod | ||
| def _parse_hex_int_nullable(cls, v: Any) -> int | None: | ||
| return parse_hex_or_int(v) |
There was a problem hiding this comment.
Nullable validator on non-nullable int fields causes ValidationError
Medium Severity
The _parse_hex_int_nullable validator (which returns int | None) is applied to gas and nonce fields that are typed as non-nullable int with default=0. In Pydantic V2, if the API provides an empty string or null for these fields, the validator returns None, and Pydantic raises a ValidationError since None is not valid for int. The sibling value field correctly uses parse_hex_or_int_zero for its validator. The gas and nonce fields need the same treatment to gracefully default to 0 instead of crashing.
…e for InMemoryCache
| break | ||
|
|
||
| # Update query params with next_page_params for next iteration | ||
| query_params = {**query_params, **next_page_params} |
There was a problem hiding this comment.
Missing type validation for API pagination parameters
Medium Severity
The next_page_params from BlockScout V2 API response is unpacked with **next_page_params without validating it's a dictionary. If the API returns malformed data where next_page_params is a string, number, or other non-dict type, the unpacking operator raises TypeError: ... argument after ** must be a mapping, not <type>, crashing the iteration.
|
|
||
| __all__ = ['AiohttpClient', 'UrlBuilderEndpoint'] | ||
| __all__ = [ | ||
| 'AiohttpClient', |
There was a problem hiding this comment.
AiohttpClient exports None when import fails
Medium Severity
When aiohttp is not installed, AiohttpClient is set to None but still exported in __all__. This allows from aiochainscan.adapters import AiohttpClient to succeed, giving users None instead of raising ImportError. When users try to instantiate it with AiohttpClient(), they get a confusing TypeError: 'NoneType' object is not callable instead of a clear import error indicating the missing dependency.
| limits=limits, | ||
| proxy=self._proxy, | ||
| ) | ||
| return self._client |
There was a problem hiding this comment.
Race condition in Network client initialization
Medium Severity
The _ensure_client method lacks synchronization for concurrent access. When multiple coroutines call API methods simultaneously on the same ChainscanClient, they can race through the if self._client is None check, causing multiple httpx.AsyncClient instances to be created. Only the last assignment is retained, leaking the other client instances and their open connections.
Additional Locations (1)
…version for wheel builds
| break | ||
|
|
||
| # Update query params with next_page_params for next iteration | ||
| query_params = {**query_params, **next_page_params} |
There was a problem hiding this comment.
iter_transactions imports aiohttp which is not a dependency
High Severity
The new iter_transactions method imports aiohttp at line 517 for the BlockScout V2 pagination path, but aiohttp is not listed in pyproject.toml dependencies (the project migrated to httpx). This will crash with ModuleNotFoundError at runtime for any user who calls iter_transactions on a BlockScout V2 client. The method also creates a new aiohttp.ClientSession inside the while True pagination loop, bypassing the client's own self._network layer (which provides rate limiting, retry, and connection pooling via httpx).
- Add [tool.mypy] config in pyproject.toml with per-module overrides - Set ignore_missing_imports=true for polars, aiohttp, mcp modules - Enables CI to pass without installing optional dependencies - Maintains full type checking when packages are installed - All 353 tests passing, mypy clean locally Fixes CI lint failures caused by missing optional dependencies
- Move polars import to TYPE_CHECKING block in analytics.py - Move FastMCP import to TYPE_CHECKING block in mcp_server.py - Use Any type with type:ignore for optional dependency fallbacks - Remove unused type:ignore comments (now handled by TYPE_CHECKING) - All decorators now properly typed without additional ignores Mypy now passes both locally and in CI without optional dependencies
- Add mypy override to disable 'misc' error code in mcp_server module - Prevents 'untyped decorator' errors when mcp package not installed - Remove type:ignore comments from decorators (now handled by config) - Works in both environments: with mcp (local) and without (CI) This solves the issue where decorators are typed locally but untyped in CI
- Add Rust toolchain installation to all test jobs - Install maturin>=1.8 before build in test-wheel-install job - Install maturin>=1.8 in test-git-install job - Install maturin>=1.8 in test-dependencies job - Install maturin for editable install step Fixes maturin 'Failed to write to zip archive' error in CI Ensures ZIP64 support for all wheel builds in test workflows
|
@codex make review |
- Fixes Medium Severity issue found by Cursor Bugbot - ETH_PRICE is not supported by all scanners (e.g., BlockScout V2) - ACCOUNT_BALANCE is universally supported across all scanners - Use Vitalik's address (0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045) for consistent testing - Add regression test to verify universal method usage
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7dd1423a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -160,34 +199,54 @@ def from_config( | |||
| # Use the original network parameter for config lookup, not the resolved chain name | |||
| # Ensure network is a string for config lookup | |||
| network_str = str(network) if not isinstance(network, str) else network | |||
There was a problem hiding this comment.
from_config advertises network as str | int and even documents chain-id usage, but this line stringifies numeric inputs and later passes that raw value into config/scanner resolution; e.g. from_config('etherscan', 8453) becomes network '8453', which fails network validation/supported-network checks instead of selecting Base. This breaks a documented initialization path for all numeric chain-id callers.
Useful? React with 👍 / 👎.
| @@ -152,6 +190,7 @@ def from_config( | |||
| # For backward compatibility, map scanner names to their config IDs | |||
| scanner_id_map = { | |||
| 'blockscout': 'blockscout_eth', | |||
There was a problem hiding this comment.
This hardcoded mapping forces every from_config('blockscout', <network>) call through blockscout_eth, so non-ETH networks like polygon or gnosis fail config validation even though BlockScoutV1 itself supports them. As a result, the unified factory rejects valid BlockScout networks and prevents normal client creation outside Ethereum mainnet.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| # Import aiohttp locally to avoid hard dependency at module level | ||
| import aiohttp |
There was a problem hiding this comment.
The V2 scanner imports aiohttp at call time, but aiohttp was removed from required package dependencies in this commit, so a default install can construct the client but then crashes with ModuleNotFoundError on the first blockscout_v2 request. Since BlockScout V2 is a primary usage path, this becomes a runtime break for normal users unless the transport is switched to declared dependencies (httpx) or aiohttp is re-declared.
Useful? React with 👍 / 👎.
1. Chain ID resolution in from_config:
- from_config('etherscan', 8453) now correctly resolves to 'base'
- Numeric chain IDs are converted to network names before config lookup
- Prevents '8453' string being passed to network validation
2. BlockScout scanner config mapping:
- Fixed hardcoded 'blockscout_eth' mapping that broke other networks
- Now maps by actual network: polygon → blockscout_polygon, gnosis → blockscout_gnosis
- Enables normal client creation for all BlockScout-supported networks
3. aiohttp dependency issue in BlockScout V2:
- Replaced all aiohttp imports with httpx (declared dependency)
- Prevents ModuleNotFoundError on first blockscout_v2 request
- Updated exception handling to httpx.HTTPError/HTTPStatusError
- Updated tests to mock httpx.AsyncClient instead of aiohttp.ClientSession
Tests:
- Added test_client_from_config_with_chain_id (chain ID → network name)
- Added test_client_from_config_blockscout_network_mapping (network-specific config)
- Updated test_call_balance/token_portfolio mocks for httpx
- All 357 tests passing ✅
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
1. High Severity - Blockscout api_kind routing to wrong domain: - Fixed hardcoded 'blockscout_eth' api_kind for all networks - Now uses network-specific scanner_id (blockscout_polygon, blockscout_gnosis, etc.) - Ensures requests route to correct explorer domain for each network - Prevents returning incorrect data or failures from wrong domain 2. Low Severity - LRU cache evicting valid entries while expired remain: - InMemoryCache.set() now clears expired keys before LRU eviction - Prevents evicting still-valid entries when expired keys exist - More efficient cache usage - expired entries removed proactively Tests: - Added test_client_from_config_blockscout_api_kind_matches_network - Added test_expired_keys_cleared_before_eviction - All 359 tests passing ✅
1 participant
Note
High Risk
High risk because it removes the legacy
Client/modulesAPI and changes core HTTP/retry/rate-limit behavior, which can affect all runtime interactions and downstream integrations.Overview
Modernizes the package’s runtime and developer tooling: bumps
aiochainscanto0.4.0, switches the default HTTP stack tohttpx(HTTP/2) with newNetworkownership inChainscanClient, addsTenacityRetryAdapter/AioLimiterAdapter, expandsMethod(contract verification + token/NFT portfolio), and introduces Pydantic v2 DTOs indomain/dto_v2.py.Removes legacy surfaces and hardens configuration: deletes the old
Clientandmodules/*, updates CLI to useChainscanClient+Method, makesconfig_manager.get_scanner_config()return a deep copy to avoid shared mutable state leakage, and improves decoding robustness (eth_utils.keccak+ narrower exception handling).Reworks CI/CD and contributor workflow:
ci.ymlno longer builds/publishes packages (and adds a fast import test early), deletestest-install.yml, adds release-onlywheels.ymlusingcibuildwheel/maturin+ trusted publishing, and updates docs/pre-commit hooks to run import tests and stricter local checks; also addspython -m aiochainscanentrypoint and an optionalmcp_serverintegration.Written by Cursor Bugbot for commit 923bee0. This will update automatically on new commits. Configure here.