fix: handle error in pickReferenceDenom instead of panic #220
Conversation
WalkthroughThe pull request changes pickReferenceDenom to return an error (signature now returns (referenceDenom string, belowThresholdVoteMap map[string]types.ExchangeRateBallot, err error)) and removes the panic on params retrieval failure by logging and returning the error. EndBlocker in x/oracle/abci.go now captures and handles the error from pickReferenceDenom (logs and early-returns). Tests in x/oracle/tally_test.go were updated to assert the success path and added a test for the error path. CHANGELOG.md was updated to document the fix. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)x/oracle/tally_test.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Line 49: Remove the duplicate changelog entry "Handle error in
pickReferenceDenom instead of panic to prevent chain halt
[#203](https://github.com/KiiChain/kiichain/issues/203)" from the released
version section (v6.0.0) so it remains only under the UNRELEASED heading; locate
the string in CHANGELOG.md (the entry under v6.0.0) and delete that single line,
leaving the UNRELEASED entry intact.
🧹 Nitpick comments (2)
x/oracle/tally_test.go (1)
100-100: Consider adding a test for the error case.The test correctly captures the new third return value. However, the fix for issue #203 specifically addresses the panic when
k.Params.Get(ctx)fails, yet there's no test verifying thatpickReferenceDenomreturns an error gracefully in that scenario.Adding a negative test case would validate the fix and prevent regressions.
x/oracle/tally.go (1)
21-23: Consider handling the error fromTotalBondedTokens.The error from
TotalBondedTokensis silently ignored. If this call fails,totalBondedTokenscould be zero or invalid, potentially causing incorrectthresholdVotescalculation. Since this PR focuses on robust error handling to prevent chain issues, consider propagating this error as well.Proposed fix
// Get total bonded power powerReductionFactor := k.StakingKeeper.PowerReduction(ctx) // get the power reduction - totalBondedTokens, _ := k.StakingKeeper.TotalBondedTokens(ctx) // total of tokens in staking + totalBondedTokens, err := k.StakingKeeper.TotalBondedTokens(ctx) // total of tokens in staking + if err != nil { + return "", nil, err + } totalBondedPower := sdk.TokensToConsensusPower(totalBondedTokens, powerReductionFactor) // Get the blockchain vote power
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdx/oracle/abci.gox/oracle/tally.gox/oracle/tally_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: tests
- GitHub Check: golangci-lint
- GitHub Check: Analyze
- GitHub Check: test-e2e
🔇 Additional comments (2)
x/oracle/abci.go (1)
78-82: LGTM!The error handling correctly addresses the critical issue #203. The implementation:
- Properly captures and checks the error from
pickReferenceDenom- Logs the error with context before returning, which aids debugging
- Returns the error to prevent proceeding with invalid state
This prevents the chain halt that would occur from a panic in EndBlocker.
x/oracle/tally.go (1)
15-15: LGTM! This correctly addresses the critical panic issue.The function signature now returns an error, and the
k.Params.Get(ctx)failure path gracefully returns an error instead of panicking. This prevents the chain halt scenario described in issue #203.Also applies to: 26-29, 67-67
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where pickReferenceDenom in the oracle module uses panic(err) instead of returning an error, which can cause chain halt when called from EndBlocker.
Key changes:
- Modified
pickReferenceDenomto return an error instead of panicking - Updated EndBlocker to properly handle and log errors from
pickReferenceDenom - Updated test to handle the new error return value
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| x/oracle/tally.go | Changed function signature to return error and replaced panic(err) with return "", nil, err |
| x/oracle/abci.go | Added error handling for pickReferenceDenom call with logging |
| x/oracle/tally_test.go | Updated test to handle new error return value from pickReferenceDenom |
| CHANGELOG.md | Added changelog entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/oracle/tally.go (1)
176-284: Critical syntax error: duplicate import statement and function definitions.Lines 176-284 contain duplicate import statements and function definitions that already appear at lines 1-174. In Go, import statements must appear before other declarations. This syntax error is causing all pipeline failures (E2E tests, liveness tests, and golangci-lint).
🔧 Fix: Remove duplicate content
Delete lines 176-284 entirely. The correct package declaration, imports, and function definitions already exist at lines 1-174.
-import ( - "cosmossdk.io/math" - - sdk "github.com/cosmos/cosmos-sdk/types" - - "github.com/kiichain/kiichain/v7/x/oracle/keeper" - "github.com/kiichain/kiichain/v7/x/oracle/types" -) - -// pickReferenceDenom selects a denom with the highest vote power as reference denom. -// If the power of 2 denominations is the same, select the reference denom -// in alphabetical order -func pickReferenceDenom(ctx sdk.Context, k keeper.Keeper, voteTargets map[string]types.Denom, voteMap map[string]types.ExchangeRateBallot) (string, map[string]types.ExchangeRateBallot, error) { - highestBallotPower := int64(0) - referenceDenom := "" - belowThresholdVoteMap := map[string]types.ExchangeRateBallot{} - - // Get total bonded power - powerReductionFactor := k.StakingKeeper.PowerReduction(ctx) // get the power reduction - totalBondedTokens, _ := k.StakingKeeper.TotalBondedTokens(ctx) // total of tokens in staking - totalBondedPower := sdk.TokensToConsensusPower(totalBondedTokens, powerReductionFactor) // Get the blockchain vote power - - // Get threshold (minimum power necessary to considerate a successful ballot) - params, err := k.Params.Get(ctx) - if err != nil { - return "", nil, err - } - - voteThreshold := params.VoteThreshold // Get vote threshold from params - thresholdVotes := voteThreshold.MulInt64(totalBondedPower).RoundInt() // Threshold to allow a ballot - - // Iterate the voting map - for denom, ballot := range voteMap { - - // If a denom is not in the vote targets or the ballot for it has failed - // that denom is removed from votemap (for efficiency) - _, exists := voteTargets[denom] - if !exists { - delete(voteMap, denom) - continue - } - - // Get ballot power and check if is greater than the threshold - ballotPower, ok := ballotIsPassing(ballot, thresholdVotes) - - // if the ballot power is lower than threshold, add denom in below - // threshold map to separe for tally evaluation - if !ok { - belowThresholdVoteMap[denom] = voteMap[denom] - delete(voteTargets, denom) - delete(voteMap, denom) - continue - } - - if ballotPower.Int64() > highestBallotPower || highestBallotPower == 0 { - referenceDenom = denom - highestBallotPower = ballotPower.Int64() - } - - // If the power is equal, select the the denom by alphabetical order - if ballotPower.Int64() == highestBallotPower && referenceDenom > denom { - referenceDenom = denom - } - } - return referenceDenom, belowThresholdVoteMap, nil -} - -// ballotIsPassing calculate the sum of each vote power per denom, then check -// if the ballot power is greater than the threshold -func ballotIsPassing(ballot types.ExchangeRateBallot, thresholdVotes math.Int) (math.Int, bool) { - ballotPower := math.NewInt(ballot.Power()) // Get the validator power - - // return ballot power and if the ballot is greater than the threshold - return ballotPower, !ballotPower.IsZero() && ballotPower.GTE(thresholdVotes) -} - -// Tally calculates the median and returns it. Sets the set of voters to be rewarded, i.e. voted within -// a reasonable spread from the weighted median to the store -// CONTRACT: ex must be sorted -func Tally(_ sdk.Context, ex types.ExchangeRateBallot, rewardBand math.LegacyDec, validatorClaimMap map[string]types.Claim) (weightedMedian math.LegacyDec) { - weightedMedian = ex.WeightedMedianWithAssertion() // Get weighted median - - // Check if result is on the reward interval - standardDeviation := ex.StandardDeviation(weightedMedian) - rewardSpread := weightedMedian.Mul(rewardBand.QuoInt64(2)) // this is the interval that will be added around weightedMedian - - if standardDeviation.GT(rewardSpread) { // if rewardSpread > deviation means the data is disperse - rewardSpread = standardDeviation - } - - // Check each vote and reward - for _, vote := range ex { - // Filter ballot winners - voter := vote.Voter.String() - claim := validatorClaimMap[voter] - - // If exchange rate is in the interval reward the validator - if vote.ExchangeRate.GTE(weightedMedian.Sub(rewardSpread)) && // lower limit - vote.ExchangeRate.LTE(weightedMedian.Add(rewardSpread)) { // upper limit - - claim.Weight += vote.Power - claim.WinCount++ - } - claim.DidVote = true - validatorClaimMap[voter] = claim - } - - return -}
🧹 Nitpick comments (1)
x/oracle/tally.go (1)
12-174: Address excessive blank lines throughout the file.Lines 12-174 contain excessive blank lines between nearly every line of code. This significantly reduces readability. The code should follow standard Go formatting conventions with appropriate spacing.
♻️ Proposed fix: Remove excessive blank lines
Run
gofmtorgoimportson this file to normalize the spacing, or manually remove the extra blank lines between statements within functions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdx/oracle/abci.gox/oracle/tally.gox/oracle/tally_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 GitHub Actions: E2E
x/oracle/tally.go
[error] 176-176: syntax error: imports must appear before other declarations
🪛 GitHub Actions: Liveness check
x/oracle/tally.go
[error] 176-176: syntax error: imports must appear before other declarations
🪛 GitHub Check: golangci-lint
x/oracle/tally.go
[failure] 176-176:
syntax error: imports must appear before other declarations) (typecheck)
[failure] 176-176:
imports must appear before other declarations (typecheck)
[failure] 176-176:
syntax error: imports must appear before other declarations (typecheck)
[failure] 176-176:
syntax error: imports must appear before other declarations) (typecheck)
[failure] 176-176:
syntax error: imports must appear before other declarations) (typecheck)
[failure] 176-176:
syntax error: imports must appear before other declarations) (typecheck)
🪛 GitHub Check: liveness-test
x/oracle/tally.go
[failure] 176-176:
syntax error: imports must appear before other declarations
🪛 GitHub Check: test-e2e
x/oracle/tally.go
[failure] 176-176:
syntax error: imports must appear before other declarations
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: test-e2e
- GitHub Check: liveness-test
- GitHub Check: Analyze
- GitHub Check: golangci-lint
🔇 Additional comments (3)
x/oracle/tally_test.go (1)
100-101: LGTM! Test correctly updated for new error return.The test properly captures and asserts the new error return value from
pickReferenceDenom, aligning with the updated function signature.x/oracle/abci.go (1)
78-81: LGTM! Proper error handling prevents chain halt.The error capture and early return correctly addresses the critical issue where a panic in
pickReferenceDenomcould halt the chain. The error is properly propagated up the call stack.x/oracle/tally.go (1)
18-18: Core error handling changes look correct.The functional changes to return an error instead of panicking are correct and address the critical issue #203:
- Line 18: Function signature updated to return error
- Lines 38-42: Returns error instead of calling
panic(err)- Line 104: Returns
nilerror on successThese changes prevent chain halt when
k.Params.Get(ctx)fails duringEndBlockerexecution.Also applies to: 40-40, 104-104
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/oracle/tally_test.go (1)
199-221: Good test coverage for the error path.This test properly validates the error scenario when params are missing, which directly addresses the critical issue #203. The test correctly:
- Removes params to simulate the failure condition
- Verifies all three return values (error, empty string, nil map)
- Ensures no panic occurs
Optional: Verify error message for better diagnostics
Consider adding an assertion to verify the error message contains relevant context about the param retrieval failure. This helps ensure the logged error provides useful debugging information:
referenceDenom, belowThresholdVoteMap, err := pickReferenceDenom(ctx, oracleKeeper, votingTarget, voteMap) require.Error(t, err) + require.Contains(t, err.Error(), "param") require.Empty(t, referenceDenom) require.Nil(t, belowThresholdVoteMap)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/oracle/tally_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
x/oracle/tally_test.go (3)
x/oracle/keeper/test_utils.go (1)
CreateTestInput(130-294)x/oracle/types/params.pb.go (6)
Params(28-50)Params(53-53)Params(54-56)Denom(113-116)Denom(119-119)Denom(120-122)x/oracle/utils/assets.go (1)
AtomDenom(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: liveness-test
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: test-e2e
🔇 Additional comments (1)
x/oracle/tally_test.go (1)
100-101: LGTM! Error handling properly added to happy path test.The update correctly captures the new error return value and asserts no error occurs in the success scenario.
2 participants
Summary
This PR fixes a critical bug where pickReferenceDenom in x/oracle/tally.go uses panic(err) which can cause chain halt when called from EndBlocker.
Changes
Impact
Prevents potential chain halt if k.Params.Get(ctx) fails during EndBlocker execution.
Fixes #203