Fixed point `sqrt` by immrsd · Pull Request #286 · OpenZeppelin/contracts-sui

immrsd · 2026-04-01T13:26:49Z

Resolves #90

Summary by CodeRabbit

Release Notes

New Features
- Added square root operations for fixed-point number types
- Extended API documentation for fixed-point arithmetic operations and casting routes
Tests
- Added comprehensive test coverage for square root functionality

coderabbitai · 2026-04-01T13:27:06Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1ee2095e-83d3-4fa5-b617-b261b6b61933

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Walkthrough

Added square root functionality to the fixed-point math library by introducing a shared fp_helpers module containing a sqrt_floor helper function implementing Newton's method, implementing sqrt for both SD29x9 and UD30x9 types, updating API documentation, and providing comprehensive test suites for both implementations.

Changes

Cohort / File(s)	Summary
Documentation `math/fixed_point/README.md`	Expanded API documentation to list additional arithmetic operations (`abs`, `ceil`, `div`, `floor`, `mul`, `pow`, `sqrt`) and added new casting/helper routes between types and constructor methods.
Core Implementation `math/fixed_point/sources/fp_helpers.move`, `math/fixed_point/sources/sd29x9/sd29x9_base.move`, `math/fixed_point/sources/ud30x9/ud30x9_base.move`	Introduced shared `sqrt_floor(a: u256)` helper using Newton's method for floor square root computation, implemented `sqrt(x: SD29x9)` with sign validation and magnitude scaling, and implemented `sqrt(x: UD30x9)` with raw value unwrapping and re-wrapping.
Public API Re-exports `math/fixed_point/sources/sd29x9/sd29x9.move`, `math/fixed_point/sources/ud30x9/ud30x9.move`	Added public re-exports mapping `sqrt` functions from base modules to top-level type namespaces for API consistency.
Test Coverage `math/fixed_point/tests/sd29x9_tests/sqrt_tests.move`, `math/fixed_point/tests/ud30x9_tests/sqrt_tests.move`	Added comprehensive test suites covering zero/one/perfect squares, fractional inputs, irrational truncation, floor property verification, roundtrip correctness, edge cases (max/min values), and monotonicity for both types; SD29x9 tests also verify negative input rejection.

Sequence Diagram(s)

sequenceDiagram
    participant UD30x9 as UD30x9::sqrt()
    participant SD29x9 as SD29x9::sqrt()
    participant Helper as fp_helpers::sqrt_floor()
    participant Newton as Newton Iteration
    
    rect rgba(100, 150, 255, 0.5)
    UD30x9->>Helper: sqrt_floor(raw * SCALE)
    end
    
    rect rgba(100, 200, 100, 0.5)
    SD29x9->>Helper: sqrt_floor(magnitude * SCALE)
    end
    
    rect rgba(200, 100, 100, 0.5)
    Helper->>Newton: Initial approximation + iterations
    Newton->>Newton: (xn + a/xn) >> 1
    Newton->>Helper: Refined result
    end
    
    Helper-->>UD30x9: floor(sqrt(a))
    Helper-->>SD29x9: floor(sqrt(a))
    UD30x9->>UD30x9: wrap(result)
    SD29x9->>SD29x9: wrap(result as SD29x9)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Add ud30x9 , sd29x9 , and base functions #129: Introduced the UD30x9 and SD29x9 fixed-point types that this PR extends with sqrt functionality.
Fix: SD29x9 overflow behaviour #203: Modifies the sd29x9_base module's internal structure and error handling, which directly overlaps with the sqrt implementation changes in this PR.

Suggested reviewers

0xNeshi
bidzyyys
ericnordelo

Poem

🐰 A rabbit hops through numbered fields so bright,
Where square roots bloom with Newton's guiding light,
From sqrt_floor to SD29x9,
Fixed-point math now feels so divine,
Tests and helpers dancing—oh what a sight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains 'Resolves `#90`' and is missing required checklist items (Tests, Documentation, Changelog) that should be marked as complete or pending.	Complete the PR description by adding the checklist with status updates for Tests, Documentation, and Changelog items to indicate what was done.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fixed point `sqrt`' is concise and clearly describes the main change: implementing square root functionality for fixed-point types.
Linked Issues check	✅ Passed	The PR successfully implements square root for fixed-point math, as requested in issue `#90`. Multiple commits add sqrt helper functions, implementations for SD29x9 and UD30x9, comprehensive tests, and documentation updates.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing sqrt functionality: helper functions, implementations for both fixed-point types, tests, and README documentation. No out-of-scope changes detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fixed-point-sqrt

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

codecov · 2026-04-01T13:30:41Z

Codecov Report

❌ Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.49%. Comparing base (0a996c9) to head (53fd0bd).

Files with missing lines	Patch %	Lines
math/fixed_point/sources/internal/common.move	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   90.32%   90.49%   +0.16%     
==========================================
  Files          21       21              
  Lines        1882     1925      +43     
  Branches      521      538      +17     
==========================================
+ Hits         1700     1742      +42     
  Misses        160      160              
- Partials       22       23       +1

