fix: disable fee abstraction when base token price is 0 to prevent incorrect fee conversions#307
Conversation
WalkthroughThe changes address a fee abstraction bug by extending price validation in the oracle keeper. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/feeabstraction/keeper/oracle.go (1)
37-44:⚠️ Potential issue | 🟡 MinorDebug message is misleading for zero/negative prices, and clarify the operational design.
The module can be re-enabled through the
UpdateParamsgovernance message (line 28 in msg_server.go), which accepts anauthorityparameter. However, once disabled by zero/negative price, automatic recovery does not occur—the module remains disabled until governance explicitly re-enables it via proposal.Additionally, the debug message
"%s has no price"(line 41) is accurate when the price is missing (!ok) but misleading when the price exists but is zero or negative (!baseTokenPrice.IsPositive()). Consider distinguishing these cases in the log message, e.g.,"%s price is not positive"or similar.The permanent-disable-until-governance-action design is a valid safety choice but may be worth documenting if the behavior isn't already evident to operators. If auto-recovery on price recovery is desired, the
BeginBlockerwould need periodic re-validation even when disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/feeabstraction/keeper/oracle.go` around lines 37 - 44, The code currently treats missing and non-positive prices the same and logs a misleading message; update the block that reads twapPriceMap[params.NativeOracleDenom] to distinguish the two cases: if !ok log via k.Logger(ctx).Debug that the denom has no price (e.g., "%s has no price"), and if baseTokenPrice exists but !baseTokenPrice.IsPositive() log a different message (e.g., "%s price is not positive") before setting params.Enabled = false and calling k.Params.Set(ctx, params); also add a short inline comment referencing the governance UpdateParams (msg_server.go) as the required manual re-enable path so operators know the module remains disabled until a governance proposal re-enables it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@x/feeabstraction/keeper/oracle.go`:
- Around line 37-44: The code currently treats missing and non-positive prices
the same and logs a misleading message; update the block that reads
twapPriceMap[params.NativeOracleDenom] to distinguish the two cases: if !ok log
via k.Logger(ctx).Debug that the denom has no price (e.g., "%s has no price"),
and if baseTokenPrice exists but !baseTokenPrice.IsPositive() log a different
message (e.g., "%s price is not positive") before setting params.Enabled = false
and calling k.Params.Set(ctx, params); also add a short inline comment
referencing the governance UpdateParams (msg_server.go) as the required manual
re-enable path so operators know the module remains disabled until a governance
proposal re-enables it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6eef121-6509-4d83-b6d5-7db572f5775c
📒 Files selected for processing (3)
CHANGELOG.mdx/feeabstraction/keeper/oracle.gox/feeabstraction/keeper/oracle_test.go
Description
Disable fee abstraction when base token price is 0 to prevent incorrect fee conversions
Type of change
How Has This Been Tested?
Regression test
PR Checklist:
Make sure each step was done:
make lint-fix