fix: filter unaccessible endpoints #1678
Conversation
WalkthroughThe changes involve modifications to the endpoint filtering logic in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js (2)
13-18: Consider enhancing the endpoint filtering logicThe current implementation could be improved in several ways:
- The regex pattern might be too strict and miss problematic variations
- The filtering logic could be more maintainable as a separate function
- Consider adding logging for filtered endpoints to help with debugging
Consider this refactoring:
+const isUnaccessibleEndpoint = (value) => { + // Filter out numeric endpoints (e.g., wss://123) due to rate limits + const isNumericEndpoint = /^wss:\/\/\d+/.test(value); + // Filter out onfinality endpoints due to access restrictions + const isOnfinalityEndpoint = value.includes('onfinality'); + + if (isNumericEndpoint || isOnfinalityEndpoint) { + console.debug(`Filtering out unaccessible endpoint: ${value}`); + return true; + } + return false; +}; const connections = endpoints.map(({ value }) => { - // Check if e.value matches the pattern 'wss://<any_number>' - // ignore due to its rate limits - if (/^wss:\/\/\d+$/.test(value) || (value).includes('onfinality')) { - return undefined; - } + if (isUnaccessibleEndpoint(value)) return undefined;
Line range hint
8-34: Consider architectural improvements for better maintainabilityThe current implementation could benefit from some architectural improvements:
- Consider extracting configuration values (timeout, patterns) to a config file
- Consider implementing a retry mechanism for failed connections
- Consider implementing connection pooling or caching for frequently used endpoints
Would you like assistance in implementing any of these architectural improvements?
packages/extension-polkagate/src/hooks/useEndpoints.ts (1)
Line range hint
19-19: Consider making the return type more specific.The current return type
DropdownOption[]could be more specific to indicate that it never returns null/undefined (as evidenced by the?? []fallback).-export function useEndpoints (genesisHash: string | null | undefined): DropdownOption[] +export function useEndpoints (genesisHash: string | null | undefined): readonly DropdownOption[]packages/extension-polkagate/src/util/utils.ts (1)
15-15: Consider Usingimport typeforWsProviderSince
WsProvidermay be used only for type annotations, consider importing it usingimport typeto avoid unnecessary runtime imports.Apply this diff if
WsProvideris used only as a type:-import { WsProvider } from '@polkadot/api'; +import type { WsProvider } from '@polkadot/api';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension-polkagate/src/hooks/useEndpoints.ts(2 hunks)packages/extension-polkagate/src/util/utils.ts(3 hunks)packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js(2 hunks)
🔇 Additional comments (4)
packages/extension-polkagate/src/hooks/useEndpoints.ts (2)
60-60: LGTM! The AUTO_MODE handling is correct.
The condition for adding AUTO_MODE is properly placed after endpoint filtering, ensuring it's only added when multiple valid endpoints are available.
39-45: LGTM! Consider adding fallback handling.
The new filtering logic effectively excludes problematic endpoints. The regex pattern for excluding numeric-only hostnames and the onfinality filter are well-implemented.
Let's verify the impact of these filters on available endpoints:
Consider adding a warning log when all endpoints for a chain are filtered out, to help identify chains that might need additional endpoint options:
const endpoints = allEndpoints?.filter((e) => e.value &&
!/^wss:\/\/\d+$/.test(e.value) &&
!e.value.includes('onfinality') &&
(
String(e.info)?.toLowerCase() === chainName?.toLowerCase() ||
String(e.text)?.toLowerCase()?.includes(chainName?.toLowerCase() ?? '')
)
);
+ if (endpoints?.length === 0) {
+ console.warn(`All endpoints were filtered out for chain: ${chainName}`);
+ }packages/extension-polkagate/src/util/utils.ts (2)
5-5: Optimize Type Imports with import type
Good use of import type for ApiPromise to ensure that only type information is imported without including runtime code. This helps in reducing the bundle size and optimizing the application.
20-20: Verify the Correctness of the Import Path for fastestEndpoint
Ensure that the fastestEndpoint function is correctly imported from './workers/utils' and that the module exports this function as expected.
Run the following script to verify the export of fastestEndpoint:
✅ Verification successful
Import Path for fastestEndpoint is Incorrect
The import path './workers/utils' is incorrect. The actual location of fastestEndpoint is at './workers/utils/fastestEndpoint.js', and it's re-exported through './workers/utils/index.ts'. The import should be from './workers/utils' (without .js extension) since the barrel file (index.ts) handles the re-export.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'fastestEndpoint' is exported from './workers/utils'
# Search for the export statement of 'fastestEndpoint' in 'utils.ts'
rg --type ts --no-filename 'export.*fastestEndpoint' ./workers/utils.ts
Length of output: 132
Script:
#!/bin/bash
# Let's first find the correct location of the file that exports 'fastestEndpoint'
fd -e ts -e js "utils" --type f
# Then search for the export of 'fastestEndpoint' in all typescript files
rg --type ts "export.*fastestEndpoint"
# Also search for any file containing 'fastestEndpoint' definition
ast-grep --pattern 'const fastestEndpoint = $_'
Length of output: 341
Script:
#!/bin/bash
# Let's check the content of the fastestEndpoint.js file
rg -A 5 "export" packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.js
# Also check if there's a .ts version
rg -A 5 "export" packages/extension-polkagate/src/util/workers/utils/fastestEndpoint.ts
# Check the directory structure to understand the import path
fd . packages/extension-polkagate/src/util/workers/
Length of output: 2417
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/util/utils.ts (1)
19-19: LGTM: Good architectural decision to use a workerMoving the endpoint selection logic to a worker is a good architectural decision as it:
- Prevents blocking the main thread during endpoint selection
- Improves UI responsiveness
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/utils.ts(3 hunks)
🔇 Additional comments (2)
packages/extension-polkagate/src/util/utils.ts (2)
5-5: LGTM: Type import is correctly added
The type-only import of ApiPromise is properly added and necessary for TypeScript type safety.
503-504:
Fix type suppression issues
The code still uses type suppressions that were addressed in a previous review. Please implement the suggested fix to properly type the API provider.
As previously suggested:
- const urls = endpoints.map(({ value }) => ({ value: value as string }));
- const { api, connections } = await fastestEndpoint(urls);
-
- // @ts-ignore
- // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
- const selectedEndpoint = api.registry.knownTypes.provider.endpoint as string;
+ const urls = endpoints.map(({ value }) => ({ value: String(value) }));
+ const { api, connections } = await fastestEndpoint(urls);
+ const selectedEndpoint = (api?.rpc?.provider as WsProvider).endpoint;Make sure to import the WsProvider type:
import type { WsProvider } from '@polkadot/api';
Summary by CodeRabbit
New Features
Bug Fixes
Documentation