feat: sqrt implementation and tests
#53
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughImplements square root functionality across all unsigned integer types (u8–u256) with configurable rounding modes. The implementation includes a core floor-square-root algorithm using Newton-Raphson iteration, a rounding utility for multiple rounding strategies, and public API functions for each integer type. Changes
Sequence DiagramsequenceDiagram
participant User
participant PublicAPI as sqrt(value, rounding_mode)<br/>on u8/u16/u32/u64/u128/u256
participant Macro as sqrt!<br/>macro
participant Common as sqrt_floor(a: u256)<br/>in common.move
participant Round as round_sqrt_result(...)
User->>PublicAPI: Call sqrt(value, rounding_mode)
PublicAPI->>Macro: Delegate to macros::sqrt!
Macro->>Macro: Normalize value to u256
Macro->>Common: Call sqrt_floor(normalized_value)
Common->>Common: Edge-case check (a <= 1)
Common->>Common: Initial high-precision estimate
Common->>Common: Iterative Newton-Raphson refinement
Common-->>Macro: Return floor(sqrt(a))
Macro->>Round: Call round_sqrt_result(value, floor_result, rounding_mode)
Round->>Round: Apply rounding logic (down/up/nearest)<br/>and handle perfect squares
Round-->>Macro: Return rounded sqrt result
Macro-->>PublicAPI: Return result cast to original type
PublicAPI-->>User: Return sqrt(value, rounding_mode)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
math/core/sources/internal/common.move (1)
33-144: LGTM! Well-implemented integer square root using Newton-Raphson.The implementation is mathematically sound and well-documented. The Newton-Raphson method with initial bit-length-based estimation and iterative refinement correctly computes floor(sqrt(a)). The extensive comments explain the convergence properties clearly.
One minor typo on Line 136: "we know have" should be "we now have".
Apply this diff to fix the typo:
- // Because e ≤ 128 (as discussed during the first estimation phase), we know have reached a precision + // Because e ≤ 128 (as discussed during the first estimation phase), we now have reached a precision
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
math/core/sources/internal/common.move(1 hunks)math/core/sources/internal/macros.move(2 hunks)math/core/sources/u128.move(1 hunks)math/core/sources/u16.move(1 hunks)math/core/sources/u256.move(1 hunks)math/core/sources/u32.move(1 hunks)math/core/sources/u64.move(1 hunks)math/core/sources/u8.move(1 hunks)math/core/tests/common_tests.move(1 hunks)math/core/tests/macros_tests.move(1 hunks)math/core/tests/u128_tests.move(1 hunks)math/core/tests/u16_tests.move(1 hunks)math/core/tests/u256_tests.move(1 hunks)math/core/tests/u32_tests.move(1 hunks)math/core/tests/u64_tests.move(1 hunks)math/core/tests/u8_tests.move(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u8.move:50-57
Timestamp: 2025-11-04T14:38:12.309Z
Learning: In math/core/sources/{u8,u16,u32,u64,u128}.move files, the mul_shr implementations correctly ignore the overflow flag from macros::mul_shr! and rely on try_as_*() to detect when the result doesn't fit in the target type. Only u256.move needs to explicitly check the overflow flag because u256 operations can exceed u256 capacity.
<!-- [/add_learning]
📚 Learning: 2025-11-04T14:38:12.309Z
Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u8.move:50-57
Timestamp: 2025-11-04T14:38:12.309Z
Learning: In math/core/sources/{u8,u16,u32,u64,u128}.move files, the mul_shr implementations correctly ignore the overflow flag from macros::mul_shr! and rely on try_as_*() to detect when the result doesn't fit in the target type. Only u256.move needs to explicitly check the overflow flag because u256 operations can exceed u256 capacity.
<!-- [/add_learning]
Applied to files:
math/core/sources/internal/common.movemath/core/sources/u16.movemath/core/tests/u16_tests.movemath/core/sources/u32.movemath/core/sources/u8.movemath/core/sources/u64.movemath/core/tests/common_tests.movemath/core/tests/u256_tests.movemath/core/tests/u8_tests.movemath/core/tests/macros_tests.movemath/core/sources/u256.movemath/core/tests/u64_tests.movemath/core/sources/u128.movemath/core/tests/u32_tests.movemath/core/tests/u128_tests.movemath/core/sources/internal/macros.move
📚 Learning: 2025-11-04T14:39:02.711Z
Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u64.move:49-56
Timestamp: 2025-11-04T14:39:02.711Z
Learning: In mul_shr implementations for integer types smaller than u256 (u8, u16, u32, u64, u128), the overflow flag from macros::mul_shr! indicates u256 overflow, which is mathematically impossible for these smaller types. The pattern `let (_, result) = macros::mul_shr!(...); result.try_as_X()` is correct, as try_as_X() handles the conversion to Option<X> and returns None when the result doesn't fit in the target type. Only u256::mul_shr needs to explicitly check the overflow flag.
Applied to files:
math/core/tests/u256_tests.movemath/core/tests/macros_tests.movemath/core/sources/u256.movemath/core/sources/internal/macros.move
📚 Learning: 2025-11-04T14:38:12.309Z
Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u8.move:50-57
Timestamp: 2025-11-04T14:38:12.309Z
Learning: In the OpenZeppelin Sui contracts math library, the `mul_shr!` and `mul_div!` macros return (overflow, result) tuples where overflow indicates the result cannot fit in u256. For integer types smaller than u256 (u8, u16, u32, u64, u128), the overflow flag can be safely ignored because the product of two values of these types will always fit in u256. The `try_as_*()` methods on the result handle the case where the u256 result doesn't fit in the target type by returning None.
<!-- [/add_learning]
Applied to files:
math/core/sources/u256.movemath/core/sources/internal/macros.move
📚 Learning: 2025-11-04T14:37:40.142Z
Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u16.move:49-56
Timestamp: 2025-11-04T14:37:40.142Z
Learning: In the mul_shr! macro (and similar macros like mul_div!), the overflow flag indicates whether the result overflows u256, not the target type. For smaller types like u8, u16, u32, u64, and u128, the overflow flag will never be true because intermediate calculations always fit in u256. The try_as_uX() methods handle cases where the u256 result doesn't fit in the target type.
Applied to files:
math/core/sources/internal/macros.move
🔇 Additional comments (8)
math/core/tests/common_tests.move (1)
74-178: LGTM! Comprehensive test coverage for sqrt_floor.The test suite thoroughly covers the sqrt_floor function across all scenarios:
- Zero and edge cases
- Perfect squares across multiple orders of magnitude
- Non-perfect squares with floor rounding verification
- All unsigned integer widths (u8–u256)
- Maximum values for each width
math/core/tests/u128_tests.move (1)
462-566: LGTM! Thorough test coverage for u128::sqrt.The test suite comprehensively validates u128::sqrt behavior:
- All rounding modes (down, up, nearest) with multiple test cases
- Perfect squares return exact results regardless of rounding
- Edge cases including zero and max_value
- Powers of four (perfect squares of powers of two)
- Midpoint behavior for nearest-rounding
The tests follow the established pattern from log2/log256 tests and ensure correctness across the full range of u128.
math/core/sources/u8.move (1)
83-89: LGTM! Consistent API addition.The sqrt function follows the established pattern in this module, delegating to the macro implementation and providing clear documentation. The signature and behavior are consistent with other mathematical functions (log2, log256).
math/core/sources/u256.move (1)
76-82: LGTM! Consistent API addition.The sqrt function follows the established pattern in this module, with proper documentation and delegation to the macro implementation.
math/core/tests/u64_tests.move (1)
461-565: LGTM! Comprehensive test coverage for u64::sqrt.The test suite thoroughly validates u64::sqrt behavior across all rounding modes, edge cases, and boundary values. The tests correctly verify that sqrt(u64::MAX) = 4294967295 for down-rounding and 4294967296 for up/nearest rounding.
math/core/tests/u8_tests.move (1)
409-523: LGTM! Excellent test coverage for u8::sqrt.The test suite provides exceptional coverage for the u8 range, including all perfect squares from 1 to 225, comprehensive rounding mode tests, and correct max value handling. The tests ensure sqrt behaves correctly across the entire u8 domain.
math/core/sources/u16.move (1)
82-88: LGTM! Consistent API addition.The sqrt function follows the established pattern across all width-specific modules, with proper documentation and delegation to the macro implementation. This completes the sqrt API surface across u8, u16, u32, u64, u128, and u256 types.
math/core/sources/internal/macros.move (1)
642-660: Nearest rounding guard is well derived.Nice to see the half-up midpoint handled with the integer inequality
value - floor² <= floor, which keeps the path branch-free and matches the tie-up behaviour we want.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 89.31% 90.23% +0.92%
==========================================
Files 10 10
Lines 805 881 +76
Branches 272 292 +20
==========================================
+ Hits 719 795 +76
Misses 71 71
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bidzyyys
changed the title
sqrt implementation and testssqrt implementation and tests
ericnordelo
left a comment
ericnordelo
left a comment
There was a problem hiding this comment.
Great job @immrsd.
Only comment, non-blocking and for your consideration (fill free to chose either way) is that maybe it makes sense to return the previous integer type on each public function, since it fits, and then people don't need to use try_as to convert if they need it.
@ericnordelo I’ve considered that option but don’t think it’s feasible. The result of On top of that, I find it counter-intuitive to return a |
Resolves #21
PR Checklist
Summary by CodeRabbit
New Features
Tests