fix: Fixed-Point pow Now Uses Binary Exponentiation#281
fix: Fixed-Point pow Now Uses Binary Exponentiation#2810xNeshi wants to merge 26 commits intorelease-v1.1from
pow Now Uses Binary Exponentiation#281Conversation
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-v1.1 #281 +/- ##
================================================
- Coverage 89.87% 89.79% -0.08%
================================================
Files 19 19
Lines 1787 1784 -3
Branches 484 490 +6
================================================
- Hits 1606 1602 -4
Misses 168 168
- Partials 13 14 +1
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:
|
There was a problem hiding this comment.
Pull request overview
Updates fixed-point pow implementations to use binary exponentiation, addressing issue #272’s concern about linear exponentiation (performance and accumulated truncation effects) in the fixed-point math packages.
Changes:
- Replaced step-by-step repeated multiplication in
UD30x9/SD29x9powwith a binary-exponentiation loop. - Expanded
powdocumentation to clarify truncation/associativity implications under the new grouping. - Updated
SD29x9powtests to assert new expected values for higher exponents and overflow behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| math/fixed_point/sources/ud30x9/ud30x9_base.move | Switches UD30x9::pow to binary exponentiation and updates docstring semantics accordingly. |
| math/fixed_point/sources/sd29x9/sd29x9_base.move | Switches SD29x9::pow to binary exponentiation while preserving sign handling, and updates documentation. |
| math/fixed_point/tests/sd29x9_tests/pow_tests.move | Adjusts high-exponent expectations and overflow test parameters to align with new pow behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Enhance pow_tests with grouping behavior check Add test for rounding behavior in binary exponentiation with large exponents. * Add expect_ne to sd29x9_pow_tests * Add inequality assertion macro for SD29x9 Added a new macro 'expect_ne' to assert inequality between two SD29x9 values.
Refactor expect_ne macro to unwrap values and add debug output on assertion failure.
Refactor expect_ne macro to unwrap values after assignment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… pow_tests, use pow(2) for overflow test Agent-Logs-Url: https://github.com/OpenZeppelin/contracts-sui/sessions/96584ab5-2b3d-4442-9775-45db402402a7 Co-authored-by: bidzyyys <25967634+bidzyyys@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
math/fixed_point/sources/internal/pow_u256.move (1)
11-11: Documentation inaccuracy:expdoes not need to be non-zero.The documentation states "Must be non-zero" for the
expparameter, but the implementation correctly handlesexp == 0(returnsscale, representing 1.0) andexp == 1(returnsbase). Since callers short-circuit these cases anyway, consider either removing this requirement or clarifying it as a precondition assumed by the callers.📝 Suggested documentation fix
/// #### Parameters /// - `base`: The base value in fixed-point representation (already scaled). -/// - `exp`: The exponent. Must be non-zero. +/// - `exp`: The exponent. /// - `scale`: The fixed-point scale factor (e.g. `10^9`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@math/fixed_point/sources/internal/pow_u256.move` at line 11, The docstring for the exponent parameter is inaccurate: it claims `exp` "Must be non-zero" even though the implementation of pow_u256 (and callers that short‑circuit exp==0 to return `scale` and exp==1 to return `base`) supports exp == 0; update the documentation in pow_u256 (the `exp` parameter description) to either remove the "Must be non-zero" requirement or change it to state that exp may be zero (with the behavior that exp==0 returns `scale` and exp==1 returns `base`), so the comment matches the implementation and caller assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@math/fixed_point/sources/internal/pow_u256.move`:
- Line 11: The docstring for the exponent parameter is inaccurate: it claims
`exp` "Must be non-zero" even though the implementation of pow_u256 (and callers
that short‑circuit exp==0 to return `scale` and exp==1 to return `base`)
supports exp == 0; update the documentation in pow_u256 (the `exp` parameter
description) to either remove the "Must be non-zero" requirement or change it to
state that exp may be zero (with the behavior that exp==0 returns `scale` and
exp==1 returns `base`), so the comment matches the implementation and caller
assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f4aea67-b438-4e36-a750-e4b949ba623a
📒 Files selected for processing (5)
math/fixed_point/sources/internal/pow_u256.movemath/fixed_point/sources/sd29x9/sd29x9_base.movemath/fixed_point/sources/ud30x9/ud30x9_base.movemath/fixed_point/tests/sd29x9_tests/pow_tests.movemath/fixed_point/tests/ud30x9_tests/pow_tests.move
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ericnordelo
left a comment
ericnordelo
left a comment
There was a problem hiding this comment.
LGTM!
It would be great to merge this PR first to avoid conflicts with constants, but non blocking by any means.
Switches
UD30x9andSD29x9powfrom linear repeated multiplication to binary exponentiation, addressing the performance and accumulated truncation concerns from issue #272.Changes:
UD30x9::powandSD29x9::powto delegate to the shared helper, eliminating per-iteration casts and redundant loop code.powdocumentation in both modules to clarify truncation and associativity implications under binary exponentiation grouping.SD29x9overflow test to use the minimal failing casesd29x9::max().pow(3)(fails thebase_mag <= MIN_NEGATIVE_VALUEassertion).PR Checklist
Summary by CodeRabbit
Refactor
Tests