🎨 Fee log cleanup#112
Conversation
WalkthroughThe changes focus on reducing debug and error logging verbosity across several internal and plugin-related files. Logging statements are removed or replaced with more consistent direct logger usage. Error handling is updated to favor error wrapping over logging, and error message prefixes are standardized for clarity. No control flow or logic is altered. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Pull Request Overview
This PR standardizes logging and error handling in the fee plugin by removing a custom Log wrapper, consolidating direct fp.logger calls, and unifying error prefixes; it also cleans up debug logs in verifier API code.
- Replaced the custom
Logmethod with directfp.loggercalls inplugin/fees/fees.go - Updated error‐message prefixes in
plugin/fees/transaction.goto consistently reference the correct functions/fields - Removed extraneous debug logs from
internal/verifierapi
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugin/fees/transaction.go | Standardize error prefixes and remove old p. shorthand in fmt.Errorf |
| plugin/fees/fees.go | Removed Log wrapper, replaced calls with fp.logger, and cleaned up error messages |
| internal/verifierapi/verifierapi.go | Removed a debug log in the constructor |
| internal/verifierapi/fees.go | Stripped out debug logs on error paths |
Comments suppressed due to low confidence (1)
plugin/fees/transaction.go:139
- The error prefix uses lowercase
plugincommon.genUnsignedTxbut the actual function isplugincommon.GenUnsignedTx. Update the case to match the function name.
return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.genUnsignedTx: %w", e)
| nonce, err := fp.nonceManager.GetNextNonce(ethAddress) | ||
| if err != nil { | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.nonceManager.GetNextNonce: %w", err) | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.nonceManager.GetNextNonce: %w", err) |
There was a problem hiding this comment.
The error prefix "plugincommon.nonceManager.GetNextNonce" does not match the actual target. It should refer to fp.nonceManager.GetNextNonce or the correct package-level function name to avoid misleading error traces.
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("plugincommon.nonceManager.GetNextNonce: %w", err) | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.nonceManager.GetNextNonce: %w", err) |
| }) | ||
| if e != nil { | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("p.txIndexerService.CreateTx: %w", e) | ||
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.tx_indexer.CreateTx: %w", e) |
There was a problem hiding this comment.
Reference to fp.tx_indexer appears incorrect; the field is named fp.txIndexerService. Update the error prefix and field usage to the correct identifier to ensure accurate context and compilation.
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.tx_indexer.CreateTx: %w", e) | |
| return []vtypes.PluginKeysignRequest{}, fmt.Errorf("fp.txIndexerService.CreateTx: %w", e) |
| if err := json.Unmarshal(task.Payload(), &feeCollectionFormat); err != nil { | ||
| fp.Log(logrus.ErrorLevel, "Failed to unmarshal fee collection config") | ||
| return err | ||
| return fmt.Errorf("fp.HandleCollections, failed to unmarshall asynq task payload, %w", err) |
There was a problem hiding this comment.
Typo in error message: 'unmarshall' should be 'unmarshal'. Also consider rephrasing for consistency (e.g., 'failed to unmarshal asynq task payload: %w').
| return fmt.Errorf("fp.HandleCollections, failed to unmarshall asynq task payload, %w", err) | |
| return fmt.Errorf("failed to unmarshal asynq task payload: %w", err) |
| return fp.collectFeesByPolicy(feeCollectionFormat.Value) | ||
| case FeeCollectionTypeByPluginID: | ||
| fp.Log(logrus.InfoLevel, "Collecting fees by plugin id") | ||
| fp.logger.Info("Collecting fees by plugin id") |
There was a problem hiding this comment.
This branch logs an info message but doesn’t return or invoke a handler, causing a silent fallthrough that returns nil. Add a return or handling logic for FeeCollectionTypeByPluginID.
| fp.logger.Info("Collecting fees by plugin id") | |
| fp.logger.Info("Collecting fees by plugin id") | |
| return fp.collectFeesByPluginID(feeCollectionFormat.Value) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/fees/fees.go (1)
314-314: Consider using structured logging for the fee run object.The current log concatenates the feeRun object, which may not provide optimal formatting.
Consider using structured logging:
-fp.logger.Info("Fee run created: ", feeRun) +fp.logger.WithField("fee_run_id", feeRun.ID).Info("Fee run created")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/verifierapi/fees.go(0 hunks)internal/verifierapi/verifierapi.go(0 hunks)plugin/fees/fees.go(5 hunks)plugin/fees/transaction.go(7 hunks)
💤 Files with no reviewable changes (2)
- internal/verifierapi/verifierapi.go
- internal/verifierapi/fees.go
🧰 Additional context used
🧠 Learnings (1)
plugin/fees/transaction.go (3)
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:0-0
Timestamp: 2025-06-18T18:23:20.077Z
Learning: In the payroll plugin (plugin/payroll/transaction.go), signRequest.Transaction is stored with the "0x" prefix, making it compatible with gcommon.FromHex which requires 0x-prefixed hex strings.
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
🧬 Code Graph Analysis (2)
plugin/fees/transaction.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
plugin/fees/fees.go (1)
plugin/fees/config.go (6)
FeeCollectionFormat(154-157)FeeCollectionType(145-145)FeeCollectionTypeAll(151-151)FeeCollectionTypeByPublicKey(148-148)FeeCollectionTypeByPolicy(149-149)FeeCollectionTypeByPluginID(150-150)
🔇 Additional comments (12)
plugin/fees/fees.go (5)
162-162: Good addition of structured logging for fee collection job tracking.This info-level log appropriately marks the start of the fee collection process, which is useful for monitoring and debugging.
170-170: Improved error message with consistent prefix and proper error wrapping.The error message now includes a clear function context prefix and uses proper error wrapping with %w verb for better error traceability.
176-184: Helpful info logs for different fee collection types.These logs provide clear visibility into which type of fee collection is being executed, which is valuable for operational monitoring.
203-203: Consistent info-level logging for fee collection status.The logging provides good visibility into the fee collection process, including pending amounts and collection status for different scenarios.
Also applies to: 217-217, 220-220, 239-239, 254-254, 273-273, 288-288
329-329: Proper error wrapping for transaction proposal failures.The error wrapping follows Go best practices and provides clear context for debugging.
plugin/fees/transaction.go (7)
124-124: Improved error message prefix for nonce manager.The updated prefix "plugincommon.nonceManager.GetNextNonce" provides better context and consistency compared to the previous shorter form.
139-139: Standardized error message prefix for transaction generation.The prefix "plugincommon.genUnsignedTx" clearly identifies the function context and maintains consistency with other error messages.
154-154: Consistent error message prefix for transaction indexer.The prefix "fp.tx_indexer.CreateTx" follows the established pattern and provides clear context for debugging.
223-223: Standardized error message prefix for transaction range query.The prefix "fp.txIndexerService.GetTxInTimeRange" is consistent with other error messages and provides clear function context.
244-244: Improved error message prefix for async task enqueueing.The prefix "fp.client.Enqueue" follows the established pattern and provides clear context for task queue operations.
254-254: Consistent error message prefix for task inspection.The prefix "fp.inspector.GetTaskInfo" maintains consistency with other error messages and clearly identifies the operation context.
277-277: Standardized error message prefix for signing completion.The prefix "fp.SigningComplete" follows the established pattern and provides clear context for the signing operation.
Summary by CodeRabbit