Skip to content

Remove math.sqrt from ballot calculation#191

Merged
jhelison merged 2 commits into
mainfrom
fix/safe-ballot
Dec 30, 2025
Merged

Remove math.sqrt from ballot calculation#191
jhelison merged 2 commits into
mainfrom
fix/safe-ballot

Conversation

@jhelison

@jhelison jhelison commented Dec 30, 2025

Copy link
Copy Markdown
Contributor

Description

This removes the unsafe math.sqrt from ballot calculation.
This solves issue #188

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (updates documentation on the project)
  • chore (Updates on dependencies, gitignore, etc)
  • test (For updates on tests)

How Has This Been Tested?

Tests are passing

Copilot AI review requested due to automatic review settings December 30, 2025 19:26
@coderabbitai

coderabbitai Bot commented Dec 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

This PR removes unsafe math.Sqrt usage from the oracle ballot standard deviation calculation. The StandardDeviation function is refactored to use variance.ApproxSqrt() with error handling instead of the manual float parsing and math.Sqrt approach. Unused imports (fmt, math, strconv) are removed from the ballot.go file. The corresponding test case is updated to use tolerance-based assertions with a delta of 2e-7 instead of exact equality checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing math.Sqrt from the ballot calculation as reflected in code changes across ballot.go and tests.
Description check ✅ Passed The description is directly related to the changeset, clearly stating the removal of unsafe math.sqrt from ballot calculation and referencing issue #188.
✨ 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 359cfae and 2f6df63.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • x/oracle/types/ballot.go
  • x/oracle/types/ballot_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). (7)
  • GitHub Check: CodeQL analysis (go)
  • GitHub Check: Agent
  • GitHub Check: tests
  • GitHub Check: liveness-test
  • GitHub Check: test-e2e
  • GitHub Check: Analyze
  • GitHub Check: golangci-lint
🔇 Additional comments (3)
CHANGELOG.md (1)

8-8: LGTM!

The changelog entry clearly documents the removal of unsafe math.Sqrt usage and aligns with the changes in the codebase.

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

182-187: LGTM! Tolerance-based assertion is appropriate for approximate calculations.

The test correctly adapts to the new ApproxSqrt() implementation by using a tolerance-based assertion with a delta of 2e-7. The expected value of 1.224745 is mathematically correct for the test data (sqrt(1.5) ≈ 1.224744871).

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

182-189: Good error handling implementation with appropriate defensive programming.

The refactoring correctly replaces unsafe math.Sqrt conversion with variance.ApproxSqrt(), which returns explicit errors. The implementation handles errors appropriately:

  • Error handling is properly layered: Line 186-189 catches ApproxSqrt() errors explicitly, while the defer/recover block (lines 169-174) serves as a backup panic handler for other operations like QuoInt64() (line 183). This dual-layer approach is good defensive programming.

  • Return value consistency: Returning zero on calculation failure is consistent across both error paths and aligns with the function's behavior for empty ballots (line 164-166).

The implementation is sound and no changes are required.

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.

@codecov

codecov Bot commented Dec 30, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
x/oracle/types/ballot.go 50.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI reviewed Dec 30, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the unsafe usage of Go's standard library math.Sqrt function in the oracle ballot standard deviation calculation, replacing it with the Cosmos SDK's built-in ApproxSqrt() method. This addresses issue #188 and improves the safety and determinism of the calculation.

Key changes:

  • Replaced float conversion and math.Sqrt with ApproxSqrt() in the standard deviation calculation
  • Updated the test to use tolerance-based comparison appropriate for the approximation function
  • Added changelog entry documenting the fix

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
x/oracle/types/ballot.go Replaced unsafe math.Sqrt with SDK's ApproxSqrt() method for calculating standard deviation
x/oracle/types/ballot_test.go Updated test to use tolerance-based comparison instead of exact equality to accommodate the approximation
CHANGELOG.md Added entry documenting the removal of unsafe math.Sqrt usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jhelison jhelison merged commit b25b198 into main Dec 30, 2025
14 of 16 checks passed
@jhelison jhelison deleted the fix/safe-ballot branch December 30, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@AndresRamirez9912 AndresRamirez9912 AndresRamirez9912 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jhelison @AndresRamirez9912