fix(oracle): return error instead of nil in RemoveExcessFeeds#244
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoveExcessFeeds in x/oracle/keeper/keeper.go was changed to log errors and continue processing remaining denoms when removing an exchange rate fails, instead of returning immediately. A new test TestRemoveExcessFeedsWithError was added to x/oracle/keeper/keeper_test.go to exercise this non-abortive behavior and verify valid exchange rates remain accessible. CHANGELOG.md was updated under UNRELEASED with a note about Return error instead of nil in RemoveExcessFeeds. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 20: Changelog contains a duplicate entry for "Return error instead of nil
in RemoveExcessFeeds"; decide which place should remain and remove the
duplicate: if this change is new and unreleased, keep the UNRELEASED entry (the
one referencing RemoveExcessFeeds) and delete the v6.0.0 duplicate; if it was
already released in v6.0.0, delete the UNRELEASED line instead—ensure only one
entry referencing RemoveExcessFeeds remains.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Thaleszh
left a comment
There was a problem hiding this comment.
Need to remove duplicated entry on changelog and review behavior change
385df7b to
0af406f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@x/oracle/keeper/keeper_test.go`:
- Around line 719-732: The test uses undefined constants TEST1, TEST2, and EXTRA
which will fail to compile; update calls to oracleKeeper.VoteTarget.Set and
oracleKeeper.SetBaseExchangeRateWithDefault to use the existing utils constants
instead: replace TEST1 with utils.AtomDenom, TEST2 with utils.EthDenom, and
EXTRA with utils.KiiDenom so the Set and SetBaseExchangeRateWithDefault
invocations compile and use the intended denoms.
🧹 Nitpick comments (1)
x/oracle/keeper/keeper_test.go (1)
711-741: Test does not actually verify error handling behavior.The test is named
TestRemoveExcessFeeds_ErrorHandlingand claims to test that "errors are logged and processing continues," but it:
- Does not inject any errors or mock storage failures
- Does not verify that errors are logged
- Only tests the happy path where all removals succeed
This test is functionally equivalent to the existing
TestRemoveExcessFeedstest (lines 345-377). To properly test error handling, consider:
- Mocking
ExchangeRate.Removeto return an error for specific denoms- Verifying that
RemoveExcessFeedscontinues processing after encountering errors- Capturing and asserting on log output
0af406f to
385df7b
Compare
d704c79 to
fe2af57
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/oracle/keeper/keeper.go (1)
269-277: Implementation diverges from PR description.The PR description states this fix "returns the error so callers receive the storage removal failure," but the implementation logs the error and continues, ultimately returning
nil. This aligns with the maintainer's feedback to "just warn the behavior" but contradicts the PR title and description.If logging-and-continue is the intended behavior (which is reasonable for resilience), update the PR description and changelog to accurately reflect this. The current messaging is misleading.
Note: This pattern differs from
AddPriceSnapshot(lines 332-336) which returns immediately onRemovefailure. Consider whether error handling should be consistent across the keeper.
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 3: Run `go mod tidy` and commit the resulting updates to go.mod and
go.sum to satisfy the CI check; then restore or confirm the Go version directive
in go.mod (change the current `go 1.23` back to `go 1.23.8` if the patch version
was intentionally removed in the diff) so the file contains the correct `go`
directive and tidy changes.
- Fix typo: return err instead of nil when ExchangeRate.Remove fails - Log error and continue processing instead of returning error immediately - Add test case TestRemoveExcessFeedsWithError to cover error handling path - Update CHANGELOG.md with fix entry This ensures storage errors are properly logged while allowing the function to continue processing remaining denoms instead of stopping on first error.
fe2af57 to
6e5b52f
Compare
|
Just missing linting |
Description
found a typo in
RemoveExcessFeeds- whenExchangeRate.Removefails it was doingreturn nilinstead ofreturn err. this swallows the error so the caller never knows removal failed.split from #234 per maintainer request
Type of change
How Has This Been Tested?
ran the existing test to make sure nothing broke:
PR Checklist:
make lint-fix