Open
Conversation
* Removed test files aggregating all type's tests * Refactor fixed-point test suite, add more test cases * Format files * tests: apply AI comments * fix: apply review comments * fix: apply comment --------- Co-authored-by: Daniel Bigos <daniel.bigos@icloud.com>
* feat: update CHANGELOG * feat: apply improvements
* feat: update CHANGELOG * feat: update comment * feat: add more doc entries * feat: add more comments * feat: apply copilot suggestions
* test: change expect helper function into macro * test: fmt * test: remove expect macro * test: use assert_eq instead of .eq on fp values
* ref: use binary exponentiation in sd29x9 pow * test: change pow invariant to exact result value * ref: add newline * doc: update * fix: assert base limit in sd29x9 pow * test: increase exp in pow_overflow_aborts_for_large_base * ref: apply binary exponentiation in ud30x9::pow * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Enhance pow_tests with grouping behavior check (#283) * 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. * Improve expect_ne macro with debugging output Refactor expect_ne macro to unwrap values and add debug output on assertion failure. * Refactor expect_ne macro for better readability Refactor expect_ne macro to unwrap values after assignment. * Fix debug print statements in assertion check * Enhance ud30x9 pow tests with expect_ne for validation Added expect_ne helper to validate non-equality in pow tests. * Enhance pow test with comment on exponent behavior Add comment about rounding/truncation behavior in pow test. * Update math/fixed_point/sources/sd29x9/sd29x9_base.move Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * test: Add expect_ne macro for inequality assertions in ud30x9 * Refactor pow test for clarity and reuse result * test: precalculate val.pow(255) in ud30x9 * test: fix to use fixed instead of pos in ud30x9 * refactor: extract binary_pow helper, replace expect with assert_eq in 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> * refactor: format binary_pow parameters and adjust pow overflow test exponent * ref: revert pow logic: no longer use internal helper * test: remove redundant unwraps * test: revert changes to unwrap * ref: shadow exp instead of mutating fun param --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bidzyyys <25967634+bidzyyys@users.noreply.github.com>
* Add `rem` function for truncated remainder semantics on SD29x9 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Redefine `mod` on SD29x9 to use Euclidean remainder semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update changelog * Improve the mod code slightly * Remove unnecessary dots from the changelog --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Daniel Bigos <daniel.bigos@openzeppelin.com> Co-authored-by: Nenad <nenad.misic@openzeppelin.com>
* feat: update CHANGELOG * feat: add conversion API * feat: add pending files * feat: improve API * feat: add missing files * feat: update CHANGELOG * feat: remove casting * feat: reuse macro constant * feat: apply review updates * fix: nits * feat: apply review updates --------- Co-authored-by: Daniel Bigos <daniel.bigos@openzeppelin.com> Co-authored-by: Nenad <nenad.misic@openzeppelin.com>
* feat: update CHANGELOG * refactor: remove bitwise operations for SD --------- Co-authored-by: Nenad <nenad.misic@openzeppelin.com>
* feat: update CHANGELOG * feat: apply review updates * feat: add division checks * feat: apply review updates * feat: apply copilot suggestions * feat: centralize common helpers * feat: update CHNGELOG * feat: apply suggestion
…. value (#280) * ref: sd29x9::pow intermediate pos. value must be < min. neg. value * refactor: rename limit to min_negative and simplify overflow check in pow Agent-Logs-Url: https://github.com/OpenZeppelin/contracts-sui/sessions/af15164d-616a-4f49-9fd9-8abc5f612c5f Co-authored-by: 0xNeshi <19427053+0xNeshi@users.noreply.github.com> * chore: update changelog * chore: fix pr id * ref: rename min_negative->max_mag in wrap_components * refactor: use macro expressions for max whole value constants --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
…hift (#288) * feat: rename lshift -> unchecked_lshift * chore: update changelog * docs: add missing error documentation for EOverflow in ud30x9 * ref: use ud30x9::zero instead of manual wrap(0) * feat: add new checked version of ud30x9::lshift * feat: rename ud30x9::rshift -> unchecked_rshift * feat: add new checked version of ud30x9::rshift * docs: mention unchecked shifts in README * chore: add missing PR ids to new changelog entries * ref: reimplement to avoid casts in lshift * ref: remove zero-value special cases from lshift and rshift * feat: add EInvalidShiftSize error for shift operations Replace EOverflow with EInvalidShiftSize for shift size validation in lshift and rshift functions. Update corresponding test cases to expect the new error code. * chore: fix pr id * chore: reword changelog * docs: clarify EOverflow range * docs: reference checked and unchecked shifts in each other's docs * chore: move back `rem` changeling * chore: remove rem entry from changed * refactor: use `std::u128::max_value!()` macro in `lshift`
#298) * feat: use insertion sort for vectors with up to 10 elements * ref: replace Lomuto partition with three-way partitioning in quicksort Replace the Lomuto partition scheme with three-way partitioning (Dutch National Flag scheme) to handle duplicate elements more efficiently. Move insertion sort optimization from preprocessing to inline handling of small sub-partitions (≤10 elements) during partitioning. Remove the standalone `insertion_sort_by` helper macro as it's now inlined. * chore: changelog * chore: fix wording in changelog * fix: update vector size check * docs: reword three way partition comment * test: add quick_sort tests * test: remove redundant quick_sort test cases Remove three test cases that are already covered by existing tests: quick_sort_partition_edge_case_pivot - Subset of quick_sort_already_sorted; ≤10 elements doesn't test pivot quick_sort_single_large_value_at_start - Same pattern as quick_sort_mostly_sorted_with_one_outlier quick_sort_single_small_value_at_end - Same pattern as quick_sort_mostly_sorted_with_one_outlier_at_end * ref: swap in insertion sort only when prev. is strictly less * test: remove comment mentions of specific code lines * test: clarify comment and fix test function name Update comment in quick_sort_by_descending_large_vector to clarify that >10 elements forces quicksort partitioning before insertion sort. Rename quick_sort_by_sort_by_id_ascending to quick_sort_by_id_ascending to remove redundant "sort_by" prefix. * chore: mention quick_sort in changelog * docs: align generic type name with actual quick_sort_by generic name * test: increase code coverage with quick_sort tests * Revert "test: increase code coverage with quick_sort tests" This reverts commit 47439ca. * docs: clarify quick_sort worst-case complexity and comparator requirements Update documentation for `quick_sort` and `quick_sort_by` to clarify that O(n²) worst-case is practically unreachable due to median-of-three pivot selection and three-way partitioning. Add warning that strict comparators (e.g., `<` instead of `<=`) can degrade performance to O(n²) with duplicate elements by defeating three-way partitioning optimization. * docs: clarify impact of sctrict comparison operators * docs: soften wording for worst case scenario * test: ensure quick_sort_by_median_of_three_crafted_inputs covers all branches * fix: handle empty, single-element & edge cases in quick_sort * docs: align range notation for eq_end --------- Co-authored-by: Daniel Bigos <daniel.bigos@openzeppelin.com>
…c/main-with-release-v1.1
WalkthroughUpdated Sui CLI dependency to v1.69.2, refactored fixed-point arithmetic APIs for UD30x9 and SD29x9 with new conversion modules, rounding-direction variants (trunc/away), and changed remainder semantics. Removed SD29x9 bitwise operations, optimized pow via binary exponentiation, and refactored quicksort from Lomuto to Dutch National Flag partitioning. Expanded test coverage across both fixed-point types with new conversion, rounding, and edge-case tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
+ Coverage 89.88% 90.21% +0.32%
==========================================
Files 19 21 +2
Lines 1790 1891 +101
Branches 484 506 +22
==========================================
+ Hits 1609 1706 +97
Misses 168 168
- Partials 13 17 +4
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
math/fixed_point/tests/ud30x9_tests/casting_tests.move (1)
19-20: Consider removing one redundant zero assertion.
assert!(converted.is_zero())andassert_eq!(converted, sd29x9::zero())validate the same condition; keeping one is enough unless you explicitly want both forms documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@math/fixed_point/tests/ud30x9_tests/casting_tests.move` around lines 19 - 20, Two assertions check the same condition for the variable converted: assert!(converted.is_zero()) and assert_eq!(converted, sd29x9::zero()); remove one to eliminate redundancy—prefer keeping either assert_eq!(converted, sd29x9::zero()) for explicit equality against the zero constructor or assert!(converted.is_zero()) if you prefer a boolean-style check; update the tests/ud30x9 casting_tests.move to delete the redundant line accordingly.math/fixed_point/README.md (1)
47-57: Tighten the cast example to avoid mixing conversion in the “casts” section.The first line in this section uses
ud30x9_convert::from_u128(42), which is a conversion API. Consider starting from a rawud30x9::wrap(...)value so the section stays purely about casts.♻️ Suggested doc tweak
-use openzeppelin_fp_math::{sd29x9, ud30x9_convert}; +use openzeppelin_fp_math::{sd29x9, ud30x9}; -let unsigned = ud30x9_convert::from_u128(42); // 42.0 +let unsigned = ud30x9::wrap(42_000_000_000); // raw bits for 42.0 let signed = unsigned.into_SD29x9(); // 42.0 as SD29x9 let roundtrip = signed.into_UD30x9(); // 42.0 as UD30x9🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@math/fixed_point/README.md` around lines 47 - 57, The example in the "Cross-Type Fixed-Point Casts" section mixes a conversion API (ud30x9_convert::from_u128) with cast examples; replace the initial construction with the raw wrapper (ud30x9::wrap(...)) so the snippet demonstrates only casts. Specifically, change the source value creation from ud30x9_convert::from_u128(42) to using ud30x9::wrap(42u128) (or the appropriate literal) and keep the subsequent calls into_SD29x9() and into_UD30x9() so the example shows purely cross-type casting via into_SD29x9 and into_UD30x9.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 31: Remove `.gitmodules` from the .gitignore so repository submodule
metadata is tracked; open the .gitignore, delete the `.gitmodules` entry (or
uncomment it), stage and commit the change so future submodule additions/updates
will include the required `.gitmodules` file in commits; ensure no other tooling
relies on ignoring that filename.
In `@math/fixed_point/tests/sd29x9_tests/pow_tests.move`:
- Around line 51-53: The explanatory comment referencing the pre-floor value for
1.25^16 * 10^9 is off by one; update the comment near the assert using
val.pow(16) / pos(...) (and mention sd29x9::pow) to state the correct pre-floor
value (approximately 35_527_136_788.005...), e.g. replace "35_527_136_787" with
"35_527_136_788" (or "≈35_527_136_788.005...") so the comment accurately
reflects the true unfloored value before asserting pos(35_527_136_781).
---
Nitpick comments:
In `@math/fixed_point/README.md`:
- Around line 47-57: The example in the "Cross-Type Fixed-Point Casts" section
mixes a conversion API (ud30x9_convert::from_u128) with cast examples; replace
the initial construction with the raw wrapper (ud30x9::wrap(...)) so the snippet
demonstrates only casts. Specifically, change the source value creation from
ud30x9_convert::from_u128(42) to using ud30x9::wrap(42u128) (or the appropriate
literal) and keep the subsequent calls into_SD29x9() and into_UD30x9() so the
example shows purely cross-type casting via into_SD29x9 and into_UD30x9.
In `@math/fixed_point/tests/ud30x9_tests/casting_tests.move`:
- Around line 19-20: Two assertions check the same condition for the variable
converted: assert!(converted.is_zero()) and assert_eq!(converted,
sd29x9::zero()); remove one to eliminate redundancy—prefer keeping either
assert_eq!(converted, sd29x9::zero()) for explicit equality against the zero
constructor or assert!(converted.is_zero()) if you prefer a boolean-style check;
update the tests/ud30x9 casting_tests.move to delete the redundant line
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9b39010-1f87-4b96-88cd-7b8ebe693dd3
⛔ Files ignored due to path filters (1)
math/fixed_point/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.github/workflows/test.yml.gitignoreCHANGELOG.mdREADME.mdmath/README.mdmath/core/sources/u256.movemath/core/sources/vector.movemath/core/tests/macros/quick_sort.movemath/core/tests/u256_tests.movemath/fixed_point/README.mdmath/fixed_point/sources/casting/u128.movemath/fixed_point/sources/internal/common.movemath/fixed_point/sources/sd29x9/conversions.movemath/fixed_point/sources/sd29x9/sd29x9.movemath/fixed_point/sources/sd29x9/sd29x9_base.movemath/fixed_point/sources/ud30x9/conversions.movemath/fixed_point/sources/ud30x9/ud30x9.movemath/fixed_point/sources/ud30x9/ud30x9_base.movemath/fixed_point/tests/sd29x9_tests/abs_tests.movemath/fixed_point/tests/sd29x9_tests/arithmetic_tests.movemath/fixed_point/tests/sd29x9_tests/bitwise_tests.movemath/fixed_point/tests/sd29x9_tests/casting_tests.movemath/fixed_point/tests/sd29x9_tests/ceil_tests.movemath/fixed_point/tests/sd29x9_tests/comparison_tests.movemath/fixed_point/tests/sd29x9_tests/conversion_tests.movemath/fixed_point/tests/sd29x9_tests/div_tests.movemath/fixed_point/tests/sd29x9_tests/floor_tests.movemath/fixed_point/tests/sd29x9_tests/helpers.movemath/fixed_point/tests/sd29x9_tests/mod_tests.movemath/fixed_point/tests/sd29x9_tests/mul_tests.movemath/fixed_point/tests/sd29x9_tests/negate_tests.movemath/fixed_point/tests/sd29x9_tests/pow_tests.movemath/fixed_point/tests/sd29x9_tests/rem_tests.movemath/fixed_point/tests/sd29x9_tests/unchecked_tests.movemath/fixed_point/tests/sd29x9_tests/wrap_tests.movemath/fixed_point/tests/ud30x9_tests/abs_tests.movemath/fixed_point/tests/ud30x9_tests/arithmetic_tests.movemath/fixed_point/tests/ud30x9_tests/bitwise_tests.movemath/fixed_point/tests/ud30x9_tests/casting_tests.movemath/fixed_point/tests/ud30x9_tests/ceil_tests.movemath/fixed_point/tests/ud30x9_tests/comparison_tests.movemath/fixed_point/tests/ud30x9_tests/conversion_tests.movemath/fixed_point/tests/ud30x9_tests/div_tests.movemath/fixed_point/tests/ud30x9_tests/floor_tests.movemath/fixed_point/tests/ud30x9_tests/helpers.movemath/fixed_point/tests/ud30x9_tests/mul_tests.movemath/fixed_point/tests/ud30x9_tests/pow_tests.movemath/fixed_point/tests/ud30x9_tests/unchecked_tests.movemath/fixed_point/tests/ud30x9_tests/wrap_tests.move
💤 Files with no reviewable changes (4)
- math/fixed_point/tests/ud30x9_tests/helpers.move
- math/fixed_point/sources/casting/u128.move
- math/fixed_point/tests/sd29x9_tests/helpers.move
- math/fixed_point/tests/sd29x9_tests/bitwise_tests.move
|
|
||
| # Claude Code | ||
| .claude/ | ||
| .gitmodules |
There was a problem hiding this comment.
Don't ignore .gitmodules.
That file is repository metadata, not local tooling state. Keeping it ignored makes future submodule additions/updates easy to commit incorrectly because the required .gitmodules change can be skipped by normal staging flows.
Suggested change
-.gitmodules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .gitmodules |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 31, Remove `.gitmodules` from the .gitignore so
repository submodule metadata is tracked; open the .gitignore, delete the
`.gitmodules` entry (or uncomment it), stage and commit the change so future
submodule additions/updates will include the required `.gitmodules` file in
commits; ensure no other tooling relies on ignoring that filename.
Comment on lines
+51
to
+53
| // Expected value is the result of `sd29x9::pow`'s binary exponentiation with intermediate | ||
| // truncation, not simply floor(1.25^16 * 10^9) (the exact value before flooring is 35_527_136_787) | ||
| assert_eq!(val.pow(16), pos(35_527_136_781)); |
There was a problem hiding this comment.
Correct the numeric reference in the explanatory comment.
On Line 52, the stated pre-floor value (35_527_136_787) appears off by one for 1.25^16 * 10^9 (approximately 35_527_136_788.005...).
✏️ Suggested comment fix
- // truncation, not simply floor(1.25^16 * 10^9) (the exact value before flooring is 35_527_136_787)
+ // truncation, not simply floor(1.25^16 * 10^9) (the exact value before flooring is ~35_527_136_788.005...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Expected value is the result of `sd29x9::pow`'s binary exponentiation with intermediate | |
| // truncation, not simply floor(1.25^16 * 10^9) (the exact value before flooring is 35_527_136_787) | |
| assert_eq!(val.pow(16), pos(35_527_136_781)); | |
| // Expected value is the result of `sd29x9::pow`'s binary exponentiation with intermediate | |
| // truncation, not simply floor(1.25^16 * 10^9) (the exact value before flooring is ~35_527_136_788.005...) | |
| assert_eq!(val.pow(16), pos(35_527_136_781)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@math/fixed_point/tests/sd29x9_tests/pow_tests.move` around lines 51 - 53, The
explanatory comment referencing the pre-floor value for 1.25^16 * 10^9 is off by
one; update the comment near the assert using val.pow(16) / pos(...) (and
mention sd29x9::pow) to state the correct pre-floor value (approximately
35_527_136_788.005...), e.g. replace "35_527_136_787" with "35_527_136_788" (or
"≈35_527_136_788.005...") so the comment accurately reflects the true unfloored
value before asserting pos(35_527_136_781).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
mul_trunc,mul_away,div_trunc,div_awayrem) with distinct semantics from moduloBug Fixes
Documentation