refactor: optimize process for finding the fastest endpoint #2021
Conversation
WalkthroughReplaces the previous connections/closeWebsockets pattern: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Worker as Worker helper
participant Fastest as fastestEndpoint
participant Providers as WsProvider[xN]
participant Apis as ApiPromise[xN]
Note over Worker,Fastest: Worker requests fastest API for endpoints
Worker->>Fastest: fastestEndpoint(urls)
Fastest->>Providers: create WsProvider for each valid url
Fastest->>Apis: init ApiPromise(api, provider) in parallel
Fastest->>Fastest: Promise.any resolves fastest ApiPromise
Fastest->>Providers: disconnect non-selected providers (parallel)
Fastest-->>Worker: { api, selectedEndpoint }
Note right of Worker: Worker uses api, then calls api.disconnect() in cleanup
Worker->>Fastest: use api → finally: api.disconnect()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/util/misc.ts (1)
175-178: UpdateFastestConnectionTypetype definition and error return to includewebSocketproperty.The
fastestEndpointfunction atpackages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js:47returns{ api, selectedEndpoint: provider.endpoint, webSocket: provider }, butFastestConnectionTypeonly definesapiandselectedEndpoint. The error return infastestConnectionatmisc.ts:175-178must also includewebSocketto match the success case:
- Update
FastestConnectionTypeinpackages/extension-polkagate/src/util/types.tsto includewebSocket: WsProvider | undefined- Update the error return in
packages/extension-polkagate/src/util/misc.ts:175-178to includewebSocket: undefined
🧹 Nitpick comments (2)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
27-38: Consider logging provider disconnect errors during setup.While silently ignoring disconnect errors (line 35) is acceptable to prevent error propagation, consider logging these errors for debugging purposes. Failed connections can provide valuable insights into endpoint issues.
.catch((error) => { - provider.disconnect().catch(() => null); + provider.disconnect().catch((disconnectError) => { + console.warn(`Failed to disconnect provider ${provider.endpoint}:`, disconnectError); + }); throw error; })
43-48: Consider logging disconnect errors for better observability.While silently ignoring disconnect errors is acceptable for cleanup, logging them can help diagnose endpoint issues during development and production monitoring.
await Promise.all( providers.map((p) => - p === provider ? Promise.resolve() : p.disconnect().catch(() => null) + p === provider ? Promise.resolve() : p.disconnect().catch((error) => { + console.warn(`Failed to disconnect provider ${p.endpoint}:`, error); + }) ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/extension-polkagate/src/util/misc.ts(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js(4 hunks)packages/extension-polkagate/src/util/workers/utils/fastestApi.js(1 hunks)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
packages/extension-polkagate/src/util/workers/utils/fastestApi.js (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
fastestEndpoint(75-75)endpoints(72-72)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js (1)
getChainEndpoints(12-26)
packages/extension-polkagate/src/util/misc.ts (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
fastestEndpoint(18-18)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
fastestEndpoint(23-23)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
fastestEndpoint(34-34)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
fastestEndpoint(75-75)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
getBalances(18-56)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (5)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
fastestEndpoint(18-18)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
fastestEndpoint(23-23)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
fastestEndpoint(34-34)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
fastestEndpoint(75-75)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (7)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
fastestEndpoint(75-75)endpoints(72-72)packages/extension-polkagate/src/util/workers/utils/fastestApi.js (1)
endpoints(13-13)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
apiPromises(190-197)
packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (2)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-51)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (28)
packages/extension-polkagate/src/util/workers/utils/fastestApi.js (1)
15-15: LGTM! Simplified passthrough pattern.The direct return of
fastestEndpoint(endpoints)correctly delegates to the updated function, eliminating unnecessary destructuring and object reconstruction.packages/extension-polkagate/src/util/misc.ts (1)
171-171: LGTM! Simplified connection logic.The direct delegation to
fastestEndpoint(urls)correctly adopts the new return shape and eliminates redundant logic.packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (3)
5-5: LGTM! Import cleanup aligns with refactoring.The removal of
closeWebsocketsfrom imports is consistent with the new WebSocket lifecycle management pattern.
18-18: LGTM! Destructuring updated correctly.The change from
connectionstowebSocketaligns with the updatedfastestEndpointreturn shape.
46-46: LGTM! WebSocket cleanup with proper error handling.The direct call to
webSocket.disconnect().catch(console.error)correctly replaces the previouscloseWebsockets()pattern and includes appropriate error handling.packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (3)
8-8: LGTM! Import cleanup aligns with refactoring.The removal of
closeWebsocketsis consistent with the new WebSocket management pattern.
34-34: LGTM! Destructuring updated correctly.The change to
webSocketaligns with the updatedfastestEndpointreturn shape.
136-136: LGTM! Proper WebSocket cleanup.The cleanup logic correctly uses
webSocket.disconnect().catch(console.error).packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (4)
6-6: LGTM! Import cleanup aligns with refactoring.The removal of
closeWebsocketsis consistent with the new pattern.
19-19: LGTM! Improved naming accuracy.The change from plural
connectionsToBeClosedto singularconnectionToBeClosedbetter reflects that there's a single WebSocket connection being managed.
21-21: LGTM! Null check updated correctly.The condition properly checks for the singular
connectionToBeClosed.
49-49: LGTM! Proper WebSocket cleanup.The cleanup correctly uses
connectionToBeClosed.disconnect().catch(console.error).packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (5)
9-9: LGTM! Import cleanup aligns with refactoring.The removal of
closeWebsocketsis consistent with the new WebSocket management pattern.
75-75: LGTM! Destructuring updated correctly.The change to
webSocketaligns with the updatedfastestEndpointreturn shape.
87-88: LGTM! Clear connection lifecycle management.The added comment clarifies the connection lifecycle, and the cleanup properly disconnects the relay chain WebSocket before connecting to the People chain.
94-94: LGTM! Clear naming for People chain connection.The
peopleWebSocketnaming clearly distinguishes the People chain connection from the relay chain connection.
139-139: LGTM! Proper cleanup for People chain connection.The cleanup correctly disconnects the People chain WebSocket with error handling.
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
23-23: LGTM! Destructuring updated correctly.The change to
webSocketaligns with the updatedfastestEndpointreturn shape.
106-106: LGTM! Proper WebSocket cleanup.The cleanup correctly uses
webSocket.disconnect().catch(console.error).packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (2)
20-20: LGTM! Destructuring updated correctly.The change to
webSocketaligns with the updatedfastestEndpointreturn shape.
52-52: LGTM! Improved naming accuracy.The return field renamed from plural
connectionsToBeClosedto singularconnectionToBeClosedbetter reflects that there's a single WebSocket being returned.packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (4)
8-8: LGTM!The removal of
closeWebsocketsimport aligns with the refactored cleanup pattern where callers now usewebSocket.disconnect()directly.
194-194: LGTM!Correctly destructures the new return shape from
fastestEndpoint, extracting bothapiandwebSocketfor later use.
196-196: LGTM!Correctly propagates the
webSocketreference in the returned object, enabling proper cleanup in the finally block.
227-229: LGTM!The new cleanup pattern using
webSocket.disconnect()is cleaner and more direct than the previouscloseWebsockets()helper. Error handling is appropriate with caught errors logged to console.packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (3)
40-41: LGTM!Using
Promise.anyefficiently selects the fastest available connection. The function will throw anAggregateErrorif all providers fail, which is appropriate behavior for callers to handle.
50-50: LGTM!The return value correctly provides all necessary components: the connected API instance, the selected endpoint URL, and the WebSocket provider for cleanup. This aligns with the updated usage pattern across all callers.
13-25: Endpoint filtering is intentional and documented.The 'onfinality' exclusion is due to rate limit issues (documented in useEndpoints.ts), and 'light' endpoints are intentionally filtered because light client connections are handled separately via dedicated logic (see lightClient-connect.ts). This filtering pattern is consistent throughout the codebase and aligns with the architecture.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
6-11: Document the return type in JSDoc.The JSDoc describes the parameter but omits the return type. Adding
@returnsdocumentation would help callers understand the return shape, especially useful given the refactored structure.Apply this diff to add return type documentation:
/** * Connects to multiple endpoints and returns the fastest one. * Automatically disconnects all other providers. * * @param {{ value: string }[]} endpoints + * @returns {Promise<{ api: ApiPromise, selectedEndpoint: string, webSocket: WsProvider }>} */
13-21: Consider documenting the filtering criteria.The filtering logic excludes numeric-wss patterns, onfinality endpoints, and light endpoints, but the rationale isn't explained. Adding brief comments would improve maintainability.
For example:
// Filter invalid endpoints const validEndpoints = endpoints .map(({ value }) => value) .filter( (value) => - !/^wss:\/\/\d+$/.test(value) && - !value.includes('onfinality') && - !value.startsWith('light') + !/^wss:\/\/\d+$/.test(value) && // Exclude malformed numeric endpoints + !value.includes('onfinality') && // Exclude onfinality (e.g., rate-limited) + !value.startsWith('light') // Exclude light client endpoints );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
fastestEndpoint(75-75)endpoints(72-72)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
apiPromises(190-197)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (4)
27-28: LGTM!Creating all providers upfront is the correct approach for parallel endpoint probing.
30-38: LGTM!The pattern of attaching the provider reference to each API promise is elegant and ensures proper tracking. Error handling appropriately cleans up failed connections.
43-50: LGTM!The non-blocking cleanup approach is appropriate here. Disconnecting unused providers in the background prevents delays while ensuring proper resource management.
52-52: LGTM!The return structure provides callers with the API, transparency about which endpoint was selected, and direct control over the WebSocket lifecycle. This aligns well with the refactoring goals described in the PR.
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
199-200: Fix provider leak by movingPromise.allinside try-finally and usingPromise.allSettled.The code at line 199 calls
Promise.all(apiPromises)outside the try/finally block. If any promise rejects after others have already resolved and created provider connections,Promise.allwill reject before the try/finally is entered, leaving successful providers undisconnected—a resource leak.Use
Promise.allSettled(apiPromises)to collect all results, extract the fulfilled ones intoapis, re-throw if any rejection (preserving the "fail-all" behavior), and ensure the finally block (lines 226–228) always runs and disconnects all successfully created providers, even when some chains fail.packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
75-139: Resource leak: Providers not cleaned up in error path.The providers are not disconnected if an error occurs before their respective disconnect calls. This can leave WebSocket connections open indefinitely.
Consider wrapping cleanup in try-finally blocks to ensure providers are always disconnected:
const endpoints = getChainEndpoints(chainName); + let provider; + let peopleWebSocket; + try { - const { api, provider } = await fastestEndpoint(endpoints); + const result = await fastestEndpoint(endpoints); + provider = result.provider; + const api = result.api; console.log('getting validators information on ' + chainName); const [electedInfo, waitingInfo, currentEra] = await Promise.all([ api.derive.staking.electedInfo({ withClaimedRewardsEras: true, withController: true, withDestination: true, withExposure: true, withExposureMeta: true, withLedger: true, withNominations: true, withPrefs: true }), api.derive.staking.waitingInfo({ withClaimedRewardsEras: true, withController: true, withDestination: true, withExposure: true, withExposureMeta: true, withLedger: true, withNominations: true, withPrefs: true }), api.query['staking']['currentEra']() ]); console.log('electedInfo, waitingInfo, currentEra fetched successfully'); // Close the initial connection to the relay chain provider.disconnect().catch(console.error); + provider = null; // Start connect to the People chain endpoints in order to fetch identities console.log('Connecting to People chain endpoints...'); const peopleChainName = getPeopleChainName(genesisHash); const peopleEndpoints = getChainEndpoints(peopleChainName); - const { api: peopleApi, provider: peopleWebSocket } = await fastestEndpoint(peopleEndpoints); + const peopleResult = await fastestEndpoint(peopleEndpoints); + peopleWebSocket = peopleResult.provider; + const peopleApi = peopleResult.api; // ... rest of processing ... peopleWebSocket.disconnect().catch(console.error); + peopleWebSocket = null; const results = { eraIndex: Number(currentEra?.toString() || '0'), genesisHash, validatorsInformation: { elected: electedValidatorsInformation, waiting: waitingValidatorsInformation } }; port.postMessage(JSON.stringify({ functionName: 'getValidatorsInformation', results: JSON.stringify(results) })); } catch (e) { console.error('Something went wrong while fetching validators', e); port.postMessage(JSON.stringify({ functionName: 'getValidatorsInformation', results: null })); + } finally { + // Ensure cleanup even in error scenarios + if (provider) { + provider.disconnect().catch(console.error); + } + if (peopleWebSocket) { + peopleWebSocket.disconnect().catch(console.error); + } } }
♻️ Duplicate comments (3)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
18-24: HandlefastestEndpointfailures and guarantee provider cleanup withtry/finally.Now that this helper owns the
provider, any error afterfastestEndpoint(endpoints)(e.g. inmetadataFromApi, queries, ortoGetNativeToken) will skip theprovider.disconnect().catch(console.error)at the bottom, leaking the websocket; additionally,fastestEndpointitself can throw (no valid endpoints / all endpoints failing), and this function still doesn’t catch that or send a structured error viaport.postMessage. Wrapping everything afterconst { api, provider } = await fastestEndpoint(endpoints);in atry { … } finally { provider.disconnect().catch(console.error); }(and optionally adding acatchthat posts an error back on the port) would address both the leak and the previously raised error-handling concern for this worker.Also applies to: 103-107
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
5-6: Always disconnect provider and handlefastestEndpointerrors in this worker.This function now owns the
providerbut only disconnects it on the happy path; if anything throws afterfastestEndpoint(endpoints)(e.g. insidemetadataFromApi,toGetNativeToken, orgetAssets), the websocket stays open, and iffastestEndpointitself throws, the worker won’t send any message onport. Wrapping the main body afterconst { api, provider } = await fastestEndpoint(endpoints);in atry { … } finally { provider.disconnect().catch(console.error); }(and optionally adding acatchthat posts a structured error back viaport.postMessage) would both close the provider reliably and align with the earlier guidance about handlingfastestEndpointfailures in this worker.Also applies to: 18-19, 43-47
packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
32-36: Fix provider leak on early returns and add robust cleanup aroundfastestEndpoint.After
const { api, provider } = await fastestEndpoint(endpoints);, there are two early returns (!memberand!accounts) that post a result and thenreturnwithout ever hittingprovider.disconnect().catch(console.error);, so those paths will always leak a websocket; any thrown error before line 136 will do the same. Wrap the body afterfastestEndpointin atry { … } finally { provider.disconnect().catch(console.error); }so both early returns and error cases clean up, and (per the earlier review) consider adding acatchthat turnsfastestEndpointfailures into a structuredport.postMessageerror instead of letting the worker crash.A minimal structural change would look like:
- const { api, provider } = await fastestEndpoint(endpoints); - - console.log(`getPool is called for ${stakerAddress} on chain ${chainName}`); - … - port.postMessage(JSON.stringify({ functionName: 'getPool', results: JSON.stringify(poolInfo) })); - - provider.disconnect().catch(console.error); + const { api, provider } = await fastestEndpoint(endpoints); + + try { + console.log(`getPool is called for ${stakerAddress} on chain ${chainName}`); + … + port.postMessage(JSON.stringify({ functionName: 'getPool', results: JSON.stringify(poolInfo) })); + } finally { + provider.disconnect().catch(console.error); + }This preserves existing behavior while ensuring the provider is always disconnected, even on early returns.
Also applies to: 46-57, 64-70, 136-137
🧹 Nitpick comments (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
75-94: Consider consistent naming for provider variables.The relay chain provider is named
providerwhile the people chain provider is namedpeopleWebSocket. For consistency, considerpeopleProviderinstead, though the current naming is descriptive.- const { api: peopleApi, provider: peopleWebSocket } = await fastestEndpoint(peopleEndpoints); + const { api: peopleApi, provider: peopleProvider } = await fastestEndpoint(peopleEndpoints);And update the disconnect call accordingly:
- peopleWebSocket.disconnect().catch(console.error); + peopleProvider.disconnect().catch(console.error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js(4 hunks)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js
🧰 Additional context used
🧬 Code graph analysis (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-53)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (4)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-53)
packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-53)
packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-53)packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js (1)
getChainEndpoints(12-26)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-53)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (7)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
fastestEndpoint(75-75)endpoints(72-72)packages/extension-polkagate/src/util/workers/utils/fastestApi.js (1)
endpoints(13-13)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
apiPromises(190-197)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (3)
packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
8-9: Provider-based wiring for NFT endpoints looks consistent.Using
fastestEndpointper chain and returning{ api, chainName, formattedAddresses, originalAddresses, provider }aligns this helper with the new provider-centric lifecycle and enables centralized cleanup in thefinallyblock; the shape is consistent with other helpers consumingfastestEndpoint.Also applies to: 189-197
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
13-52: fastestEndpoint’s provider-centric implementation looks solid; ensure callers handle its error modes.The new flow (filter valid endpoints → instantiate
WsProviders → createApiPromiseinstances paired with providers →Promise.anyto select the fastest and disconnect the rest) is clean and correctly centralizes lifecycle management around a singleprovider. The main remaining contract is that callers must still handle thrown errors (no valid endpoints, or all connection attempts failing) and thatselectedEndpoint: provider.endpointmatches theWsProviderAPI for the @polkadot/api version in use; if either assumption changes, callers should be adjusted rather than letting failures bubble unhandled from workers.packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
9-9: LGTM! Import correctly updated for provider-based lifecycle.The removal of
closeWebsocketsimport aligns with the new pattern whereprovider.disconnect()is called directly.
|
Why not use |
… returning provider
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
40-48: Existing concern about caller error handling remains valid.The use of
Promise.anywith proper error wrapping is correct. However, as noted in the past review comment, several callers still lack try-catch blocks to handle the case where all endpoints fail:
getAssetOnAssetHub.js(line 18)getAssetOnMultiAssetChain.js(line 23)getBalances.js(line 20)I've flagged these in separate comments above.
Consider documenting the error behavior in the JSDoc:
/** * Connects to multiple endpoints and returns the fastest one. * Automatically disconnects all other providers. * * @param {{ value: string }[]} endpoints + * @returns {Promise<{ api: import('@polkadot/api').ApiPromise, selectedEndpoint: string }>} + * @throws {Error} When no valid endpoints are provided or all endpoints fail to connect */ export async function fastestEndpoint (endpoints) {
🧹 Nitpick comments (1)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
18-20: Consider adding error handling at this level.While
getAssetOnRelayChain.jswraps this function in a try-catch, adding error handling here would make the function more robust and prevent potential issues if called from other contexts.export async function getBalances (chainName, addresses, userAddedEndpoints, port) { + try { const chainEndpoints = getChainEndpoints(chainName, userAddedEndpoints); const { api } = await fastestEndpoint(chainEndpoints); if (api.isConnected && api.derive.balances) { const { metadata } = metadataFromApi(api); console.info(chainName, 'metadata : fetched and saved.'); port.postMessage(JSON.stringify({ functionName: FETCHING_ASSETS_FUNCTION_NAMES.RELAY, metadata })); const requests = addresses.map(async (address) => { const allBalances = await api.derive.balances.all(address); const systemBalance = await api.query['system']['account'](address); const existentialDeposit = api.consts['balances']['existentialDeposit']; const balances = { ...allBalances, ED: existentialDeposit, // @ts-ignore frozenBalance: systemBalance.data.frozen }; const { pooled, soloTotal } = await getStakingBalances(address, api); return { address, balances, poolName: pooled?.poolName, poolReward: pooled?.poolReward ?? BN_ZERO, pooledBalance: pooled?.pooledBalance ?? BN_ZERO, soloTotal }; }); return { api, balanceInfo: await Promise.all(requests) }; } return undefined; + } catch (error) { + console.error(`getBalances: Error fetching balances for ${chainName}:`, error); + return undefined; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js(2 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js(3 hunks)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js(4 hunks)packages/extension-polkagate/src/util/workers/utils/fastestApi.js(0 hunks)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(1 hunks)packages/extension-polkagate/src/util/workers/utils/index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/extension-polkagate/src/util/workers/utils/index.ts
- packages/extension-polkagate/src/util/workers/utils/fastestApi.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js
- packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js
🧰 Additional context used
🧬 Code graph analysis (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (3)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
getBalances(18-56)packages/extension-polkagate/src/util/workers/sharedWorker.js (1)
port(20-20)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
api(41-41)
packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (5)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
fastestEndpoint(18-18)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
fastestEndpoint(23-23)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (1)
fastestEndpoint(34-34)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
fastestEndpoint(75-75)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (1)
fastestEndpoint(12-60)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (6)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (2)
fastestEndpoint(18-18)endpoints(17-17)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (2)
fastestEndpoint(23-23)endpoints(22-22)packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
fastestEndpoint(20-20)packages/extension-polkagate/src/util/workers/shared-helpers/getPool.js (2)
fastestEndpoint(34-34)endpoints(32-32)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
fastestEndpoint(75-75)endpoints(72-72)packages/extension-polkagate/src/util/workers/shared-helpers/getNFTs.js (1)
apiPromises(190-197)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
fastestEndpoint(12-60)api(41-41)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
fastestEndpoint(12-60)api(41-41)
packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
fastestEndpoint(12-60)api(41-41)packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js (1)
getChainEndpoints(12-26)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (10)
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
46-46: LGTM: Cleanup now uses api.disconnect().The migration from
closeWebsockets(connections)toapi.disconnect()aligns with the refactoredfastestEndpointthat manages provider cleanup internally. This addresses the concern raised in PR comments about avoiding the need to send providers to workers for disconnection.packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (2)
18-23: LGTM: Proper error handling in place.This function correctly wraps the
getBalancescall (which internally callsfastestEndpoint) in a try-catch block. The updated destructuring to{ api, balanceInfo }and validation logic align with the refactored return shape.
49-49: LGTM: Cleanup migrated to api.disconnect().Consistent with the refactor pattern across the PR, replacing
closeWebsocketswith direct API disconnection.packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
106-106: LGTM: Cleanup now uses api.disconnect().The migration from
closeWebsockets(connections)toapi.disconnect()is consistent with the PR's refactoring objectives.packages/extension-polkagate/src/util/workers/shared-helpers/getBalances.js (1)
52-52: LGTM: Return shape updated correctly.The updated return shape
{ api, balanceInfo }aligns with the removal of connection tracking, simplifying the API surface.packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (2)
74-88: LGTM: Proper error handling and cleanup for relay chain connection.The function correctly wraps the
fastestEndpointcall in a try-catch block and properly disconnects the relay chain API before connecting to the People chain. The updated destructuring to{ api }aligns with the refactored return shape.
94-139: LGTM: Proper cleanup for People chain connection.The People chain API is correctly destructured and disconnected after processing identities. The pattern is consistent with the relay chain cleanup above.
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (3)
13-25: LGTM: Robust upfront validation.The early filtering of invalid endpoints (numeric WSS addresses, OnFinality endpoints, and light clients) with explicit validation prevents downstream issues. Throwing an error when no valid endpoints remain provides clear feedback to callers.
27-38: LGTM: Clever provider tracking with proper error handling.Wrapping each
ApiPromise.createto maintain the{ api, provider }link enables identifying which provider succeeded. The error handling correctly disconnects failed providers and re-throws to ensurePromise.anycan try remaining endpoints.
50-59: LGTM: Non-blocking cleanup of unused providers.The parallel disconnection of non-selected providers is a good design choice. Using non-blocking cleanup (caught and ignored errors) prevents cleanup failures from affecting the returned API. Returning
selectedEndpointalongsideapiprovides useful debugging information.
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
Refactor
Chores