fix(oracle): handle errors in pickReferenceDenom instead of panic by ngapaxs · Pull Request #232 · KiiChain/kiichain

ngapaxs · 2026-01-14T15:04:18Z

fix(oracle): handle errors in pickReferenceDenom instead of panic

Handle error from TotalBondedTokens instead of ignoring with _
Return error from Params.Get instead of panic
Update pickReferenceDenom signature to return error
Update caller in abci.go to handle the error

Fixes #230

Description

In x/oracle/tally.go, the pickReferenceDenom function has two error handling issues:

Line 22: Error from TotalBondedTokens() is ignored with _
Line 28: Error from Params.Get() causes panic instead of returning error

This PR restructures pickReferenceDenom to return an error to the oracle EndBlocker, as suggested by maintainer @Thaleszh.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Code review - verified error handling logic
Checked that function signature change propagates correctly to caller in abci.go

coderabbitai · 2026-01-14T15:04:32Z

Walkthrough

The pickReferenceDenom function in x/oracle/tally.go was changed to return an additional error and now propagates errors from k.StakingKeeper.TotalBondedTokens(ctx) instead of ignoring them (logging and returning ("", nil, err) on failure). The existing Params.Get(ctx) error handling remains. A unit test TestPickReferenceDenomTotalBondedTokensError was added, introducing MockStakingKeeperError to simulate a failing TotalBondedTokens. CHANGELOG.md was updated to document the fix.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses the TotalBondedTokens error handling from issue `#230`, but does not show updates to the caller in abci.go to handle the new error return.	Verify that abci.go's call to pickReferenceDenom has been updated to handle the new error return value in the PR changes.
Out of Scope Changes check	❓ Inconclusive	The raw summary shows changes to tally.go, tally_test.go, and CHANGELOG.md, but does not confirm whether abci.go was modified as required by the PR description.	Provide the complete PR diff including abci.go changes to verify all necessary callers were updated with error handling.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing error handling in pickReferenceDenom to return errors instead of panicking.
Description check	✅ Passed	The description clearly explains the bug, the solution, and references the linked issue `#230`, providing good context for the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 040da8b and aaea112.

📒 Files selected for processing (1)

x/oracle/tally_test.go

🧰 Additional context used

🧬 Code graph analysis (1)

x/oracle/tally_test.go (4)

x/oracle/types/expected_keepers.go (1)

StakingKeeper (17-24)

x/oracle/types/params.pb.go (3)

Denom (113-116)

Denom (119-119)

Denom (120-122)

x/oracle/utils/assets.go (1)

AtomDenom (6-6)

x/oracle/types/ballot.go (1)

ExchangeRateBallot (51-51)

🔇 Additional comments (4)

x/oracle/tally_test.go (4)

4-5: LGTM!

The new imports are required for the mock implementation: context for the method signature and fmt for error creation.

226-229: LGTM!

Good use of interface embedding for a partial mock. The design will correctly panic if the implementation under test fails to return early on error, which serves as an additional safeguard.

231-239: LGTM!

The mock methods correctly implement the StakingKeeper interface signatures. Docstrings were added addressing the previous review feedback.

242-264: LGTM!

The test properly validates the core PR objective: ensuring pickReferenceDenom propagates errors from TotalBondedTokens instead of ignoring them. The assertions comprehensively verify the error condition and expected return values.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Thaleszh

Hello there! Nice PR ongoing, but there are a few things requiring change:

The code you pushed is using a different spacing lint. Run make lint-fix to ensure it has the same pattern as the repo's. It will also make it much better for reviewing. You can check if it's working by reviewing the change tab on the PR, it should only highlight your changes.
Each change must be documented in the changelog. Please add the fix you are pushing under Unreleased > Fixed
It is also good practice to add tests cases on tally_test.go that checks the new behaviors introduced.

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@CHANGELOG.md`:
- Line 20: The CHANGELOG.md contains a duplicate entry "Handle ignored error
from TotalBondedTokens in pickReferenceDenom
[`#230`](https://github.com/KiiChain/kiichain/issues/230)" appearing in both the
UNRELEASED section and the v6.0.0 section; remove the duplicate so the fix is
listed only once (preferably keep it under UNRELEASED until release), ensuring
the remaining entry stays correctly formatted under the intended section header.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 89cbb55 and 0e7234c.

