-
Notifications
You must be signed in to change notification settings - Fork 0
fix: 🛠️ Add Base Support #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* refactor: ♻️ big refactor * refactor: ♻️ Tidy * docs: 📝 update readme * refactor: ♻️ pr comments
* feat: add validation for asset prices in total_assets calculation (#35) * feat: add validation for asset prices in total_assets calculation - Implemented checks for invalid prices (<= 0) in the calculate_total_assets function. - Added error handling to raise ValueError with details of invalid prices. - Introduced a new test to ensure that invalid prices trigger the appropriate exception. * refactor: turn invalid prices computation into list comprehension * feat: add price validation for price adapter (#36) * fix: update WstETHAdapter and tests to set ETH base asset price to 1 - Changed the base asset price for ETH in WstETHAdapter from 0 to 1 to ensure accurate pricing. - Updated related tests to verify that the ETH price is now correctly set to 1 instead of 0. - Adjusted documentation to clarify the base asset pricing behavior. * feat: add price validation to BasePriceAdapter * refactor: change validate_prices method to synchronous in price adapters - Updated the validate_prices method in BasePriceAdapter to be synchronous instead of asynchronous. - Adjusted calls to validate_prices in ChainlinkAdapter, CowSwapAdapter, and WstETHAdapter accordingly. * feat: add TypedDict for network addresses in constants.py * feat: add NetworkAddresses instances * refactor: rename NetworkAddresses to NetworkTokens and update address constants * feat: add network configuration and asset retrieval to OracleCLIConfig * refactor: update WstETHAdapter to use dynamic asset addresses from config * refactor: enhance WstETHAdapter to enforce required ETH address and simplify checks * refactor: update CowSwapAdapter to use dynamic asset addresses and improve network configuration * refactor: update ChainlinkAdapter to use dynamic asset addresses from config and enforce required ETH address * refactor: update IdleBalancesAdapter to use dynamic asset addresses from config and enforce required ETH and USDC addresses * refactor: remove optional asset address attributes from Chainlink, CowSwap, and WstETH adapters * refactor: enforce required USDC address in HyperliquidAdapter and update asset address retrieval from config * refactor: update Chainlink test suite to use dynamic asset addresses from config * refactor: update WstETHAdapter tests to utilize dynamic asset addresses from config * refactor: enhance HyperliquidAdapter tests to utilize dynamic USDC addresses from config * refactor: update tests to utilize dynamic asset addresses from config for improved consistency * refactor: update Chainlink and CowSwap tests to utilize dynamic asset addresses from config for improved consistency * refactor: update Chainlink tests to include dynamic ETH address in price fetching for USDT and USDS on testnet * refactor: update import paths from config to settings in CowSwap and Hyperliquid test files
Co-authored-by: Copilot <[email protected]>
* docs * docs
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
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 implements comprehensive configuration management, Base network support, and modular asset adapter infrastructure for the TQ Oracle system.
Key Changes:
- Migrated from dataclass-based config to Pydantic Settings with multi-source configuration precedence (CLI > ENV > TOML)
- Added Base network support with network-specific asset mappings
- Restructured asset adapters to support per-subvault configuration and multi-chain operation
- Refactored orchestration into modular pipeline stages (preflight, assets, pricing, report)
Reviewed Changes
Copilot reviewed 49 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/tq_oracle/settings.py |
New Pydantic-based settings with TOML/ENV/CLI precedence and network-specific derivations |
src/tq_oracle/state.py |
Application state container replacing global config |
src/tq_oracle/constants.py |
Network-specific asset mappings for mainnet/sepolia/base |
src/tq_oracle/main.py |
Restructured CLI with callback-based initialization and settings injection |
src/tq_oracle/pipeline/ |
Modular pipeline stages replacing monolithic orchestrator |
src/tq_oracle/adapters/asset_adapters/ |
Chain-aware adapters with registry and per-subvault configuration |
tests/ |
Updated tests to use OracleSettings instead of OracleCLIConfig |
tq-oracle-example.toml |
Example configuration file demonstrating new TOML-based config |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Always sets the base asset to price 0 | ||
| if prices.base_asset in prices.prices: |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 89 is misleading as it refers to setting the base asset price but appears above the sorting logic. Move this comment directly above line 90-91 where the base asset price is actually set, and keep the sorting comment on line 92 where it belongs.
| # Always sets the base asset to price 0 | |
| if prices.base_asset in prices.prices: | |
| if prices.base_asset in prices.prices: | |
| # Always sets the base asset to price 0 |
| (SEPOLIA_ASSETS.get("USDC"), 6), | ||
| (ETH_MAINNET_ASSETS.get("USDT"), 6), | ||
| (ETH_MAINNET_ASSETS.get("USDS"), 18), |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TOKEN_DECIMALS dictionary construction filters out None addresses but doesn't include Base network assets. This creates an inconsistency where Base network USDC/USDT/USDS addresses won't be found in TOKEN_DECIMALS, potentially causing runtime errors. Add Base network assets to the list comprehension for completeness.
| (SEPOLIA_ASSETS.get("USDC"), 6), | |
| (ETH_MAINNET_ASSETS.get("USDT"), 6), | |
| (ETH_MAINNET_ASSETS.get("USDS"), 18), | |
| (SEPOLIA_ASSETS.get("USDC"), 6), | |
| (BASE_ASSETS.get("USDC"), 6), | |
| (ETH_MAINNET_ASSETS.get("USDT"), 6), | |
| (BASE_ASSETS.get("USDT"), 6), | |
| (ETH_MAINNET_ASSETS.get("USDS"), 18), | |
| (BASE_ASSETS.get("USDS"), 18), |
| addr | ||
| for addr in asset_addresses | ||
| if addr.lower() == self.wsteth_address.lower() | ||
| ) |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next() call will raise StopIteration if no matching address is found in asset_addresses. Since this code is only reached when has_wsteth is True (which checks if wsteth_address is in asset_addresses_lower), this should theoretically be safe. However, add a default value or explicit check to prevent potential edge cases where case sensitivity mismatches could cause runtime errors.
| addr | |
| for addr in asset_addresses | |
| if addr.lower() == self.wsteth_address.lower() | |
| ) | |
| (addr for addr in asset_addresses if addr.lower() == self.wsteth_address.lower()), | |
| None | |
| ) | |
| if wsteth_addr_actual is None: | |
| raise ValueError( | |
| f"wstETH address {self.wsteth_address} found in asset_addresses_lower but not in asset_addresses. " | |
| "This may be due to a case sensitivity mismatch." | |
| ) |
| if key in body: | ||
| raise ValueError( | ||
| f"Security violation: '{key}' found in TOML config file. " | ||
| f"Secrets must only be provided via environment variables or CLI flags." |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The security check prevents secrets in TOML files, which is good practice. However, the error message could be more actionable by specifying the exact environment variable name format (e.g., 'Use TQ_ORACLE_PRIVATE_KEY environment variable instead').
| f"Secrets must only be provided via environment variables or CLI flags." | |
| f"Use the '{OracleSettings.model_config['env_prefix']}{key.upper()}' environment variable instead." |
There was a problem hiding this comment.
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 30 out of 30 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
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 32 out of 32 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
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 32 out of 32 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Changes
eth_mainnet_rpcconfiguration option for L2 deploymentsTechnical Details
The wstETH adapter now distinguishes between:
This enables accurate wstETH pricing for vaults deployed on L2s like Base while maintaining the canonical wstETH exchange rate from mainnet.