make megavault withdrawal logging more succinct #2607
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/withdraw.go (2)
Line range hint
307-317: Consider maintaining consistent error severity levelsThe change from error to debug logging follows the pattern from the previous block. While this makes logging more succinct, consider maintaining error-level logging for critical vault operations like leverage and equity calculations, even during simulations.
Consider this alternative approach:
-if !simulate { +if err != nil { log.DebugLog( ctx, - "Megavault withdrawal: failed to get vault leverage and equity. Skipping this vault", + "Megavault withdrawal: %s. Skipping vault %v", + simulate ? "simulation failed" : "failed", "Vault ID", vaultId, "Error", err, ) + if !simulate { + log.ErrorLogWithError(ctx, "Failed to get vault leverage and equity", err) + } }
Line range hint
350-359: Consider elevating quantum validation errorsInvalid quantum transfers could indicate underlying calculation issues or overflow conditions. These should potentially remain at error level even during simulations, as they might reveal important edge cases.
-if !simulate { +if quantumsToTransfer.Sign() <= 0 || !quantumsToTransfer.IsUint64() { log.DebugLog( ctx, "Megavault withdrawal: quantums to transfer is invalid. Skipping this vault", "Vault ID", vaultId, "Quantums", quantumsToTransfer, ) + if !simulate { + log.ErrorLogWithError( + ctx, + "Invalid quantum transfer detected", + fmt.Errorf("quantum: %v", quantumsToTransfer), + ) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/x/vault/keeper/withdraw.go(4 hunks)
🔇 Additional comments (2)
protocol/x/vault/keeper/withdraw.go (2)
Line range hint 330-340: LGTM - Consistent with previous changes
The logging changes follow the established pattern.
Line range hint 293-303: Verify impact of reduced error visibility during simulations
The change from error to debug logging and the condition to only log when !simulate could make it harder to debug simulation-related issues. Consider keeping error logs during simulations for critical failures like perpetual/market retrieval.
Let's verify the usage of simulation mode in the codebase:
Changelist
remove unnecessary error logs and emit debug log if not in simulate mode
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
These updates enhance the reliability and user experience during transactions involving vault withdrawals and redemptions.