📒 Files selected for processing (2)

CHANGELOG.md
x/oracle/tally.go

🧰 Additional context used

🧬 Code graph analysis (1)

x/oracle/tally.go (1)

x/oracle/types/expected_keepers.go (1)

StakingKeeper (17-24)

🔇 Additional comments (3)

x/oracle/tally.go (3)

21-27: Critical fix for the threshold bypass vulnerability.

This properly addresses issue #230. Previously, ignoring the error from TotalBondedTokens would result in totalBondedTokens = 0, causing thresholdVotes = 0, which would allow any ballot to pass regardless of actual voting power. The fix correctly propagates the error to prevent oracle manipulation.

72-72: LGTM!

Correct success return with nil error, consistent with the error-returning paths.

15-15: Function signature updated to return error — all callers properly handle the returned error.

The signature change to return an error is the correct approach for proper error propagation. Verification confirms that all callers in abci.go (lines 78-80) and tally_test.go (lines 100, 217) correctly handle the returned error. The error handling for TotalBondedTokens and Params.Get is properly implemented.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

- Add error handling for TotalBondedTokens call in pickReferenceDenom - Remove duplicate changelog entry - Fixes KiiChain#230

codecov · 2026-01-14T18:36:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Thaleszh

Looking excellent. Just have a nitpick on missing docstring on some tests

Thaleszh

Looking good, waiting for pipelines and then will merge

ngapaxs requested a review from jhelison as a code owner January 14, 2026 15:04

ngapaxs force-pushed the fix/oracle-error-handling branch from 7027019 to 89cbb55 Compare January 14, 2026 15:24

Thaleszh suggested changes Jan 14, 2026

View reviewed changes

fix(oracle): handle TotalBondedTokens error instead of ignoring

0e7234c

ngapaxs force-pushed the fix/oracle-error-handling branch from 89cbb55 to 0e7234c Compare January 14, 2026 15:45

coderabbitai Bot reviewed Jan 14, 2026

View reviewed changes

Comment thread CHANGELOG.md

ngapaxs added 2 commits January 14, 2026 22:58

fix(oracle): handle TotalBondedTokens error instead of ignoring

5d7513d

- Add error handling for TotalBondedTokens call in pickReferenceDenom - Remove duplicate changelog entry - Fixes KiiChain#230

style: run make lint-fix

51a6277

test(oracle): add test for TotalBondedTokens error handling

040da8b

Thaleszh suggested changes Jan 14, 2026

View reviewed changes

Comment thread x/oracle/tally_test.go

docs: add docstrings to test functions

aaea112

Thaleszh approved these changes Jan 15, 2026

View reviewed changes

Thaleszh merged commit 3124791 into KiiChain:main Jan 15, 2026
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(oracle): handle errors in pickReferenceDenom instead of panic#232

fix(oracle): handle errors in pickReferenceDenom instead of panic#232
Thaleszh merged 5 commits into
KiiChain:mainfrom
ngapaxs:fix/oracle-error-handling

ngapaxs commented Jan 14, 2026

Uh oh!

coderabbitai Bot commented Jan 14, 2026 •

edited

Loading

Uh oh!

Thaleszh left a comment •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Jan 14, 2026 •

edited

Loading

Uh oh!

Thaleszh left a comment

Uh oh!

Uh oh!

Thaleszh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ngapaxs commented Jan 14, 2026

fix(oracle): handle errors in pickReferenceDenom instead of panic

Description

Type of change

How Has This Been Tested?

Uh oh!

coderabbitai Bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Uh oh!

Thaleszh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Thaleszh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thaleszh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jan 14, 2026 •

edited

Loading

Thaleszh left a comment •

edited

Loading

codecov Bot commented Jan 14, 2026 •

edited

Loading