-
Notifications
You must be signed in to change notification settings - Fork 1
Sam/ws transport #12
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
base: main
Are you sure you want to change the base?
Sam/ws transport #12
Conversation
5bf8600 to
d4fde3b
Compare
d4fde3b to
0003e4a
Compare
- Use Pino for performant logging - Use Pino's .child() pattern for efficient scoped logging - Add pino-pretty support for human-readable development output - Disable logging in tests via NODE_ENV check - Automatically rename conflicting fields (time → time_, scope → scope_)
Remove the following plugins to reduce load: - abstract, amoy, arbitrum, avalanche, base - bobevm, celo, ethereumclassic, ethereumpow, fantom - filecoinfevm, filecoinfevmcalibration, holesky, hyperevm - pulsechain, rsk, sepolia, sonic
Add serviceKeysFromUrl utility that matches API keys to URLs by hostname
with subdomain fallback support (e.g., api.example.com → example.com).
Keys can be configured with or without ports.
- Add src/util/serviceKeys.ts with ServiceKeys type and matching logic
- Add src/util/replaceUrlParams.ts for {{param}} placeholder substitution
- Add src/util/authenticateUrl.ts to combine key lookup with URL templating
- Add comprehensive tests for serviceKeysFromUrl
Refactor RPC and scan adapter configuration:
- Switch to authenticated RPC endpoints using {{apiKey}} templates
- Make scanAdapters required in EvmRpcOptions
- Update EtherscanV1/V2 adapters to use serviceKeysFromUrl
- Fix pickRandom return type to T | undefined for empty arrays
- Improve error handling when no URLs or API keys are configured
This reduces complexity and re-uses the new infrastructure. The only trade-off is to remove the logging of the URL without the key, which we can accept since these logs should be treated as company sensitive.
0003e4a to
d9c897f
Compare
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
| logger.warn('No API key found for', host) | ||
| if (url == null) { | ||
| logger.error({ msg: 'No URLs for EtherscanV1ScanAdapter provided' }) | ||
| return false |
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.
Scan adapters return false on config error
Medium Severity
When pickRandom(urls) returns undefined (empty URLs array), the new null check returns false, indicating "no changes detected." This is incorrect because we couldn't actually verify the address state. Returning false may cause clients to miss real updates. The safer fail-safe behavior (consistent with HTTP errors on the first request which return true) would be to return true to prompt client-side verification.
Additional Locations (1)
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.
Yeah, if we can't connect to any server, we generally return true, as there might be changes.
swansontec
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.
Found a few small things.
| .then(result => { | ||
| if (result.subscribed) { | ||
| logger.log('Block connection initialized') | ||
| logger.info({ scope: 'foo' }, 'Block connection initialized') |
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.
Yup, { scope: 'foo' } needs to go.
| .then(() => initBlockConnection()) | ||
| .catch(err => { | ||
| console.error('Failed to re-initialize block connection:', err) | ||
| logger.error({ err }, 'Failed to re-initialize block connection') |
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.
Below we did ${String(error}. Here we log the object. Should we standardize on one of the other?
| pluginErrorCounter.inc({ pluginId, url: logUrl }) | ||
|
|
||
| logger.warn('WebSocket error:', error) | ||
| logger.warn(`WebSocket error: ${String(error)}`) |
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.
Above we did { err }. Here we log the stringified version. Should we standardize on one of the other?
| pluginLogger.error('message') | ||
|
|
||
| // Pass an object with extra fields: | ||
| pluginLogger.info({ ip: '1.2.3.4' }, 'connected') |
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.
You pass an object with a msg field in a ton of places, but your own docs suggest that this is the wrong approach. Should you be passing the msg field as a second string parameter? If so, that applies to a bunch of places.
| logger.warn('No API key found for', host) | ||
| if (url == null) { | ||
| logger.error({ msg: 'No URLs for EtherscanV1ScanAdapter provided' }) | ||
| return false |
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.
Yeah, if we can't connect to any server, we generally return true, as there might be changes.
| logger.warn({ host, msg: 'No API key found' }) | ||
| if (url == null) { | ||
| logger.error({ msg: 'No URLs for EtherscanV2ScanAdapter provided' }) | ||
| return false |
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.
Same here?
| const socketReady = new Promise<void>(resolve => { | ||
| ws.on('open', () => { | ||
| pluginConnectionCounter.inc({ pluginId, url: logUrl }) | ||
| pluginConnectionCounter.inc({ pluginId, url: url }) |
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.
url: url could be just url. There are a bunch of other places where this happens in this same commit.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Introduces structured JSON logging and auth templating, and enables WS RPC transports while simplifying supported networks.
docs/logging.md; replace console logs acrosssrc/index.ts,src/hub.ts,plugins/blockbook.ts,plugins/evmRpc.tswith structured eventsAddressHub.handleConnectionnow acceptsip; logspid/sid/ip, bulk subscribe/unsubscribe, updates and errors; tests updatedsrc/util/serviceKeys.ts,replaceUrlParams,authenticateUrl; switch config toserviceKeys(removenowNodesApiKey); adapters use keys resolved from URL host; add testsviem(createTransportwithws*/http*), improved error handling/logs, requirescanAdapters; internal trace fallbacks loggedurl; improved block/tx handling and reconnection logsauthenticateUrl; keepbitcoinfamily + select EVM chains (bsc,botanix,ethereum,optimism,polygon,zksync); remove many othersgetAddressPrefix, makepickRandomreturn optional; addpinodependency and Jest setup forserviceKeysmockWritten by Cursor Bugbot for commit d9c897f. This will update automatically on new commits. Configure here.