Feat/debug tools #12
Feat/debug tools #12
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a comprehensive debugging and price submission system for a blockchain price feeder application. The changes span multiple files and add new CLI commands, gRPC services, and enhanced configuration management. The primary focus is on creating a robust debugging interface that allows users to send price messages, listen for block events, and interact with the price submission service through a structured command-line and gRPC-based approach. Changes
Sequence DiagramsequenceDiagram
participant CLI as Debug CLI
participant Server as Price Submit Server
participant ExoClient as Exo Blockchain Client
CLI->>Server: SubmitPrice(PriceMsg)
Server->>ExoClient: Prepare Transaction
ExoClient-->>Server: Transaction Prepared
Server-->>CLI: Submit Price Response
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (16)
exoclient/client.go (1)
76-77: Enforce debug endpoint ontxOnlymode.
The early return on missingendpointDebugfor debug mode is appropriate. Consider returning a wrapped error that indicates the caller must provide theendpointDebugfor clarity.- return nil, errors.New("rpc endpoint is empty under debug mode") + return nil, fmt.Errorf("no debug endpoint provided while txOnly=true; please set endpointDebug")types/types.go (2)
50-50: Typo check onGprc.
The fieldGprcmight be a typo forGrpc. Consider renaming for clarity:- Gprc string `mapstructure:"grpc"` + Grpc string `mapstructure:"grpc"`
163-169: Improved error checks inInitConfig.
The new error checks for empty or nonexistent config files are appropriate. Consider logging these errors before returning them to help debugging in a production environment.if len(cfgFile) == 0 { - return nil, errors.New("empty file name") + logger.Debug("Config file name is empty") + return nil, errors.New("empty config file name") }exoclient/tx.go (1)
22-24: Error propagation inSendTx.
The updated code returnsnil, errifgetSignedTxBytesfails. Consider adding context for debugging, such as the feeder ID or partial message info, to ease troubleshooting.- return nil, err + return nil, fmt.Errorf("SendTx -> getSignedTxBytes error for feederID:%d, baseBlock:%d, %w", feederID, baseBlock, err)cmd/feeder_debug.go (4)
35-39:newServerhelper.
Creates a server with minimal overhead. Looks good. Add doc comments if this function is externally used, so devs know the purpose and lifecycle of the returned server instance.
41-49:PriceJSON: in sync withPriceMsg.
PriceJSONfields overlap with those indebugger.PriceMsg. Confirm that struct duplication is intended. If so, keep them consistent or consider merging them if they serve the same role.
78-84: Status code usage.
Defining customstatusCodeconstants can help clarify different outcomes. Consider if these codes are used widely enough to be part of a separate error or status package for standardization.
94-181:DebugPriceFeederfunction.
- Good usage of a retry loop for exoclient initialization.
- The in-memory
pendingReqsstructure might grow large if block times or usage are high. Consider pruning it or applying capacity bounds for stability.- Graceful shutdown: consider a signal or context-based approach to terminate the goroutine that processes blocks.
Overall, this provides a robust debug environment.
proto/debugger/v1/service.proto (1)
1-3: Ensure clear package ownership and naming convention
Thepackage proto.debugger.v1;is clear and versioned. Verify that this path aligns with the repository’s domain naming conventions to avoid future conflicts or confusion.cmd/root.go (2)
32-37: PersistentPreRunE for config loading is robust
Good approach to load the config once and propagate errors early. Consider fallback or default behaviors if the config is missing or partially formed.
64-67: Commands grouped effectively
You’ve consolidatedstartCmdanddebugStartCmdunder root, which is logical. Ensure consistent help text and usage examples are provided.cmd/debug.go (3)
20-28: Consider grouping flags in a single place
FlagsfeederIDandheightappear in multiple subcommands. For maintainability, consider centralizing their definitions if they’re shared across commands.
72-94: Immediate send command is well-structured
debugSendImmCmdparallelsdebugSendCmdwith immediate execution. Good code reuse pattern. Consider combining if logic remains identical except for timing.
96-102: Proto printer is helpful for debugging
printProtofunction is straightforward. Confirm that large or repeated prints won’t clutter logs in production.cmd/feeder_tool.go (1)
39-39: Validate error handling for component initialization.
The panic is acceptable for critical failures, but consider returning a friendlier error message if there might be a recovery path.exoclient/types.go (1)
418-418: Check for consistent naming betweenGprcandGrpc.
The parameter list usesconfExocore.GprcandconfExocore.Rpc. IfGprcis a typo forGrpc, it’s best to rename for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
debugger/service.pb.gois excluded by!**/*.pb.godebugger/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
cmd/debug.go(1 hunks)cmd/feeder_debug.go(1 hunks)cmd/feeder_tool.go(3 hunks)cmd/root.go(4 hunks)cmd/start.go(2 hunks)cmd/types.go(1 hunks)debugger/price_msg.go(1 hunks)exoclient/client.go(5 hunks)exoclient/tx.go(3 hunks)exoclient/types.go(2 hunks)external/feeder.go(0 hunks)proto/debugger/v1/service.proto(1 hunks)types/types.go(5 hunks)
💤 Files with no reviewable changes (1)
- external/feeder.go
🔇 Additional comments (34)
exoclient/client.go (6)
4-4: Consider adding context to errors import.
The newly added "errors" import is fine. However, it might be beneficial to clarify if any standard library error wrapping (fmt.Errorf) or sentinel errors from "errors" are needed here.
14-14: Dependency check for rpchttp.
The import rpchttp "github.com/cometbft/cometbft/rpc/client/http" appears to be newly introduced. Please ensure that this dependency is listed in go.mod and is compatible with the rest of the tooling in the repository.
38-39: Separation of txClient and txClientDebug.
Adding txClientDebug for debug functionality is a good approach. Keep in mind to document clearly when external code should use txClient vs. txClientDebug, to avoid confusion and ensure no cross usage in production code.
60-60: Extended constructor signature.
The change to include endpointDebug string and txOnly bool in the constructor is logical. Confirm that all call sites are updated to avoid signature mismatch.
79-83: Error propagation correctness.
If the client creation fails at line 80, you wrap the error at line 82. This is a good best practice. Ensure logs also capture the error details for easier diagnosis.
85-115: Conditional connection logic for gRPC & WebSocket.
The if !txOnly block cleanly separates normal usage from debug transactions. A few considerations:
- Verify there is no scenario where
endpointorwsEndpointis empty iftxOnlyis false. - Ensure that setting both debug and normal endpoints simultaneously will not cause unintended conflicts or resource usage.
Otherwise, this separation of concerns is well structured.
types/types.go (3)
4-4: New error usage.
Importing "errors" is consistent with the new error checks. Good usage for returning sentinel errors in InitConfig.
154-154: Global viper instance.
The declaration var v *viper.Viper sets a package-level global. This is acceptable for a single-configuration workflow, but be mindful if concurrency or multiple config contexts are needed.
184-184: Return pointer usage.
Returning *Config instead of Config is beneficial for passing references around the codebase. This makes the function more flexible and aligns with typical usage patterns.
exoclient/tx.go (3)
14-14: Coretypes import usage.
Import of coretypes "github.com/cometbft/cometbft/rpc/core/types" addresses the debug broadcast. Ensure version compatibility with this package if it's newly introduced.
40-51: Introducing SendTxDebug.
This method is a clean extension for debug usage. Keep in mind to handle partial failures consistently, and test thoroughly under scenarios with slow networks or repeated calls to ensure no concurrency issues with txClientDebug.
107-143: In getSignedTxBytes, broaden validations on price.
While the logic for building and signing MsgCreatePrice is correct, consider verifying that price.Price or price.Decimal are not zero or invalid. Early validation helps avoid signing invalid transactions.
cmd/feeder_debug.go (7)
1-9: New debugging CLI logic.
This new file sets up core structures (server, PriceJSON, etc.) for debugging. Good initial approach. Ensure logs or usage examples are well documented.
18-21: server struct definition.
The unexported sendCh field is fine for encapsulation. If you'd like external packages to inject a channel directly, consider whether an interface-based approach is more flexible. For now, this is acceptable.
23-33: SubmitPrice concurrency.
Pushing requests to s.sendCh and waiting on result is straightforward. Check that the buffer size is sufficient under high concurrency to avoid blocking the server.
50-57: getPriceInfo function.
This function clarifies how the JSON fields map to internal PriceInfo. If user input can contain invalid values (e.g., negative decimals), add checks or validations.
69-77: sendReq struct usage.
Storing request data plus a result channel is a neat approach. Some might prefer context-based solutions or an ID-based callback mechanism. Nonetheless, this approach is clear and straightforward.
86-92: DebugRetryConfig.
Having a separate retry config is helpful. Ensure that future expansions of retry logic (e.g., backoff strategies) remain consistent across debug and production code if needed.
183-199: sendTx function with gRPC dial.
Dialing locally at localhost:50051 is appropriate for debug. Add a short comment about whether the user can override the port. Also, consider reusing the connection instead of connecting on each call in higher-throughput scenarios.
debugger/price_msg.go (1)
1-12: GetPriceInfo method.
This straightforward mapping from PriceMsg to PriceInfo is well structured. Verify that any future fields added to PriceMsg are also updated here.
proto/debugger/v1/service.proto (4)
5-7: RPC definition looks good
The PriceSubmitService service with a SubmitPrice RPC is coherent and follows a straightforward request-response pattern.
9-16: Consider validating fields at JSON registration level
PriceMsg includes fields like price, det_id, decimal, and so on; consider how you will handle invalid or missing values on the receiving side.
18-22: Consider optional vs. required fields in your request
SubmitPriceRequest may need enforced validation rules for height, feeder_id, and price. Confirm that all fields are consistently set before proceeding.
24-32: Response message is comprehensive
The SubmitPriceResponse includes logs and error strings, which is useful for debugging. Ensure logs don’t expose sensitive details.
cmd/start.go (2)
12-12: Private validator key file constant is well-defined
The privFile constant helps avoid hard-coded strings. Confirm that the file path is configurable if needed across different environments.
26-30: Error-aware RunE method is a good update
Switching from Run to RunE allows graceful error handling. Ensure any possible errors within RunPriceFeeder are suitably surfaced or logged.
cmd/root.go (1)
13-17: Configuration variables are well-scoped
Global usage of feederConfig may simplify code but ensure concurrency and thread-safety if multiple goroutines access configuration simultaneously.
cmd/debug.go (2)
30-39: Debug command for block listening is clear
debugStartCmd logic is concise and uses the new logger. Confirm that all critical errors in DebugPriceFeeder are captured, especially with network timeouts.
41-70: Deserialization logic is correct but watch for JSON format inconsistencies
When unmarshaling PriceMsg, ensure you handle malformed or unexpected fields gracefully.
cmd/feeder_tool.go (3)
33-33: Use of pointer to config improves memory efficiency.
Switching from a value parameter to a pointer for conf avoids unnecessary copies. Just ensure no unexpected side effects occur when conf is modified elsewhere.
157-157: Confirm that mnemonic and privFile are in scope.
The initComponents function references mnemonic and privFile inside, but they are not function parameters. Ensure that these variables are properly declared in the outer scope or consider passing them explicitly to improve clarity.
168-168: Confirm txOnly is correctly handled.
The call to exoclient.Init(conf, mnemonic, privFile, false, standalone) passes false for txOnly. Verify if certain scenarios require this flag to be set to true.
exoclient/types.go (1)
359-359: New function signature ensures more flexible initialization.
Accepting a pointer to feedertypes.Config plus a txOnly boolean allows finer control over transaction-related operations. Make sure all call sites reflect this update.
cmd/types.go (1)
16-19: New constant statusOk is a clear addition for transaction status checks.
The dedicated constant clarifies success status codes.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
types/types.go (1)
Line range hint
179-184: Replace panic with error return in Unmarshal error handling.The function returns errors in other cases but panics on Unmarshal error, which is inconsistent with the error handling pattern.
Apply this change:
conf := &Config{} if err := v.Unmarshal(conf); err != nil { - panic(ErrInitFail.Wrap(err.Error())) + return nil, ErrInitFail.Wrap(err.Error()) } return conf, nil
♻️ Duplicate comments (1)
cmd/feeder_debug.go (1)
201-221: 🛠️ Refactor suggestionFix function name spelling and reduce code duplication
The function name is misspelled and contains duplicated response building logic.
+type txResponse struct { + CheckTx *abci.ResponseCheckTx + DeliverTx *abci.ResponseDeliverTx + Hash []byte + Height int64 +} + +func buildProtoResponse(res *txResponse) *debugger.SubmitPriceResponse { + return &debugger.SubmitPriceResponse{ + CheckTxSuccess: res.CheckTx.Code == 0, + CheckTxLog: res.CheckTx.Log, + DeliverTxSuccess: res.CheckTx.Code == 0 && res.DeliverTx.Code == 0, + DeliverTxLog: res.DeliverTx.Log, + TxHash: hex.EncodeToString(res.Hash), + Height: res.Height, + } +} -func sendTxImmediately(feederID uint64, price *PriceJSON) (*debugger.SubmitPriceResponse, error) { +func sendTxImmediately(feederID uint64, price *PriceJSON) (*debugger.SubmitPriceResponse, error) { if err := exoclient.Init(feederConfig, mnemonic, privFile, true, true); err != nil { return nil, fmt.Errorf("failed to init exoclient in txOnly mode for debug, error:%w", err) } ec, _ := exoclient.GetClient() pInfo := price.getPriceInfo() res, err := ec.SendTxDebug(feederID, price.BaseBlock, pInfo, price.Nonce) if err != nil { return nil, err } - protoRes := &debugger.SubmitPriceResponse{ - CheckTxSuccess: res.CheckTx.Code == 0, - CheckTxLog: res.CheckTx.Log, - DeliverTxSuccess: res.CheckTx.Code == 0 && res.DeliverTx.Code == 0, - DeliverTxLog: res.DeliverTx.Log, - TxHash: res.Hash.String(), - Height: res.Height, - } - return protoRes, nil + return buildProtoResponse(res), nil }
🧹 Nitpick comments (3)
types/types.go (1)
Line range hint
154-160: Consider adding debug-related error definitions.Given the PR's focus on CLI debug tools, consider adding specific error definitions for debug operations, such as:
- Invalid block height
- Transaction sending failures
- Invalid feeder ID
Example additions:
ErrInvalidBlockHeight = NewErr("invalid block height") ErrTransactionSendFailed = NewErr("failed to send transaction") ErrInvalidFeederID = NewErr("invalid feeder ID")cmd/feeder_debug.go (2)
23-33: Add context cancellation handling in SubmitPriceThe SubmitPrice method should handle context cancellation to prevent potential goroutine leaks and ensure proper cleanup.
func (s *server) SubmitPrice(ctx context.Context, req *debugger.SubmitPriceRequest) (*debugger.SubmitPriceResponse, error) { result := make(chan *debugger.SubmitPriceResponse, 1) s.sendCh <- &sendReq{ height: req.Height, feederID: req.FeederId, price: req.Price, result: result, } - r := <-result - return r, nil + select { + case r := <-result: + return r, nil + case <-ctx.Done(): + return nil, ctx.Err() + } }
1-221: Standardize error handling patternsThe codebase shows inconsistent error handling patterns. Consider implementing the following improvements:
- Use wrapped errors consistently
- Add error codes for different types of failures
- Improve error messages
Example implementation:
+type ErrorCode string + +const ( + ErrInitialization ErrorCode = "INIT_ERROR" + ErrConnection ErrorCode = "CONNECTION_ERROR" + ErrValidation ErrorCode = "VALIDATION_ERROR" + ErrTransaction ErrorCode = "TRANSACTION_ERROR" +) + +type FeederError struct { + Code ErrorCode + Message string + Err error +} + +func (e *FeederError) Error() string { + if e.Err != nil { + return fmt.Sprintf("[%s] %s: %v", e.Code, e.Message, e.Err) + } + return fmt.Sprintf("[%s] %s", e.Code, e.Message) +} + +func newFeederError(code ErrorCode, message string, err error) error { + return &FeederError{ + Code: code, + Message: message, + Err: err, + } +}Apply this pattern consistently throughout the codebase when handling errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/feeder_debug.go(1 hunks)types/types.go(5 hunks)
🔇 Additional comments (2)
types/types.go (2)
4-4: LGTM: Import addition aligns with error handling improvements.
The addition of the "errors" package is appropriate for the enhanced error handling in the InitConfig function.
163-169: LGTM: Improved error handling for configuration initialization.
The changes follow Go best practices by:
- Validating input parameters
- Checking file existence
- Returning errors instead of failing silently
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
types/types.go (1)
Line range hint
180-184: Replace panic with error return for unmarshal failures.Instead of panicking on unmarshal errors, consider returning the error to allow callers to handle it gracefully.
func InitConfig(cfgFile string) (*Config, error) { // ... existing code ... conf := &Config{} if err := v.Unmarshal(conf); err != nil { - panic(ErrInitFail.Wrap(err.Error())) + return nil, ErrInitFail.Wrap(err.Error()) } return conf, nil }
🧹 Nitpick comments (3)
types/types.go (1)
50-52: Add field documentation to clarify the purpose of each endpoint.The Exocore struct now has both
GrpcandRpcfields. While this is valid, adding documentation comments would help users understand their distinct purposes:
Grpcfor price submission serviceRpcfor blockchain interactiontype Config struct { Tokens []TokenSources `mapstructure:"tokens"` Sender struct { Mnemonic string `mapstructure:"mnemonic"` Path string `mapstructure:"path"` } `mapstructure:"sender"` Exocore struct { ChainID string `mapstructure:"chainid"` AppName string `mapstructure:"appname"` + // Grpc endpoint for price submission service Grpc string `mapstructure:"grpc"` Ws string `mapstructure:"ws"` + // Rpc endpoint for blockchain interaction Rpc string `mapstructure:"rpc"` } `mapstructure:"exocore"` }cmd/debug.go (2)
30-39: Debug command stands well as an entrypoint.
The user-friendlyShortandLongdescriptions help clarify usage.Could expand on the logs or the states it might display, but it’s optional.
99-105: printProto function is fine for debugging.
It doesn’t return an error on marshal failure, but printing to stdout is acceptable in most debugging workflows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/debug.go(1 hunks)cmd/feeder_debug.go(1 hunks)exoclient/types.go(2 hunks)types/types.go(5 hunks)
🔇 Additional comments (19)
types/types.go (3)
4-4: LGTM: Appropriate use of standard error package.
The addition of the "errors" package aligns with the enhanced error handling in the InitConfig function.
154-154: LGTM: Appropriate use of package-level Viper instance.
The package-level Viper instance correctly maintains configuration state between InitConfig and ReloadConfig functions.
163-169: LGTM: Improved input validation and error handling.
The function now properly validates the input filename and checks file existence before proceeding.
cmd/feeder_debug.go (10)
1-16: Imports and basic setup look consistent.
No immediate issues spotted. All dependencies appear relevant for gRPC and debugging functionality.
18-25: Struct design is clean and comprehensible.
The sendReq struct properly encapsulates the transaction parameters and a channel to retrieve results. The unexported fields are suitable if usage is confined within this package.
53-56: Server struct design is minimalistic yet functional.
The sendCh channel pipeline integrates well with pendingRequestManager.
58-68: Potential channel blocking scenario.
Although there's a buffer of 10 in sendCh, substantial traffic might risk blocking calls to SubmitPrice. Monitor usage or consider increasing the buffer if usage spikes are expected.
76-83: PriceJSON struct is straightforward.
This minimal set of fields is clear, and JSON tagging is correct. Ensure the design covers all expected data fields.
98-105: getPriceInfo method is concise and effective.
No issues detected. The direct mapping from PriceJSON to PriceInfo is appropriate.
114-191: Large function with multiple responsibilities.
This was previously flagged for refactoring into smaller, modular functions (initialization, subscription setup, goroutine management, gRPC serving).
193-209: gRPC Dial usage is correct but missing advanced config (timeouts, etc.).
As described in a past comment, consider specifying a context with timeout or a retry mechanism for reliability.
211-231: Initialization with true, true.
Ensure the combination of txOnly and standalone matches your intended usage. If there are edge cases, explore separate flags or additional checks.
26-51: 🛠️ Refactor suggestion
Consider thread-safety for the map-based request manager.
pendingRequestManager is a raw map accessed from multiple goroutines (line 175 for adding requests, line 153 for processing). If concurrency is guaranteed single-threaded, this is fine. Otherwise, add synchronization (e.g., a mutex) to avoid data races.
type pendingRequestManager struct {
- requests map[int64][]*sendReq
+ mu sync.Mutex
+ requests map[int64][]*sendReq
}
func (p *pendingRequestManager) add(height int64, req *sendReq) {
+ p.mu.Lock()
if pendings, ok := p.requests[height]; ok {
p.requests[height] = append(pendings, req)
} else {
p.requests[height] = []*sendReq{req}
}
+ p.mu.Unlock()
}
func (p *pendingRequestManager) process(height int64, handler func(*sendReq) error) {
+ p.mu.Lock()
for h, pendings := range p.requests {
if h <= height {
...
}
}
+ p.mu.Unlock()
}Likely invalid or redundant comment.
cmd/debug.go (5)
1-13: Package definition and imports appear appropriate.
No issues. The chosen dependencies match the CLI tooling requirements.
15-18: Global flag definitions look consistent.
feederID and height are clearly named.
20-28: Initialization pattern is valid.
Attaching subcommands here is a standard approach.
41-70: send command handles input parsing and JSON unmarshal properly.
Error handling is present for flags and JSON. The usage of sendTx is correct.
72-97: send-imm command parallels send with immediate sending.
The validation step for PriceJSON is an excellent addition.
exoclient/types.go (1)
Line range hint 359-418: New Init function parameters add flexibility.
The signature now accepts a pointer to feedertypes.Config and a txOnly boolean, improving clarity of intent. Initialization code looks consistent with the new parameters. Nice job.
Add CLIs to support sending create-price tx from cli
price-feeder --config [cfgFile] debug send --feederID [feederID ]--height [height] '{"base_block":310,"nonce":1,"price":"999237000000","det_id":"123","decimal":8,"timestamp":"2024-12-27 15:16:17"}'price-feeder --config [cfgFile] debug send-imm --feederID [feederID] '{"base_block":310,"nonce":1,"price":"999237000000","det_id":"123","decimal":8,"timestamp":"2024-12-27 15:16:17"}'Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Technical Updates