-
Notifications
You must be signed in to change notification settings - Fork 92
feat: enable Zerion balances globally with dynamic chain discovery #2851
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
Replace per-chain allowlist with global on/off switch for Zerion balances. The balances API now dynamically fetches supported chains from Zerion's API, eliminating the need to maintain hardcoded chain lists. Changes: - Replace FF_ZERION_BALANCES_CHAIN_IDS with FF_ZERION_BALANCES_ENABLED - Add dynamic chain fetching to ZerionBalancesApi (similar to portfolio API) - Fetch and cache chain mappings from Zerion's /v1/chains endpoint - Update BalancesApiManager to use boolean flag instead of chain ID array - Update all tests to reflect new configuration structure This enables Zerion on all 40+ supported chains with a single flag, instead of maintaining a hardcoded list of 14 chains. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Instead of introducing a new environment variable, reuse the existing FF_ZERION_BALANCES_CHAIN_IDS but change its behavior to accept a boolean value (true/false) instead of a comma-separated list of chain IDs. This maintains backwards compatibility and simplifies deployment: - No new environment variables needed - Existing variable name preserved - Old format (chain IDs) implicitly disabled (evaluates to false) - New format: FF_ZERION_BALANCES_CHAIN_IDS=true 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Check if FF_ZERION_BALANCES_CHAIN_IDS is set to any value (truthy) instead of specifically checking for 'true'. This provides: - Better backwards compatibility: existing chain ID lists automatically enable Zerion - Simpler logic: any non-empty value enables the feature - No breaking changes: deployments with the old format continue working Usage: - FF_ZERION_BALANCES_CHAIN_IDS=true (enabled) - FF_ZERION_BALANCES_CHAIN_IDS=1,10,137 (enabled, old format) - FF_ZERION_BALANCES_CHAIN_IDS= (disabled) - Unset (disabled) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove dynamic Zerion chain fetching from _getChainName to prevent: - Extra cache lookups on every balance/collectibles request - Increased API usage and costs - Unnecessary async overhead Chain names are now resolved from: 1. chain.balancesProvider.chainName (from Safe Config API) 2. Hardcoded chainsConfiguration (fallback) New chains can be added to Safe Config's chain configuration instead of relying on dynamic Zerion API calls. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements lazy initialization to fetch chain mappings from Zerion's API: - Fetches chains on first balance/collectibles request - Stores mappings in memory for the lifetime of the service - Handles concurrent requests during initial fetch - Falls back to minimal hardcoded list if fetch fails Benefits: - No hardcoded chain list maintenance (reduced from 14 to 3 fallback chains) - Automatic support for all 40+ Zerion chains - No startup delay (fetches only when needed) - No per-request overhead (in-memory after first fetch) - Single API call per network type (mainnet/testnet) Chain resolution priority: 1. chain.balancesProvider.chainName (from Safe Config) 2. Dynamically fetched mapping from Zerion API 3. Hardcoded fallback (ethereum, polygon, arbitrum) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove the hardcoded chains configuration and fallback logic: - Deleted chains object from zerion config - Removed chainsConfiguration property and import - Simplified chain resolution to rely fully on dynamic fetch Chain resolution now: 1. chain.balancesProvider.chainName (from Safe Config) - if set 2. Dynamically fetched from Zerion API - all other chains This completes the goal of removing maintenance burden of hardcoded chain lists. If Zerion API is unavailable, the service will properly fail rather than partially work with a limited fallback list. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove unused 'sample' import from balances-api.manager.spec.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove chains object from test configuration - Update zerion-balances-api tests to reflect new dynamic chain fetching - Replace static chain fallback test with error case test for unsupported chains - Add warn method to mock logging service 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Set zerionBalancesEnabled: false in default test configuration to allow most tests to use SafeBalancesApi behavior - Override zerionBalancesEnabled to true in Zerion-specific tests (zerion-collectibles.controller.spec.ts) - This fixes failing tests that expected new API instances after cache clearing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@claude review |
|
Claude finished @katspaugh's task —— View job PR Review: Zerion Global Balances FeatureTodo List:
Code Review Findings🛠️ Issues to Address1. Race Condition in Chain Mapping InitializationLocation: The concurrent handling during chain mapping fetch has a potential race condition: private async _ensureChainMappings(): Promise<void> {
// If already fetched, return immediately
if (
Object.keys(this.chainMappings.mainnet).length > 0 ||
Object.keys(this.chainMappings.testnet).length > 0
) {
return;
}Problem: The condition checks if either mainnet OR testnet has mappings, but the fetch process could fail for one network while succeeding for the other. This could cause the service to think mappings are complete when only partial data exists. Fix: Change the logic to track completion status separately or ensure both networks are properly fetched. 2. Error Handling InconsistencyLocation: try {
// Fetch mainnet chains
const mainnetMapping = await this._fetchChainMappingForNetwork(false);
this.chainMappings.mainnet = mainnetMapping;
} catch (error) {
this.loggingService.warn(
`Failed to fetch mainnet chains from Zerion: ${error}`,
);
}Problem: If chain fetching fails for both networks, the service silently continues with empty mappings, leading to runtime failures later when Fix: Consider throwing an error if both network fetches fail, or implement a retry mechanism. 3. Potential Memory LeakLocation: The chain mappings are stored in memory indefinitely without any cleanup mechanism. In a long-running service, if Zerion adds many new chains, this could lead to memory bloat. Recommendation: Consider implementing a TTL mechanism or periodic refresh strategy. 4. Missing Input ValidationLocation: const decimalChainId = hexToNumber(
chain.attributes.external_id as Hex,
).toString();Problem: No validation that Fix: Add validation or proper error handling for malformed external IDs. ⚡ Performance Considerations
🔒 Security AssessmentNo security vulnerabilities identified. The implementation properly:
📊 Test CoverageNeeds Improvement: The test files show updated boolean flag behavior, but missing tests for:
✅ Positive Aspects
Overall Assessment: Solid implementation with good architectural decisions. The main concerns are around edge case error handling and potential race conditions during initialization. Consider addressing the race condition issue and improving error resilience for production deployment. |
- Fix race condition by tracking initialization status separately - Improve error handling to fail fast when both network fetches fail - Add input validation for external_id hex conversion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
vseehausen
left a comment
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.
Nice one!
I would vouch got renaming the env variable to "zerion enabled", like internally, so the env variable name is precise.
Also, there should be logic to fetch and cache chain configuration that could be reused.
Last comment is that the Zerion Balance endpoint is not used in production currently. We could think about removing it.
None of this is blocking for me, but the first comment is a more-than-nice-to-have.
I kept the variable name the same to avoid migrating the existing one, but it's a valid point for clarity. I'll rename it. |
Renames the environment variable to be more precise about its purpose, as requested in PR review. The variable now clearly indicates it enables Zerion globally rather than referencing the old chain IDs behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Thanks for the review @vseehausen! I've addressed the env variable naming concern:
Regarding the other points:
Let me know if you'd like any additional changes! |
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 29
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Support both the new FF_ZERION_ENABLED and legacy FF_ZERION_BALANCES_CHAIN_IDS environment variables to ensure existing deployments continue to work. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Responding to @cursor[bot]'s review comment about backwards compatibility: Good catch! Fixed in commit fcd4a5d - now supporting both environment variables: zerionBalancesEnabled:
!!process.env.FF_ZERION_ENABLED ||
!!process.env.FF_ZERION_BALANCES_CHAIN_IDS,This maintains backwards compatibility:
|
Resolves https://linear.app/safe-global/issue/WA-1216/enable-zerion-balances-globally-with-dynamic-chain-discovery
Summary
FF_ZERION_BALANCES_CHAIN_IDSfrom a per-chain allowlist to a global enable/disable flagChanges
Configuration
Before:
FF_ZERION_BALANCES_CHAIN_IDS=1,10,137,42161,... # Comma-separated chain IDsAfter:
Backwards Compatibility
✅ Zero breaking changes! Existing configurations with chain ID lists will continue to work:
FF_ZERION_BALANCES_CHAIN_IDS=1,10,137→ Zerion enabled globallyFF_ZERION_BALANCES_CHAIN_IDS=true→ Zerion enabled globallyFF_ZERION_BALANCES_CHAIN_IDS=or unset → Zerion disabledThe feature flag now uses truthy checking (
!!process.env.FF_ZERION_BALANCES_CHAIN_IDS), so any non-empty value enables it.Dynamic Chain Fetching
Chain Resolution Priority
chain.balancesProvider.chainName(from Safe Config API) - if configuredBalances API Manager
if (chainId in array)toif (zerionBalancesEnabled)Benefits
Performance Impact
Migration Path
For chains not in old hardcoded list (e.g., Linea, Blast):
balancesProvider.chainNameto Safe Config for that chainTest plan
true,1,10,137, empty, unset🤖 Generated with Claude Code
Note
Introduces a global toggle for Zerion and removes chain-specific allowlists by discovering supported chains at runtime.
features.zerionBalancesChainIdswithfeatures.zerionBalancesEnabled; derive fromFF_ZERION_ENABLEDor legacyFF_ZERION_BALANCES_CHAIN_IDS. Update.env.sampleaccordingly.BalancesApiManagernow selectsZerionBalancesApiwhenzerionBalancesEnabledis true; retains counterfactual fallback logic otherwise.ZerionBalancesApifetches/v1/chains(mainnet/testnet) on first request, caches mappings in memory, validatesexternal_idhex, and errors if a chain is unsupported. Removes static chain config.chain.balancesProvider.chainNameor dynamic mapping; include testnet header viagetZerionHeaders.Written by Cursor Bugbot for commit fcd4a5d. This will update automatically on new commits. Configure here.