Flag	Coverage Δ
contracts/access	`44.33% <ø> (ø)`
math/core	`86.30% <ø> (ø)`
math/fixed_point	`66.61% <97.82%> (+2.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0xNeshi · 2026-04-01T14:03:45Z

+public fun sqrt(x: UD30x9): UD30x9 {
+    let raw = x.unwrap() as u256;
+    if (raw == 0) {
+        return wrap(0)


Use zero()

Shouldn't be handled in fp_helpers?

0xNeshi · 2026-04-01T14:08:11Z

+    if (mag == 0) {
+        return zero()
+    };
+    let result = fp_helpers::sqrt_floor(mag * SCALE);


Add comment explaining why * SCALE is necessary

0xNeshi · 2026-04-01T14:08:22Z

+    if (raw == 0) {
+        return wrap(0)
+    };
+    let result = fp_helpers::sqrt_floor(raw * SCALE_U256);


Add comment explaining why * SCALE_U256 is necessary

0xNeshi · 2026-04-01T14:10:45Z

+public fun sqrt(x: SD29x9): SD29x9 {
+    let Components { neg, mag } = decompose(x.unwrap());
+    assert!(!neg, ENegativeSqrt);
+    if (mag == 0) {


Zero-check is redundant, as sqrt_floor performs it in the first line

0xNeshi · 2026-04-01T14:13:06Z

+
+#[test]
+fun sqrt_of_zero_is_zero() {
+    expect(fixed(0).sqrt(), fixed(0));


Suggested change

expect(fixed(0).sqrt(), fixed(0));

expect(ud30x9::zero().sqrt(), ud30x9::zero());

0xNeshi · 2026-04-01T14:19:38Z

+    values.destroy!(|x| {
+        let result = x.sqrt();
+        // Result is non-negative: raw bits should not have sign bit set
+        assert!(result.unwrap() <= 0x7FFF_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF);


It would be useful to have something like sd29x9::is_positive & sd29x9::is_negative, so this assertion would become:

assert!(result.is_positive());

0xNeshi · 2026-04-01T14:21:01Z

+#[test]
+fun sqrt_result_is_always_non_negative() {


Suggested change

#[test]

fun sqrt_result_is_always_non_negative() {

#[random_test]

fun sqrt_result_is_always_non_negative(raw: u128) {

Perfect candidate for a prop test

0xNeshi · 2026-04-01T14:24:36Z

+        assert!(r * r <= scaled);
+        assert!((r + 1) * (r + 1) > scaled);


These assertions are invariants on all non-negative values, making this a good candidate for a proptest.

Alternatively, these invariant assertions could be incorporated into sqrt_result_is_always_non_negative.

0xNeshi · 2026-04-01T14:26:36Z

+#[test]
+fun sqrt_floor_property_for_non_perfect_squares() {


these invariants hold for all valid values, not just non-perfect squares

therefore, candidate for prop. test

0xNeshi · 2026-04-01T14:28:15Z

+#[test]
+fun sqrt_monotonicity() {


Suggested change

#[test]

fun sqrt_monotonicity() {

#[random_test]

fun sqrt_monotonicity(x: u128, y: u128) {

good candidate for a prop. test

bidzyyys

Left some comments. @immrsd resolve conflicts first.

bidzyyys · 2026-04-27T12:44:21Z

+public fun sqrt(x: UD30x9): UD30x9 {
+    let raw = x.unwrap() as u256;
+    if (raw == 0) {
+        return wrap(0)


Shouldn't be handled in fp_helpers?

bidzyyys · 2026-04-27T12:47:46Z

+///
+/// #### Returns
+/// - `floor(sqrt(a))`.
+public(package) fun sqrt_floor(a: u256): u256 {


Shouldn't we use implementation from openzeppelin_math? I feel like it looks like code duplication.
Curious to hear your thoughts @ericnordelo & @0xNeshi 👀

That would require importing openzeppelin_math into openzeppelin_fp_math. If the final package size isn't significantly increased, i.e. the Move compiler prunes all unused openzeppelin_mathtypes/functions from the finalopenzeppelin_fp_math` package, I think this is the right approach.

Otherwise, it might be better to duplicate code to avoid unnecessary package size increase.

@ericnordelo do we know Move compiler's behavior in this case?

immrsd added 6 commits April 1, 2026 15:51

Add fixed-point helper for sqrt calculation

7c5064c

Implement sqrt for SD29x9 type

c0631d6

Implement sqrt for UD30x9

45d9674

Add tests for fixed-point sqrt impl

59e4a9e

Add sqrt and remaining functions to README

f100373

Merge main

25fd007

immrsd self-assigned this Apr 1, 2026

immrsd requested review from 0xNeshi, bidzyyys and ericnordelo as code owners April 1, 2026 13:26

Format files

06f659d

0xNeshi reviewed Apr 1, 2026

View reviewed changes

Use sqrt_floor implementation from math/core package

cedb015

bidzyyys reviewed Apr 27, 2026

View reviewed changes

immrsd added 6 commits April 29, 2026 14:13

Address review comments

699d6af

Merge main

63e7155

Move sqrt_floor function from fp_helpers to common module

2aa9db4

Fix tests to use assert_eq instead of deprecated expect

7e9cb7f

Remove redundant zero check in ud30x9::sqrt

b62c72c

Format files

53fd0bd

	expect(fixed(0).sqrt(), fixed(0));
	expect(ud30x9::zero().sqrt(), ud30x9::zero());

-#[test]
-fun sqrt_result_is_always_non_negative() {
+#[random_test]
+fun sqrt_result_is_always_non_negative(raw: u128) {

		assert!(r * r <= scaled);
		assert!((r + 1) * (r + 1) > scaled);

Conversation

immrsd commented Apr 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

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

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNeshi Apr 1, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bidzyyys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNeshi Apr 27, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

immrsd commented Apr 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 1, 2026 •

edited

Loading

codecov Bot commented Apr 1, 2026 •

edited

Loading

0xNeshi Apr 1, 2026 •

edited

Loading

0xNeshi Apr 27, 2026 •

edited

Loading