feat: improve fp math conversion API#264
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 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 @@
## release-v1.1 #264 +/- ##
================================================
+ Coverage 89.87% 90.16% +0.28%
================================================
Files 19 21 +2
Lines 1787 1860 +73
Branches 484 493 +9
================================================
+ Hits 1606 1677 +71
Misses 168 168
- Partials 13 15 +2
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
This PR aims to clarify and improve the fixed-point “conversion vs raw casting” API by expanding the casting_u128 surface and refactoring shared fixed-point constants to a common place.
Changes:
- Add
casting_u128::into_SD29x9(u128, bool)and a test asserting it matchessd29x9::wrap. - Refactor
SD29x9/UD30x9implementations to usefixed_point_common::*for scale and bit-constant values. - Update README to distinguish “raw casting” from “whole-number conversions”, and update
Move.lockwith mainnet pins.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| math/fixed_point/tests/sd29x9_tests/wrap_tests.move | Adds a new test covering raw u128 -> SD29x9 casting behavior. |
| math/fixed_point/sources/casting/u128.move | Documents and adds raw casting helper into_SD29x9. |
| math/fixed_point/sources/sd29x9/sd29x9.move | Switches constants/limits to fixed_point_common. |
| math/fixed_point/sources/sd29x9/sd29x9_base.move | Switches scale/bit constants to fixed_point_common (including in pow). |
| math/fixed_point/sources/ud30x9/ud30x9.move | Switches scale/max constants to fixed_point_common. |
| math/fixed_point/sources/ud30x9/ud30x9_base.move | Switches scale/bit constants to fixed_point_common (including in pow). |
| math/fixed_point/README.md | Updates docs/examples around raw casting vs conversion modules. |
| math/fixed_point/Move.lock | Adds pinned mainnet entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
math/fixed_point/tests/sd29x9_tests/conversion_tests.move (1)
33-57: Optional: factor repeatedmax_wholesetup into a helper.This would trim duplication and make boundary intent easier to maintain across tests.
♻️ Suggested cleanup
+fun max_whole(): u128 { + MAX_POSITIVE_VALUE / SCALE +} + #[test] fun from_u128_scales_max_supported_whole_magnitude() { - let max_whole = MAX_POSITIVE_VALUE / SCALE; + let max_whole = max_whole(); let positive = sd29x9_convert::from_u128(max_whole, false); let negative = sd29x9_convert::from_u128(max_whole, true); @@ #[test] fun try_from_u128_returns_some_for_max_supported_whole_magnitude() { - let max_whole = MAX_POSITIVE_VALUE / SCALE; + let max_whole = max_whole(); let expected = sd29x9::wrap(max_whole * SCALE, false); assert_eq!(sd29x9_convert::try_from_u128(max_whole, false), option::some(expected)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@math/fixed_point/tests/sd29x9_tests/conversion_tests.move` around lines 33 - 57, Several tests in this file repeat computing max_whole = MAX_POSITIVE_VALUE / SCALE; factor that into a small helper (e.g., a private module-level function or a test helper like fn max_supported_whole() -> u128) and replace the repeated lines in tests from_u128_aborts_when_scaled_value_overflows, try_from_u128_returns_none_when_scaled_value_overflows, try_from_u128_returns_some_for_max_supported_whole_magnitude, and the earlier conversion test to call that helper; ensure the helper returns the same type used by sd29x9_convert::from_u128 and sd29x9::try_from_u128 and keep existing assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@math/fixed_point/sources/conversions/sd29x9.move`:
- Around line 118-121: to_u128_trunc (and try_to_u128_trunc) currently trust the
is_negative returned by to_parts_trunc which intentionally clears the sign for
values with whole == 0 (e.g., -0.5), allowing negative fractionals to slip
through; update to_u128_trunc and try_to_u128_trunc to inspect the original
SD29x9 sign directly (e.g., via an existing raw/sign accessor or an
is_negative_raw helper) before truncation and abort/return none for any negative
input, rather than relying on to_parts_trunc's is_negative; ensure to_u64_trunc
(which calls to_u128_trunc) inherits this corrected behavior.
---
Nitpick comments:
In `@math/fixed_point/tests/sd29x9_tests/conversion_tests.move`:
- Around line 33-57: Several tests in this file repeat computing max_whole =
MAX_POSITIVE_VALUE / SCALE; factor that into a small helper (e.g., a private
module-level function or a test helper like fn max_supported_whole() -> u128)
and replace the repeated lines in tests
from_u128_aborts_when_scaled_value_overflows,
try_from_u128_returns_none_when_scaled_value_overflows,
try_from_u128_returns_some_for_max_supported_whole_magnitude, and the earlier
conversion test to call that helper; ensure the helper returns the same type
used by sd29x9_convert::from_u128 and sd29x9::try_from_u128 and keep existing
assertions unchanged.
🪄 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: cc2283b7-14ee-4420-a4b6-f1ba7cb064cc
⛔ Files ignored due to path filters (1)
math/fixed_point/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
math/fixed_point/README.mdmath/fixed_point/sources/casting/u128.movemath/fixed_point/sources/conversions/sd29x9.movemath/fixed_point/sources/conversions/ud30x9.movemath/fixed_point/sources/internal/common.movemath/fixed_point/sources/sd29x9/sd29x9.movemath/fixed_point/sources/sd29x9/sd29x9_base.movemath/fixed_point/sources/ud30x9/ud30x9.movemath/fixed_point/sources/ud30x9/ud30x9_base.movemath/fixed_point/tests/sd29x9_tests/conversion_tests.movemath/fixed_point/tests/sd29x9_tests/wrap_tests.movemath/fixed_point/tests/ud30x9_tests/conversion_tests.move
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 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 14 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0xNeshi
left a comment
0xNeshi
left a comment
There was a problem hiding this comment.
Left responses to previous comments (see above)
bidzyyys
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// #### Returns | ||
| /// - `floor(u128::MAX / 10^9)`. | ||
| public(package) fun max_ud30x9_whole(): u128 { | ||
| (std::u128::max_value!()) / SCALE |
There was a problem hiding this comment.
| (std::u128::max_value!()) / SCALE | |
| std::u128::max_value!() / SCALE |
nit
| let inverted = bits ^ std::u128::max_value!(); | ||
| let sum = (inverted as u256) + 1; | ||
| (sum & (U128_MAX_VALUE as u256)) as u128 | ||
| (sum & ((std::u128::max_value!()) as u256)) as u128 |
There was a problem hiding this comment.
| (sum & ((std::u128::max_value!()) as u256)) as u128 | |
| (sum & (std::u128::max_value!() as u256)) as u128 |
nit: there are a couple of other places in the PR where redundant brackets are used like (std::u128::max_value!())
| } else { | ||
| let shifted = raw >> bits; | ||
| let mask = U128_MAX_VALUE << (128 - bits); | ||
| let mask = (std::u128::max_value!()) << (128 - bits); |
There was a problem hiding this comment.
| let mask = (std::u128::max_value!()) << (128 - bits); | |
| let mask = std::u128::max_value!() << (128 - bits); |
Resolves #267.
Summary by CodeRabbit
New Features
try_) and non-fallible conversion options with overflow handlingDocumentation
Tests
Refactor