-
Notifications
You must be signed in to change notification settings - Fork 18
Update websocketChainReader #2383
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: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis PR extends the blockchain support in the ChainReader by adding Binance as a new blockchain type. It updates the enumeration with a new value, augments the configuration to include a Binance websocket URL, and modifies the client initialization logic to establish and store a Binance client connection. The changes ensure that the ChainReader can now handle Binance, alongside Ethereum and Kaia. Changes
Sequence Diagram(s)sequenceDiagram
participant CR as ChainReader
participant Cfg as ChainReaderConfig
participant Dial as Dial (WebSocket)
participant ChainMap as chainIdToChainType Map
Cfg->>CR: Provide configuration (BinanceWebsocketUrl included)
CR->>Dial: Check if BinanceWebsocketUrl is not empty
Dial-->>CR: Return connection/client and chain ID
CR->>ChainMap: Add Binance chain ID and type mapping
CR->>CR: Store BinanceClient in ChainReader
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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:
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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node/pkg/chain/websocketchainreader/websocketreader.go (2)
50-64
: Initialize Binance clientThis block closely parallels the setup for Ethereum, including proper error checks and chain ID retrieval. Consider adding a log statement confirming a successful Binance WebSocket connection to aid in debugging and operational visibility.
+ log.Info().Str("BinanceUrl", config.BinanceWebsocketUrl).Msg("Connected to Binance Websocket")
168-178
: Avoid panic in production codeThe default case currently calls
panic("invalid chainType")
, which can abruptly terminate your application. Consider returning a nil client or structuring the function to return an error instead, so callers can gracefully handle unexpected chain types.default: - panic("invalid chainType") + log.Error().Msgf("invalid chainType: %v", chainType) + return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/node.test.yaml
is excluded by!**/*.yaml
📒 Files selected for processing (2)
node/pkg/chain/websocketchainreader/type.go
(3 hunks)node/pkg/chain/websocketchainreader/websocketreader.go
(3 hunks)
🔇 Additional comments (5)
node/pkg/chain/websocketchainreader/type.go (4)
16-16
: Add enumerated chain typeIntroducing
Binance
in theBlockchainType
enum is consistent with existing naming patterns and helps maintain clarity for the new blockchain support.
20-23
: Expand the configAdding separate fields for each chain’s WebSocket URL maintains clarity and a straightforward configuration approach. If the user ever sets these fields to empty strings, consider whether a default or fallback behavior is necessary.
Could you confirm if there's any fallback logic or environment variable that might populate these fields when left blank?
40-44
: Consistent option constructorThis new
WithBinanceWebsocketUrl
function matches the existing pattern for specifying WebSocket URLs in other chains, ensuring a uniform and predictable API.
55-55
: Add new client fieldThe
BinanceClient
field aligns well with the existing client fields for Kaia and Ethereum. Ensure you have corresponding test cases or integration tests that verifyBinanceClient
is initialized and used correctly.Please verify whether the existing or new tests adequately cover the Binance client usage in
ChainReader
.node/pkg/chain/websocketchainreader/websocketreader.go (1)
85-85
: Store new clientAssigning
binanceClient
to theChainReader
struct improves clarity and maintainability. Ensure that an emptyBinanceWebsocketUrl
does not inadvertently trigger unnecessary client usage.Do you plan to guard against accidentally using
BinanceClient
ifBinanceWebsocketUrl
is empty?
Description
preparation for binance dex pool price subscription
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment