Feat/params update #11
Conversation
|
Warning Rate limit exceeded@leonz789 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces significant changes to the feeder management system, focusing on refactoring the initialization, configuration, and operational flow of price feeders. The modifications span across multiple files, including Changes
Sequence DiagramsequenceDiagram
participant Main as RunPriceFeeder
participant Feeders as Feeders
participant Feeder as Individual Feeder
participant Chainlink as Chainlink Fetcher
participant ExoClient as Exo Client
Main->>Feeders: NewFeeders()
Feeders->>Feeder: SetupFeeder()
Feeders->>Feeders: Start()
loop Event Processing
ExoClient->>Feeders: Trigger Event
Feeders->>Feeder: Update Price
Feeder->>Chainlink: Fetch Price
Chainlink-->>Feeder: Return Price
Feeder->>ExoClient: Submit Price
end
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
🧹 Nitpick comments (8)
cmd/types.go (6)
100-113: Consider adding JSON tags or docstrings to exported fields in FeederInfo.
If these fields are exposed outside this package or intended for marshaling, addingjson:"..."tags can improve clarity.
130-130: Method visibility suggests internal usage only.
RenamingnewFeedertocreateFeederor adding doc comments might improve clarity on its intended usage.
142-143: Late variable initialization is clear enough here.
However, consider grouping fetcher/submitter initialization logic together for readability.
308-319: Synchronization approach in Feeders
Instead of a single mutex for the entire struct, you could explore fine-grained locking for better scalability if the feeder set grows large.
322-335: Buffered channel rationale looks good.
Allowing a buffer prevents blocking the event loop but ensure buffer sizes are sufficient for high-throughput scenarios. Monitor for potential dropped updates if the channel saturates.
442-442: Blocking call for UpdateOracleParams.
While this approach is simple, note that feeding large param sets or frequent updates might slow other operations waiting on the same channel capacity. Consider non-blocking sends or handling partial updates.fetcher/chainlink/chainlink.go (1)
19-19: Suffix-based approach is straightforward.
However, consider normalizing case or clarifying that “USDT” must always be lowercase or the suffix check could fail.cmd/feeder_tool.go (1)
35-35: Commented-out loggerTagPrefix is now redundant.
You might safely remove it from here to avoid confusion, given it’s redefined intypes.go.-// loggerTagPrefix = "feed_%s_%d"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/feeder_tool.go(4 hunks)cmd/types.go(9 hunks)fetcher/chainlink/chainlink.go(1 hunks)
🔇 Additional comments (10)
cmd/types.go (5)
5-7: Imports seem appropriate and essential for synchronization and string manipulation.
17-18: Good use of a logger prefix constant.
This makes log filtering easier, especially when debugging multiple feeders.
115-126: Potential pointer safety consideration.
The function returns a struct referencing lastPrice and lastSent by value. Any concurrent updates to those fields in another goroutine might not reflect in the returned copy. Confirm whether you need real-time synchronization or an immutable snapshot is sufficient.
259-274: Validate concurrency for updateFeederParams.
Since this updates shared state (f.endBlock, f.startBaseBlock, etc.), verify that the single-threaded approach (goroutine queue) remains intact. If multiple param updates arrive in quick succession, ensure no updates are lost.
Line range hint 353-394: Long-running goroutine with select might cause partial blocking if channels fill.
Monitor channel capacities during peak load to ensure events (trigger, updatePrice, updateParams) do not stall each other. If necessary, implement back-pressure or expand channel buffers.
fetcher/chainlink/chainlink.go (1)
22-24: Appending baseCurrency might cause confusion if token already has a variant of “usdt”.
To avoid potential duplication or mismatch (e.g. “tokenusdtusdt”), confirm tokens are consistently lowercased and validated upstream.
cmd/feeder_tool.go (4)
31-32: Defining these constants is fine.
They’re concise and self-explanatory. Keep them updated if the private file name changes.
70-70: Using NewFeeders clarifies feeder setup.
Good shift from ad-hoc map usage to a structured approach.
85-85: SetupFeeder call is well-placed.
Ensures feeders are configured before starting the main loop. Confirm no concurrency race when reconfiguring feeders after initial start.
95-95: Updated logging approach is consistent.
Using logger.Error is preferred over standard prints for better observability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cmd/types.go (4)
17-17: Consider a more descriptive logger tag format.The current format
feed_%s_%dcould be more descriptive. Consider usingfeeder_token_%s_id_%dfor better clarity in logs.-const loggerTagPrefix = "feed_%s_%d" +const loggerTagPrefix = "feeder_token_%s_id_%d"
100-113: Add field documentation to exported struct.Since
FeederInfois now exported, consider adding documentation for each field to help API consumers understand their purpose.type FeederInfo struct { + // Source represents the price data source (e.g., Chainlink) Source string + // Token represents the token symbol Token string + // TokenID is the unique identifier for the token TokenID uint64 + // FeederID is the unique identifier for this feeder FeederID int // TODO: add check for rouleID, v1 can be skipped // ruleID + // StartRoundID is the initial round ID for this feeder StartRoundID int64 + // StartBaseBlock is the starting block height StartBaseBlock int64 + // Interval is the number of blocks between price updates Interval int64 + // EndBlock is the final block height (0 means no end) EndBlock int64 + // LastPrice contains the most recent price information LastPrice localPrice + // LastSent contains the last sent transaction information LastSent signInfo }
260-278: Improve error handling and validation in updateFeederParams.The error handling could be more specific and the validation more comprehensive:
- Use specific error types instead of generic errors
- Validate individual field values
- Add logging for parameter changes
+var ( + ErrNilParams = errors.New("oracle params is nil") + ErrInvalidFeederID = errors.New("invalid feeder ID in oracle params") +) func (f *feeder) updateFeederParams(p *oracletypes.Params) error { - if p == nil || len(p.TokenFeeders) < f.feederID+1 { - return errors.New("invalid oracle parmas") + if p == nil { + return ErrNilParams + } + if len(p.TokenFeeders) < f.feederID+1 { + return fmt.Errorf("%w: feeder ID %d", ErrInvalidFeederID, f.feederID) } - // TODO: update feeder's params tokenFeeder := p.TokenFeeders[f.feederID] + + // Log parameter changes if f.endBlock != int64(tokenFeeder.EndBlock) { + f.logger.Info("updating end block", "old", f.endBlock, "new", tokenFeeder.EndBlock) f.endBlock = int64(tokenFeeder.EndBlock) } if f.startBaseBlock != int64(tokenFeeder.StartBaseBlock) { + f.logger.Info("updating start base block", "old", f.startBaseBlock, "new", tokenFeeder.StartBaseBlock) f.startBaseBlock = int64(tokenFeeder.StartBaseBlock) } if f.interval != int64(tokenFeeder.Interval) { + f.logger.Info("updating interval", "old", f.interval, "new", tokenFeeder.Interval) f.interval = int64(tokenFeeder.Interval) } - if p.MaxNonce > 0 { + if p.MaxNonce > 0 && f.lastSent.maxNonce != p.MaxNonce { + f.logger.Info("updating max nonce", "old", f.lastSent.maxNonce, "new", p.MaxNonce) f.lastSent.maxNonce = p.MaxNonce } return nil }
383-398: Extract new feeder creation logic to a separate method.The feeder creation logic in the Start method is complex and could be extracted to improve readability and maintainability.
+func (fs *Feeders) createNewFeeder(tfID int, tf *oracletypes.TokenFeeder, params *oracletypes.Params) { + tokenName := strings.ToLower(params.Tokens[tf.TokenID].Name) + source := fetchertypes.Chainlink + + if fetchertypes.IsNSTToken(tokenName) { + nstToken := fetchertypes.NSTToken(tokenName) + if source = fetchertypes.GetNSTSource(nstToken); len(source) == 0 { + fs.logger.Error("failed to add new feeder, source of nst token is not set", "token", tokenName) + return + } + } + + feeder := newFeeder(tf, tfID, fs.fetcher, fs.submitter, source, tokenName, params.MaxNonce, fs.logger) + fs.feederMap[tfID] = feeder + feeder.start() +} // In the Start method: for tfID, tf := range params.TokenFeeders { if _, ok := existingFeederIDs[int64(tfID)]; !ok { - // create and start a new feeder - tokenName := strings.ToLower(params.Tokens[tf.TokenID].Name) - source := fetchertypes.Chainlink - if fetchertypes.IsNSTToken(tokenName) { - nstToken := fetchertypes.NSTToken(tokenName) - if source = fetchertypes.GetNSTSource(nstToken); len(source) == 0 { - fs.logger.Error("failed to add new feeder, source of nst token is not set", "token", tokenName) - } - } - feeder := newFeeder(tf, tfID, fs.fetcher, fs.submitter, source, tokenName, params.MaxNonce, fs.logger) - fs.feederMap[tfID] = feeder - feeder.start() + fs.createNewFeeder(tfID, tf, params) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/types.go(9 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/types.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/price-feeder#11
File: cmd/types.go:340-347
Timestamp: 2024-12-27T08:13:34.691Z
Learning: The 'locker' field in the 'Feeders' struct ensures concurrency safety. When 'SetupFeeder' is called after 'Start' has been invoked, it logs an error and refuses to proceed, effectively preventing concurrency problems.
update corresponding fields for existing feeders or setup and start new feeders when a 'updateParams' events is received.
Notable Changes
feedersSummary by CodeRabbit
New Features
Bug Fixes
Documentation