-
Notifications
You must be signed in to change notification settings - Fork 10
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
add: set logger to price-feeder #265
Conversation
WalkthroughThe pull request updates the Exocore application at multiple levels. The command root now integrates a logger when starting the price feeder and sets explicit state synchronization parameters. Dependencies in the module file have been revised with several version updates and replace directives. Additionally, the local node setup script has been modified to adjust token feeder parameters, introduce new configuration fields, and update RPC as well as debugger settings for improved configuration management and initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command Line
participant PreRun as startCmd.PreRunE
participant Logger as serverCtx.Logger
participant PF as PriceFeeder
CLI->>PreRun: Execute start command
PreRun->>PF: Invoke StartPriceFeeder(logger)
PF->>Logger: Log operation events
sequenceDiagram
participant Script as local_node.sh
participant YAML as Config Files (oracle_env, oracle_feeder, genesis)
participant Node as Exocore Node
Script->>YAML: Update configuration (nstid, token structure, start_base_block)
YAML-->>Script: Return updated config data
Script->>Node: Initialize and start node with new parameters
Node-->>Script: Node running with metrics and updated logging
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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
🧹 Outside diff range and nitpick comments (1)
cmd/exocored/root.go (1)
164-165
: LGTM! Good enhancement to logging.The addition of server context for structured logging is a good improvement. The logger is properly configured with the module name "price-feeder".
Consider adding log level configuration for the price-feeder module specifically, allowing for fine-grained control over logging verbosity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/exocored/root.go
(1 hunks)go.mod
(3 hunks)local_node.sh
(2 hunks)proto/exocore/delegation/v1/query.proto
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- proto/exocore/delegation/v1/query.proto
🔇 Additional comments (4)
go.mod (2)
56-56
: LGTM! Aligned with logging improvements.
The addition of go.uber.org/zap
as an indirect dependency aligns with the logging enhancements in cmd/exocored/root.go
.
10-10
: Verify the price-feeder dependency version.
The price-feeder dependency has been updated to a development version (v0.0.0-20241211184332-72b02c56c68d
) from a release version (v0.1.15
). This could introduce instability.
local_node.sh (2)
131-131
: Verify impact of reduced start_base_block.
The start_base_block
parameter has been reduced from 20 to 15 blocks. While this speeds up the start of token feeders, ensure this provides sufficient blockchain stability.
Consider documenting the rationale for choosing 15 blocks as the starting point and any potential implications for oracle reliability.
234-235
: LGTM! Well-structured oracle configuration.
The addition of the nstid
field with a properly formatted Ethereum address enhances the oracle configuration.
b78193a
to
1413a38
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cmd/exocored/root.go (1)
149-151
: 💡 Verification agent🧩 Analysis chain
Address the TODO comment regarding EVMOS dependency.
The comment indicates a temporary workaround for a version mismatch. This should be resolved once the price feeder is updated to use EVMOS v16.
🏁 Script executed:
#!/bin/bash # Check the current EVMOS version in price-feeder grep -r "github.com/evmos/evmos/" $(find . -name go.mod)Length of output: 206
Update on Price Feeder and EVMOS Dependency Handling
- We verified via the
go.mod
entries that the project now references EVMOS v16. In particular, the dependency is defined as a standard v16 (v16.0.0) with a replacement mapping to the Exocore fork atv16.0.3-0.20240828081344-d5cfcd34a812
.- This confirms that the intended update toward EVMOS v16 is in place. However, the price feeder remains temporarily disabled per the TODO comment in
cmd/exocored/root.go
(lines 149–151).- Actionable Suggestions:
- Revisit the temporary disablement of the price feeder. If the updated EVMOS dependency (via the Exocore fork) now resolves the version mismatch issue, re-enable the price feeder.
- While you're at it, consider improving the error handling in the price feeder initialization to include more specific error messages. This will help in diagnosing any future issues related to dependency mismatches or initialization failures.
🧹 Nitpick comments (1)
cmd/exocored/root.go (1)
156-162
: Enhance error message in panic recovery.Consider adding more context to the error message to help with debugging.
-fmt.Println("price-feeder failed", err) +fmt.Printf("price-feeder initialization failed: %v\n", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/exocored/root.go
(1 hunks)go.mod
(3 hunks)local_node.sh
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-unit-e2e
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
cmd/exocored/root.go (1)
164-165
: LGTM! Enhanced logging for price feeder.The addition of structured logging with module context improves observability.
go.mod (2)
270-286
: LGTM! Security and compatibility improvements in replace directives.The updates address security advisories and ensure compatibility with the latest versions.
11-11
:❓ Verification inconclusive
Verify compatibility with the new price-feeder version.
The update to price-feeder v0.2.3 and addition of zap logger align with the logging improvements.
Also applies to: 61-61
🏁 Script executed:
#!/bin/bash # Check for breaking changes in price-feeder v0.2.3 curl -s https://api.github.com/repos/ExocoreNetwork/price-feeder/releases/tags/v0.2.3 | jq -r '.body'Length of output: 106
Dependency and Compatibility Verification
- The upgrade from price-feeder v0.1.15 to v0.2.3 aligns with the intended PR objectives. However, since fetching the release notes for v0.2.3 returned null, there are no documented breaking changes from that source. Please verify via the official documentation or through integration tests to ensure compatibility.
- The addition of the zap logger dependency (line 61) and the updated replace directives (lines 270–286) support the required logging improvements and overall security adjustments.
local_node.sh (4)
130-130
: Verify impact of reduced start_base_block.The change from 20 to 15 blocks could affect token feeder synchronization. Please ensure this doesn't impact data consistency.
233-238
: LGTM! Improved token configuration structure.The new format with explicit token and sources fields improves readability.
244-248
: LGTM! Enhanced debugging capabilities.The addition of debugger configuration and explicit RPC/WebSocket settings improves operability.
260-261
: LGTM! Added beacon chain identifier.The nstid configuration is properly formatted and enables beacon chain integration.
go mod download works fine locally with either: |
@leonz789 rebase & resolve the confict |
supersed by #315 |
Description
tasks
Closes #XXX
Summary by CodeRabbit
New Features
Chores