Feat/evm performance optimizations #1532
Conversation
Major performance improvements for EVM packages: - Parallel API calls: Replace sequential round-robin with Promise.any pattern - 2-3x faster provider queries using race conditions - All providers compete, fastest response wins - Parallel gas estimation: Run gas price and limit queries simultaneously - 40-50% faster fee calculation - Uses Promise.all for concurrent execution - Smart caching layer: Reduce object creation overhead - Cache Contract instances by address - Cache BigNumber instances by value - 20-30% memory reduction - Add fallback for environments without Promise.any support - Maintain backward compatibility with existing APIs - Benefits all EVM chain packages (Ethereum, BSC, Avalanche, etc.) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
📝 WalkthroughWalkthroughAdds an in-memory caching module for provider-scoped Contracts and string-keyed BigNumbers, integrates the caches into client and utility code, refactors provider round‑robin logic to use a promiseAny helper, adds MAYAChain gas price support, and adds tests for contract caching behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/xchain-evm/src/clients/client.ts (3)
689-697: Fix incorrect Transaction value/chainId types (breaks signing/broadcasting).In the native-asset path,
tx.valuemust bebigint, andtx.chainIdshould bebigint(you already use bigint in the ERC20 branch). Assigning a string/number here can cause runtime/type errors in ethers v6.- tx.chainId = await this.cachedNetworkId.getValue() + tx.chainId = BigInt(await this.cachedNetworkId.getValue()) tx.to = recipient - tx.value = amount.amount().toFixed() + tx.value = BigInt(amount.amount().toFixed()) tx.nonce = nonce
204-207: Clear caches on network change to avoid cross-network provider leakage.
getCachedContractcaches by address only (see cache.ts), so switching networks can reuse a contract bound to the previous provider. Clear caches when network changes.-import { getCachedContract, getCachedBigNumber } from '../cache' +import { getCachedContract, getCachedBigNumber, clearCaches } from '../cache' @@ setNetwork(network: Network): void { super.setNetwork(network) + // ensure cached Contract/BN instances don’t retain the old provider/network + clearCaches() this.cachedNetworkId = new CachedValue<number>(() => getNetworkId(this.getProvider())) }Additionally, consider keying the contract cache by
${chainId}:${address}(see note on cache.ts below).
940-945: Mirror v5 EIP-1559 compatibility in approve(), too.
transfer()upgrades to type-2 when EIP‑1559 fields are present;approve()always uses type-1. Align behaviors to avoid legacy-type-only approvals on type‑2 networks.- const tx = Transaction.from(rawUnsignedTx) - tx.type = 1 - tx.gasLimit = BigInt(gasLimit.toFixed()) - tx.gasPrice = BigInt(gasPrice.toFixed()) - tx.maxFeePerGas = null - tx.maxPriorityFeePerGas = null + const tx = Transaction.from(rawUnsignedTx) + const feeInfo = await this.getProvider().getFeeData() + tx.gasLimit = BigInt(gasLimit.toFixed()) + if (feeInfo.maxFeePerGas && feeInfo.maxPriorityFeePerGas) { + // emulate v5: map legacy gasPrice into both EIP‑1559 fields + const gp = BigInt(gasPrice.toFixed()) + tx.type = 2 + tx.maxFeePerGas = gp + tx.maxPriorityFeePerGas = gp + tx.gasPrice = null + } else { + tx.type = 1 + tx.gasPrice = BigInt(gasPrice.toFixed()) + tx.maxFeePerGas = null + tx.maxPriorityFeePerGas = null + }
🧹 Nitpick comments (12)
packages/xchain-client/src/protocols.ts (1)
6-6: Nit: fix protocol name in comment casing.Use "MAYACHAIN" consistently.
- MAYACHAIN = 2, // Protocol value for MAYAChain + MAYACHAIN = 2, // Protocol value for MAYACHAINpackages/xchain-evm/src/cache.ts (1)
24-30: Unbounded BigNumber cache can grow indefinitely.Consider capping size (simple LRU) since inputs can be unbounded.
If helpful, I can provide a tiny LRU wrapper.
packages/xchain-client/src/BaseXChainClient.ts (2)
165-221: Duplicate unit-conversion logic between Thorchain and Mayachain.Factor into a shared
normalizeGasRate(gasRate, gasRateUnits, chain)helper to avoid drift (ARB centigwei handling already diverges here vs. Thorchain).I can draft the helper and update both call sites.
247-259: Add axios timeouts and basic retries for robustness.Network calls are on critical paths; add a small timeout and 1–2 retries with backoff.
Example (outside this hunk): create an axios instance with
timeout: 10_000and use it in both node getters.packages/xchain-evm/src/clients/client.ts (8)
81-106: Prefer native Promise.any with AggregateError; fall back only when unavailable.Your helper matches “any” semantics, but the comment mentions
Promise.race, and you lose per-cause details. UsePromise.anywhen available and fall back to the polyfill, preserving anAggregateErrorshape.-/** - * Helper function to race promises and return the first successful result - * Uses Promise.race with proper error handling as fallback for Promise.any - */ -async function promiseAny<T>(promises: Promise<T>[]): Promise<T> { +/** + * Promise.any polyfill with native fast-path; returns first fulfilled value, + * rejects with AggregateError if all reject. + */ +async function promiseAny<T>(promises: Promise<T>[]): Promise<T> { // Use Promise.race to get the first resolved promise - const errors: Error[] = [] + const errors: unknown[] = [] return new Promise((resolve, reject) => { let completedCount = 0 + if (promises.length === 0) { + reject(new AggregateError([], 'No promises provided')) + return + } + + if ('any' in Promise && typeof (Promise as any).any === 'function') { + ;(Promise as any).any(promises).then(resolve, reject) + return + } promises.forEach((promise) => { promise.then(resolve).catch((error) => { errors.push(error) completedCount += 1 if (completedCount === promises.length) { - reject(new Error('All promises failed: ' + errors.map((e) => e.message).join(', '))) + reject(new AggregateError(errors, 'All promises failed')) } }) }) - - if (promises.length === 0) { - reject(new Error('No promises provided')) - } }) }
409-423: Avoid float math for gwei→wei conversion (precision).Multiplying JS numbers by
1e9risks precision drift. Use BigNumber (cached) or bigint and pass strings tobaseAmount.- const ratesInGwei: FeeRates = standardFeeRates(await this.getFeeRateFromMayachain()) - return { - [FeeOption.Average]: baseAmount(ratesInGwei[FeeOption.Average] * 10 ** 9, this.config.gasAssetDecimals), - [FeeOption.Fast]: baseAmount(ratesInGwei[FeeOption.Fast] * 10 ** 9, this.config.gasAssetDecimals), - [FeeOption.Fastest]: baseAmount(ratesInGwei[FeeOption.Fastest] * 10 ** 9, this.config.gasAssetDecimals), - } + const ratesInGwei: FeeRates = standardFeeRates(await this.getFeeRateFromMayachain()) + const toWei = (g: number) => getCachedBigNumber(g).multipliedBy(1e9).toFixed() + return { + [FeeOption.Average]: baseAmount(toWei(ratesInGwei[FeeOption.Average]), this.config.gasAssetDecimals), + [FeeOption.Fast]: baseAmount(toWei(ratesInGwei[FeeOption.Fast]), this.config.gasAssetDecimals), + [FeeOption.Fastest]: baseAmount(toWei(ratesInGwei[FeeOption.Fastest]), this.config.gasAssetDecimals), + }Also consider the same change in the THORChain block (Lines 399–403).
342-347: Fix error message to match the method being checked.Minor but confusing: you check
broadcastTransactionthen throw “sendTransaction”.- if (!provider.broadcastTransaction) { - throw new Error('Provider does not support sendTransaction') - } + if (!provider.broadcastTransaction) { + throw new Error('Provider does not support broadcastTransaction') + }
559-577: Round-robin helper: reduce duplication and remove no-op filter(Boolean).
filter(Boolean)on promises is a no-op.- Four methods duplicate the same pattern; consider a small internal helper
roundRobin<T>(fn)to DRY.- const promises = this.config.dataProviders - .map(async (provider) => { + const promises = this.config.dataProviders.map(async (provider) => { const prov = provider[this.network] if (!prov) throw new Error('Provider not available for network') return await prov.getBalance(address, assets) - }) - .filter(Boolean) + })
587-604: Same as above: consider extracting a shared round-robin utility.
613-630: Same as above: remove no-op filter and unify error strings casing (“getTransactions”).
637-655: Same as above: DRY round-robin and prefer consistent error text (“getFeeRates”).
217-218: Nit: getAddressAsync default verify=false is fine; consider exposingverifyin higher-level APIs if Ledger UX needs it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/xchain-client/src/BaseXChainClient.ts(3 hunks)packages/xchain-client/src/protocols.ts(1 hunks)packages/xchain-evm/src/cache.ts(1 hunks)packages/xchain-evm/src/clients/client.ts(15 hunks)packages/xchain-evm/src/utils.ts(6 hunks)packages/xchain-utxo/src/client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/xchain-utxo/src/client.ts (1)
packages/xchain-client/src/feeRates.ts (1)
standardFeeRates(19-25)
packages/xchain-evm/src/utils.ts (2)
packages/xchain-util/src/asset.ts (1)
baseAmount(103-123)packages/xchain-evm/src/cache.ts (2)
getCachedBigNumber(24-30)getCachedContract(12-19)
packages/xchain-evm/src/cache.ts (1)
packages/xchain-evm/src/index.ts (1)
abi(35-38)
packages/xchain-client/src/BaseXChainClient.ts (2)
packages/xchain-client/src/types.ts (1)
FeeRate(145-145)packages/xchain-util/src/types/chain.ts (1)
Chain(1-1)
packages/xchain-evm/src/clients/client.ts (1)
packages/xchain-evm/src/cache.ts (2)
getCachedBigNumber(24-30)getCachedContract(12-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
packages/xchain-utxo/src/client.ts (2)
180-188: LGTM: correct THORCHAIN branch check.Switching to
protocol === Protocol.THORCHAINavoids truthy mistakes.
189-196: MAYACHAIN fee path added — verify unit normalization end-to-end.Ensure
getFeeRateFromMayachain()returns units your UTXO clients expect (sats/byte for UTXO; no extra scaling here).I can add an integration test hitting
/inbound_addresseson Mayanode and asserting the mappedstandardFeeRates.packages/xchain-evm/src/utils.ts (4)
96-98: LGTM: approval amount now uses cache.Matches semantics and reduces allocations.
153-160: LGTM: dynamic call wrapper.Contract connection via signer only when provided is correct.
179-180: LGTM: contract factory now uses cache.Return type remains Promise-compatible via async wrapper.
242-246: LGTM: allowance check now uses cached instances.Good conversion from bigint to BigNumber string.
packages/xchain-evm/src/clients/client.ts (4)
501-511: Parallelization LGTM.Running
estimateGasPricesandestimateGasLimitconcurrently is a clean win.
456-465: Caching ERC20 contract and using cached BigNumber looks good.Reduces allocations and RPC churn on hot paths.
356-432: Units sanity-check: are provider fee rates (round-robin/default) wei or gwei?
- THORChain/MAYAchain branches treat inputs as gwei and convert to wei.
- The round‑robin provider path and the default path pass raw numbers to
baseAmountwithout conversion.Please verify:
EvmOnlineDataProviders#getFeeRates()returns wei or gwei?defaults[network].gasPriceunit.If both are gwei, apply the same 1e9 conversion used in the THOR/MAYA branches.
739-755: Good: approve() now uses cached Contract; ensure cache is network-safe.This benefits performance. See earlier note to clear caches on
setNetwork()and/or key cache by chainId.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
packages/xchain-evm/src/cache.ts (3)
4-7: Make the contract cache provider-scoped (WeakMap) to avoid cross-runner bleed and memory leaks.Keying by a computed string risks returning a Contract bound to the wrong provider when two distinct Provider instances share the same URL. It also prevents GC of entries tied to short-lived providers. Use WeakMap<Provider, Map<lowercasedAddress, Contract>> instead. This also removes the need for network I/O during keying.
Apply:
-// Per-provider contract cache to ensure contracts are properly isolated -// Key format: `${providerNetwork}_${chainId}_${address}` -const contractCache = new Map<string, Contract>() +// Per-provider contract cache to ensure contracts are isolated and GC-friendly +// provider -> (address -> Contract) +let contractCache: WeakMap<Provider, Map<string, Contract>> = new WeakMap()
59-62: Reset WeakMap on clear to release provider-scoped caches.-export function clearCaches(): void { - contractCache.clear() - bigNumberCache.clear() -} +export function clearCaches(): void { + contractCache = new WeakMap() + bigNumberCache.clear() +}
36-43: Make getCachedContract synchronous and provider-scoped.With WeakMap<Provider,...>, contract lookup/creation is sync and avoids network calls.
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -export async function getCachedContract(address: string, abi: any, provider: Provider): Promise<Contract> { - const key = await getContractCacheKey(address, provider) - if (!contractCache.has(key)) { - contractCache.set(key, new Contract(address, abi, provider)) - } - return contractCache.get(key)! -} +export function getCachedContract(address: string, abi: InterfaceAbi, provider: Provider): Contract { + const addr = address.toLowerCase() + let byAddress = contractCache.get(provider) + if (!byAddress) { + byAddress = new Map<string, Contract>() + contractCache.set(provider, byAddress) + } + if (!byAddress.has(addr)) { + byAddress.set(addr, new Contract(address, abi, provider)) + } + return byAddress.get(addr)! +}packages/xchain-client/src/BaseXChainClient.ts (1)
30-33: Verify and fix the Testnet Mayanode API URL.Based on previous verification, the testnet endpoint at
https://testnet.mayanode.mayachain.info/mayachainfails with DNS resolution errors. Consider making these URLs configurable via environment variables or constructor parameters.#!/bin/bash # Verify current status of all three Mayanode endpoints for network in mainnet stagenet testnet; do url="https://${network}.mayanode.mayachain.info/mayachain/inbound_addresses" if [ "$network" = "mainnet" ]; then url="https://mayanode.mayachain.info/mayachain/inbound_addresses" fi echo "Testing $network: $url" if curl -fsSL --max-time 5 "$url" 2>/dev/null | head -c 100 > /dev/null; then echo "✓ $network endpoint is accessible" else echo "✗ $network endpoint failed: $(curl -sSL --max-time 5 "$url" 2>&1 | head -1)" fi echo done
🧹 Nitpick comments (6)
packages/xchain-evm/src/cache.ts (3)
48-54: Consider canonical keys or an LRU for BigNumber cache.Keying by raw value.toString() can create duplicates ("1" vs "1.0") and grow unbounded. Either normalize via new BigNumber(value).toString(10) or cap the cache (e.g., LRU 1–5k entries).
Example minimal normalization:
-export function getCachedBigNumber(value: string | number): BigNumber { - const stringValue = value.toString() - if (!bigNumberCache.has(stringValue)) { - bigNumberCache.set(stringValue, new BigNumber(stringValue)) - } - return bigNumberCache.get(stringValue)! -} +export function getCachedBigNumber(value: string | number): BigNumber { + const canonical = new BigNumber(value).toString(10) + let bn = bigNumberCache.get(canonical) + if (!bn) { + bn = new BigNumber(canonical) + bigNumberCache.set(canonical, bn) + } + return bn +}
23-27: Prettier nits: chain the calls on separate lines.If you keep this code path, format the chaining across lines as suggested by the linter.
36-43: If you decide to keep the async API, de-duplicate concurrent creations.Two concurrent calls for the same key can create two Contract instances. Option: maintain an inFlight map (key -> Promise) to coalesce.
packages/xchain-evm/__tests__/cache.test.ts (1)
46-46: Prettier: add trailing newline at EOF.packages/xchain-client/src/BaseXChainClient.ts (2)
96-159: Consider extracting the gas rate conversion logic into a reusable function.The gas rate conversion logic in
getFeeRateFromThorchainandgetFeeRateFromMayachain(lines 165-206) is nearly identical. This violates the DRY principle and makes maintenance harder.Extract the conversion logic into a shared private method:
+ /** + * Convert gas rate to the expected unit based on gas_rate_units + * @param {number} gasRate The raw gas rate value + * @param {string} gasRateUnits The units of the gas rate + * @param {Chain} chain The blockchain chain for context + * @returns {FeeRate} The normalized fee rate + */ + private normalizeGasRate(gasRate: number, gasRateUnits: string, chain: Chain): FeeRate { + // Convert gas_rate based on gas_rate_units to the expected unit for each chain type + // EVM clients expect values in gwei and will multiply by 10^9 to get wei + // UTXO clients expect satoshis per byte directly + + // First, try unit-based conversion for common patterns + switch (gasRateUnits) { + case 'gwei': + return gasRate // Already in gwei for EVM chains + case 'mwei': + return gasRate / 1e3 // Convert mwei to gwei (1 mwei = 0.001 gwei) + case 'centigwei': + return gasRate / 100 // Convert centigwei to gwei + case 'satsperbyte': + return gasRate // UTXO chains use this directly + case 'drop': + return gasRate // XRP uses drops + case 'uatom': + return gasRate // Cosmos chains use micro units + default: + // Fall back to chain-specific logic for nano units and special cases + break + } + + // Chain-specific handling for special cases + switch (chain) { + case 'AVAX': + // nAVAX = nano AVAX = 10^-9 AVAX = gwei equivalent + // Already in the right unit for EVM client + if (gasRateUnits !== 'nAVAX') { + console.warn(`Unexpected gas_rate_units for AVAX: ${gasRateUnits}`) + } + return gasRate + + default: + // For nano-prefixed units (nETH, nBSC, etc.), treat as gwei equivalent + if (gasRateUnits.startsWith('n') && gasRateUnits.length > 1) { + return gasRate // nano units = gwei equivalent for EVM chains + } + // For micro-prefixed units (uatom, etc.), return as-is for Cosmos chains + if (gasRateUnits.startsWith('u') && gasRateUnits.length > 1) { + return gasRate // micro units for Cosmos chains + } + break + } + + // If we reach here, log a warning but return the raw value + console.warn(`Unknown gas_rate_units "${gasRateUnits}" for chain ${chain}. Using raw value.`) + return gasRate + } protected async getFeeRateFromThorchain(): Promise<FeeRate> { const respData = await this.thornodeAPIGet('/inbound_addresses') if (!Array.isArray(respData)) throw new Error('bad response from Thornode API') const chainData: { chain: Chain gas_rate: string gas_rate_units?: string } = respData.find((elem) => elem.chain === this.chain && typeof elem.gas_rate === 'string') if (!chainData) throw new Error(`Thornode API /inbound_addresses does not contain fees for ${this.chain}`) const gasRate = Number(chainData.gas_rate) const gasRateUnits = chainData.gas_rate_units || '' - // Convert gas_rate based on gas_rate_units to the expected unit for each chain type - // EVM clients expect values in gwei and will multiply by 10^9 to get wei - // UTXO clients expect satoshis per byte directly - - // First, try unit-based conversion for common patterns - switch (gasRateUnits) { - case 'gwei': - return gasRate // Already in gwei for EVM chains - case 'mwei': - return gasRate / 1e3 // Convert mwei to gwei (1 mwei = 0.001 gwei) - case 'centigwei': - return gasRate / 100 // Convert centigwei to gwei - case 'satsperbyte': - return gasRate // UTXO chains use this directly - case 'drop': - return gasRate // XRP uses drops - case 'uatom': - return gasRate // Cosmos chains use micro units - default: - // Fall back to chain-specific logic for nano units and special cases - break - } - - // Chain-specific handling for special cases - switch (this.chain) { - case 'AVAX': - // nAVAX = nano AVAX = 10^-9 AVAX = gwei equivalent - // Already in the right unit for EVM client - if (gasRateUnits !== 'nAVAX') { - console.warn(`Unexpected gas_rate_units for AVAX: ${gasRateUnits}`) - } - return gasRate - - default: - // For nano-prefixed units (nETH, nBSC, etc.), treat as gwei equivalent - if (gasRateUnits.startsWith('n') && gasRateUnits.length > 1) { - return gasRate // nano units = gwei equivalent for EVM chains - } - // For micro-prefixed units (uatom, etc.), return as-is for Cosmos chains - if (gasRateUnits.startsWith('u') && gasRateUnits.length > 1) { - return gasRate // micro units for Cosmos chains - } - break - } - - // If we reach here, log a warning but return the raw value - console.warn(`Unknown gas_rate_units "${gasRateUnits}" for chain ${this.chain}. Using raw value.`) - return gasRate + return this.normalizeGasRate(gasRate, gasRateUnits, this.chain) }Then update
getFeeRateFromMayachainsimilarly to use the shared method.
165-206: Remove debug logging from production code.The debug log on line 182 should be removed or conditionally enabled only in development mode to avoid cluttering production logs.
- // Log for debugging - if (gasRateUnits) { - console.debug(`Mayachain gas_rate for ${this.chain}: ${gasRate} ${gasRateUnits}`) - } + // Only log in development mode + if (process.env.NODE_ENV === 'development' && gasRateUnits) { + console.debug(`Mayachain gas_rate for ${this.chain}: ${gasRate} ${gasRateUnits}`) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/fuzzy-ducks-mate.md(1 hunks)packages/xchain-client/src/BaseXChainClient.ts(3 hunks)packages/xchain-evm/__tests__/cache.test.ts(1 hunks)packages/xchain-evm/src/cache.ts(1 hunks)packages/xchain-evm/src/clients/client.ts(15 hunks)packages/xchain-evm/src/utils.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/fuzzy-ducks-mate.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xchain-evm/src/utils.ts
- packages/xchain-evm/src/clients/client.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/xchain-evm/__tests__/cache.test.ts (1)
packages/xchain-evm/src/cache.ts (1)
getCachedContract(37-43)
packages/xchain-client/src/BaseXChainClient.ts (2)
packages/xchain-client/src/types.ts (1)
FeeRate(145-145)packages/xchain-util/src/types/chain.ts (1)
Chain(1-1)
🪛 ESLint
packages/xchain-evm/__tests__/cache.test.ts
[error] 46-46: Insert ⏎
(prettier/prettier)
packages/xchain-evm/src/cache.ts
[error] 22-22: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 23-26: Replace ·providerIdentity._request?.url·||·⏎··························providerIdentity.connection?.url·||·⏎··························providerIdentity._url·||·⏎·························· with ⏎······providerIdentity._request?.url·||·providerIdentity.connection?.url·||·providerIdentity._url·||·
(prettier/prettier)
[error] 27-27: Replace .toString('base64').replace(/[^a-zA-Z0-9]/g,·'') with ⏎······.toString('base64')⏎······.replace(/[^a-zA-Z0-9]/g,·'')⏎······
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
packages/xchain-evm/src/cache.ts (1)
1-63: No missing await calls for getCachedContract
Verified that every usage of getCachedContract is awaited; no changes required.packages/xchain-client/src/BaseXChainClient.ts (2)
119-121: LGTM! The mwei to gwei conversion is now correct.The conversion factor has been properly fixed from 1e6 to 1e3, correctly converting mwei to gwei (1 gwei = 1,000 mwei).
135-142: Ignore enum suggestion
Chainin@xchainjs/xchain-utilis defined astype Chain = string; no enum or constants exist. Strict validation would require introducing a union type or constants in xchain-util.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xchain-evm/src/cache.ts (1)
1-84: Resolve remaining ESLint/Prettier errors
- Remove all
as anycasts (lines 17, 20, 39–46)- Refactor out the
asyncprovider.getNetwork()call (line 26)- Eliminate
Buffer.fromusage (line 49)
♻️ Duplicate comments (3)
packages/xchain-evm/src/cache.ts (3)
5-9: Scope cache by Provider using a WeakMap (prevents cross-network bleed and leaks).Current Map<string, Contract> is global and can retain contracts beyond a provider’s lifecycle; also contradicts the comment about per-provider isolation. Prefer WeakMap<Provider, Map<address, Contract>>.
-// Per-provider contract cache to ensure contracts are properly isolated -// Key format: `${networkName}_${chainId}_${providerInstanceId}_${address}` -const contractCache = new Map<string, Contract>() +// Per-provider contract cache to ensure contracts are properly isolated +let contractCacheByProvider: WeakMap<Provider, Map<string, Contract>> = new WeakMap() const bigNumberCache = new Map<string, BigNumber>()
80-83: Reset WeakMap on clear to actually drop provider partitions.WeakMap doesn’t support clear(); reassign instead.
-export function clearCaches(): void { - contractCache.clear() - bigNumberCache.clear() -} +export function clearCaches(): void { + contractCacheByProvider = new WeakMap() + bigNumberCache.clear() +}
13-23: Don’t mutate Provider instances; drop the symbol-based instance ID.Attaching Symbol.for('cache_instance_id') mutates third‑party objects and introduces eslint any violations. With a provider-scoped WeakMap, no mutation is needed.
-async function getContractCacheKey(address: string, provider: Provider): Promise<string> { - // Use provider instance reference as unique identifier to ensure - // different provider instances are cached separately even if they - // point to the same network - const providerInstanceId = (provider as any)[Symbol.for('cache_instance_id')] || - (() => { - const id = Math.random().toString(36).substring(2, 15); - (provider as any)[Symbol.for('cache_instance_id')] = id; - return id; - })(); +// No key helper needed; see provider-scoped cache below.
🧹 Nitpick comments (1)
packages/xchain-evm/src/cache.ts (1)
69-75: Hex-safe BigNumber parsing (optional).RPC values can be hex strings. Detect and parse base‑16; normalize cache key casing.
-export function getCachedBigNumber(value: string | number): BigNumber { - const stringValue = value.toString() - if (!bigNumberCache.has(stringValue)) { - bigNumberCache.set(stringValue, new BigNumber(stringValue)) - } - return bigNumberCache.get(stringValue)! -} +export function getCachedBigNumber(value: string | number): BigNumber { + const raw = value.toString() + const isHex = /^0x[0-9a-f]+$/i.test(raw) + const key = isHex ? raw.toLowerCase() : raw + if (!bigNumberCache.has(key)) { + bigNumberCache.set(key, isHex ? new BigNumber(raw.slice(2), 16) : new BigNumber(raw)) + } + return bigNumberCache.get(key)! +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xchain-evm/__tests__/cache.test.ts(1 hunks)packages/xchain-evm/src/cache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xchain-evm/tests/cache.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-evm/src/cache.ts (1)
packages/xchain-evm/src/index.ts (1)
abi(35-38)
🪛 ESLint
packages/xchain-evm/src/cache.ts
[error] 17-17: Replace ·(provider·as·any)[Symbol.for('cache_instance_id')]·||· with ⏎····(provider·as·any)[Symbol.for('cache_instance_id')]·||
(prettier/prettier)
[error] 17-17: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 19-19: Delete ;
(prettier/prettier)
[error] 20-20: Replace (provider·as·any)[Symbol.for('cache_instance_id')]·=·id; with ;(provider·as·any)[Symbol.for('cache_instance_id')]·=·id
(prettier/prettier)
[error] 20-20: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 21-21: Delete ;
(prettier/prettier)
[error] 22-22: Delete ;
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 37-37: Replace typeof·providerUnknown·===·'object'·&& with ⏎······typeof·providerUnknown·===·'object'·&&⏎·····
(prettier/prettier)
[error] 38-38: Replace ········'connection'·in·providerUnknown·&&· with ······'connection'·in·providerUnknown·&&
(prettier/prettier)
[error] 39-39: Replace ··typeof·(providerUnknown·as·any).connection?.url·===·'string' with typeof·(providerUnknown·as·any).connection?.url·===·'string'⏎····
(prettier/prettier)
[error] 39-39: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 40-40: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 43-43: Replace typeof·providerUnknown·===·'object'·&& with ⏎······typeof·providerUnknown·===·'object'·&&⏎·····
(prettier/prettier)
[error] 44-44: Replace ·············'url'·in·providerUnknown·&&· with ······'url'·in·providerUnknown·&&
(prettier/prettier)
[error] 45-45: Replace ·············typeof·(providerUnknown·as·any).url·===·'string' with ······typeof·(providerUnknown·as·any).url·===·'string'⏎····
(prettier/prettier)
[error] 45-45: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 46-46: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 48-48: Delete ····
(prettier/prettier)
[error] 49-49: Replace .toString('base64').replace(/[^a-zA-Z0-9]/g,·'') with ⏎······.toString('base64')⏎······.replace(/[^a-zA-Z0-9]/g,·'')⏎······
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xchain-evm/src/utils.ts (1)
123-126: Use ethers v6 estimateGas pattern and fix the typo.Prefer method’s estimateGas helper and correct variable naming.
Apply:
- const contract = getCachedContract(contractAddress, abi, provider) - const estiamtion = await contract.getFunction(funcName).estimateGas(...funcParams) - return getCachedBigNumber(estiamtion.toString()) + const contract = getCachedContract(contractAddress, abi, provider) + // In v6, methods expose estimateGas directly + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const estimation = await (contract as any)[funcName].estimateGas(...funcParams) + return getCachedBigNumber(estimation.toString())
🧹 Nitpick comments (11)
packages/xchain-evm/src/cache.ts (2)
21-29: Normalize with checksummed address (optional).Lowercasing works for keying, but using getAddress(address) yields a canonical checksummed key and rejects invalid inputs earlier.
Apply:
-import { Contract } from 'ethers' -import type { Provider, InterfaceAbi } from 'ethers' +import { Contract, getAddress } from 'ethers' +import type { Provider, InterfaceAbi } from 'ethers' - const normalizedAddress = address.toLowerCase() + const normalizedAddress = getAddress(address).toLowerCase()
49-53: Expose a per-provider clear to aid tests and long-lived apps.WeakMap can’t be cleared globally, but you can clear the nested Map for a given provider.
Apply:
export function clearCaches(): void { // Note: WeakMap doesn't have a clear() method, and that's by design // The contract cache will be automatically cleaned up when providers are GC'd bigNumberCache.clear() } + +export function clearProviderContracts(provider: Provider): void { + const byProvider = contractCacheByProvider.get(provider) + if (byProvider) byProvider.clear() +}packages/xchain-evm/src/utils.ts (2)
153-160: Dynamic call is fine; consider getFunction for stricter typing (optional).If you want runtime safety, you could resolve the method via getFunction and invoke it.
Apply:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (contract as any)[funcName](...funcParams) + return contract.getFunction(funcName)(...funcParams)
170-180: Drop unnecessary async wrapper.Function returns synchronously from cache.
Apply:
-export const getContract = async ({ +export const getContract = ({ provider, contractAddress, abi, -}: { +}: { provider: Provider contractAddress: Address abi: InterfaceAbi -}): Promise<Contract> => { - return getCachedContract(contractAddress, abi, provider) +}): Contract => { + return getCachedContract(contractAddress, abi, provider) }packages/xchain-evm/src/clients/client.ts (7)
29-29: Import Provider as a type-only import.It’s only used for types; avoid bundling runtime symbol.
Apply:
-import { Provider, Transaction, toUtf8Bytes, hexlify } from 'ethers' +import type { Provider } from 'ethers' +import { Transaction, toUtf8Bytes, hexlify } from 'ethers'
81-107: Prefer built-in Promise.any with a robust fallback.Use Promise.any when available; tighten error handling for non-Error rejections.
Apply:
async function promiseAny<T>(promises: Promise<T>[]): Promise<T> { - // Use Promise.race to get the first resolved promise - const errors: Error[] = [] - - return new Promise((resolve, reject) => { - let completedCount = 0 - - promises.forEach((promise) => { - promise.then(resolve).catch((error) => { - errors.push(error) - completedCount += 1 - if (completedCount === promises.length) { - reject(new Error('All promises failed: ' + errors.map((e) => e.message).join(', '))) - } - }) - }) - - if (promises.length === 0) { - reject(new Error('No promises provided')) - } - }) + if (promises.length === 0) throw new Error('No promises provided') + if ('any' in Promise) return (Promise as any).any(promises) + const errors: string[] = [] + return new Promise<T>((resolve, reject) => { + let pending = promises.length + for (const p of promises) { + p.then(resolve).catch((err) => { + const msg = err instanceof Error ? err.message : String(err) + errors.push(msg) + if (--pending === 0) reject(new Error('All promises failed: ' + errors.join(', '))) + }) + } + }) }
560-571: Redundant filter(Boolean) on promises.The mapper always returns a Promise; filtering is a no-op.
Apply (same for similar round-robin methods below):
- .filter(Boolean)
586-604: Same nit: remove filter(Boolean) and unify errors.Also applies here.
Apply:
- .filter(Boolean)
613-630: Same nit: remove filter(Boolean).Promises are always truthy.
- .filter(Boolean)
637-655: Same nit: remove filter(Boolean).And keep consistent error wording.
- .filter(Boolean)
342-349: Error message mismatch.You check for broadcastTransaction but the message mentions sendTransaction.
Apply:
- throw new Error('Provider does not support sendTransaction') + throw new Error('Provider does not support broadcastTransaction')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/xchain-evm/__tests__/cache.test.ts(1 hunks)packages/xchain-evm/src/cache.ts(1 hunks)packages/xchain-evm/src/clients/client.ts(15 hunks)packages/xchain-evm/src/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xchain-evm/tests/cache.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/xchain-evm/src/cache.ts (1)
packages/xchain-evm/src/index.ts (1)
abi(35-38)
packages/xchain-evm/src/utils.ts (2)
packages/xchain-util/src/asset.ts (1)
baseAmount(103-123)packages/xchain-evm/src/cache.ts (2)
getCachedBigNumber(37-43)getCachedContract(13-32)
packages/xchain-evm/src/clients/client.ts (4)
packages/xchain-evm/src/cache.ts (2)
getCachedBigNumber(37-43)getCachedContract(13-32)packages/xchain-client/src/types.ts (1)
FeeRates(150-150)packages/xchain-client/src/feeRates.ts (1)
standardFeeRates(19-25)packages/xchain-util/src/asset.ts (1)
baseAmount(103-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
packages/xchain-evm/src/cache.ts (1)
5-7: Good move: provider-scoped contract cache.Using a WeakMap keyed by Provider prevents cross-network bleed-through and enables GC. Solid choice.
packages/xchain-evm/src/utils.ts (3)
6-6: LGTM: switch to cached Contract/BigNumber utilities.This aligns with the new cache module and reduces allocations.
97-97: LGTM: approval amount helper uses cached BigNumber.Branching to MAX_APPROVAL remains intact.
242-246: LGTM: cached BigNumber and Contract usage in allowance path.Reduces churn and keeps behavior unchanged.
packages/xchain-evm/src/clients/client.ts (5)
378-385: LGTM: cache BigNumber for gas price path.Keeps conversions in one format and reduces allocations.
456-465: LGTM: ERC20 estimate uses cached contract and cached BigNumber.Pattern matches utils and reduces allocations.
479-480: LGTM: cache gas estimation result.Consistent BigNumber handling.
501-510: Nice: parallelize prices and limit estimation.Cuts latency on common hot path.
409-424: getFeeRateFromMayachain is already implemented and inherited by the EVM client
Defined in packages/xchain-client/src/BaseXChainClient.ts (line 165) and available on the EVM client via class inheritance — no further action needed.
| export function getCachedBigNumber(value: string | bigint): BigNumber { | ||
| const stringValue = typeof value === 'bigint' ? value.toString() : value | ||
| if (!bigNumberCache.has(stringValue)) { | ||
| bigNumberCache.set(stringValue, new BigNumber(stringValue)) | ||
| } | ||
| return bigNumberCache.get(stringValue)! | ||
| } |
There was a problem hiding this comment.
What is this cache trying to optimize? It seems that it only saves the BigNumber instance. Is this really a costly process?
| * Uses Promise.race with proper error handling as fallback for Promise.any | ||
| */ | ||
| async function promiseAny<T>(promises: Promise<T>[]): Promise<T> { | ||
| // Use Promise.race to get the first resolved promise |
There was a problem hiding this comment.
Would it make sense to use promise.any ?
Summary by CodeRabbit
New Features
Performance
Reliability
Tests