ref: restructure fixed_point package

[immrsd]

I ran into an issue while implementing the div function (and adding an abort with a custom error for mod). I needed to declare additional error constants in the sd29x9_base module, but an EOverflow error was already declared in the sd29x9 file. On top of that, aborting with EOverflow from sd29x9_base is problematic, since constants cannot be imported in Move (and making a separate abort function is weird).

I experimented with several approaches, starting with moving most of the implementation logic into sd29x9_base (including the wrap function). However, this led to issues when constructing SD29x9 values from a module that does not declare the type.

After some consideration, I came to the conclusion that the best approach is to declare a fixed-point type and its math functions in the same file and restructured the package accordingly.

Summary by CodeRabbit

• Refactor
  • Reorganized fixed-point math module structure for SD29x9 and UD30x9 types
  • Updated public APIs with new helper functions (wrap, unwrap, min, max, zero) for fixed-point operations

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes restructure the SD29x9 and UD30x9 fixed-point type modules by consolidating implementations from nested files into their parent modules. This eliminates the nested file hierarchy while maintaining public APIs and adding new constructors and conversion helpers.

Changes

Cohort / File(s)	Summary
SD29x9 Module Consolidation math/fixed_point/sources/sd29x9.move	Consolidates SD29x9 implementation into parent module; adds public struct, constants (MAX_POSITIVE_VALUE, MIN_NEGATIVE_VALUE, U128_MAX_VALUE), and public functions (unwrap, wrap, zero, min, max, from_bits); updates bitwise operations to use U128_MAX_VALUE; introduces bounds checking in wrap function.
SD29x9 Nested Implementation Removal math/fixed_point/sources/sd29x9/sd29x9.move	Removes entire SD29x9 module file including struct definition, error type EOverflow, re-exported base operations, and utility functions (two_complement, from_bits).
UD30x9 Module Consolidation math/fixed_point/sources/ud30x9.move	Consolidates UD30x9 implementation into parent module; adds public struct, constants (MAX_VALUE, SCALE), and public functions (wrap, unwrap, zero, max); expands public API surface.
UD30x9 Nested Implementation Removal math/fixed_point/sources/ud30x9/ud30x9.move	Removes entire UD30x9 module file including struct definition, re-exported base operations, and conversion/accessor functions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Add ud30x9 , sd29x9 , and base functions #129: Directly modifies the same fixed-point types (SD29x9 and UD30x9), adding struct definitions and wrap/unwrap helpers with overlapping functionality.
• feat: implement abs, ceil, div #132: Extends the public API surface of SD29x9 and UD30x9 modules by exposing arithmetic and bitwise operations through re-exports and new constructors.

Suggested reviewers

• qalisander
• ericnordelo
• bidzyyys

Poem

🐰 Nested files collapse and consolidate with grace,
SD29x9 and UD30x9 find their rightful place,
Parent modules embrace their young,
Where wrap and zero functions hum,
A cleaner home for fixed-point math—hooray! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description clearly explains the motivation (error constant management and aborts), the problem with initial approaches, and the chosen solution, but lacks specific information about testing, documentation, and changelog as required by the template.	Add sections addressing Tests, Documentation, and Changelog checklist items to fully comply with the PR template requirements.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title 'ref: restructure fixed_point package' is directly related to the core objective of this PR, which involves reorganizing the fixed-point module structure by moving type definitions and operations into consolidated files rather than splitting them across separate modules.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/fixed-point-restructure

---

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

[codecov]

Codecov Report

❌ Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.66%. Comparing base (939f59f) to head (28e6d17).
⚠️ Report is 12 commits behind head on main.

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #134       +/-   ##
===========================================
+ Coverage   74.82%   96.66%   +21.83%     
===========================================
  Files          13       16        +3     
  Lines         433     1440     +1007     
  Branches      110      366      +256     
===========================================
+ Hits          324     1392     +1068     
+ Misses        106       27       -79     
- Partials        3       21       +18     

Flag	Coverage Δ
contracts/access	53.50% <ø> (ø)
math/core	95.96% <ø> (?)
math/fixed_point	64.83% <97.56%> (-0.12%)	⬇️

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.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

math/fixed_point/sources/ud30x9.move (1)

60-71: Document overflow behavior for near-maximum values.

The ceil function will abort on overflow when x has a non-zero fractional part and its integer component is close to MAX_VALUE. For example, ceil(max()) will overflow because the integer part plus SCALE exceeds u128::MAX.

This is mathematically correct (the ceiling cannot be represented), but consider documenting this edge case in the function comment or providing a try_ceil variant that returns Option<UD30x9>.

🧹 Nitpick comments (3)

math/fixed_point/sources/sd29x9.move (3)

72-77: Consider documenting abort behavior for abs(min()).

When called with min() (representing -2^127), abs will abort because |−2^127| = 2^127 exceeds MAX_POSITIVE_VALUE (2^127 − 1). This is mathematically correct behavior for signed integers, but documenting this edge case would help users avoid unexpected aborts.

---

154-159: Redundant mask in lshift.

The & U128_MAX_VALUE mask on line 158 is redundant because the left shift on a u128 already produces a u128 result with upper bits naturally discarded.

Suggested simplification

 public fun lshift(x: SD29x9, bits: u8): SD29x9 {
     if (bits >= 128) {
         return zero()
     };
-    from_bits((x.unwrap() << bits) & U128_MAX_VALUE)
+    from_bits(x.unwrap() << bits)
 }

---

181-187: Missing division-by-zero check in mod.

The PR description mentions wanting to add a custom abort for mod. Currently, if y is zero, line 185 will abort with Move's default division-by-zero error rather than a custom EOverflow or dedicated error constant.

If a custom error is desired for consistency with other operations, consider adding an explicit check:

Suggested implementation with explicit error

+#[error(code = 1)]
+const EDivisionByZero: vector<u8> = b"Division by zero";
+
 public fun mod(x: SD29x9, y: SD29x9): SD29x9 {
     let x_components = decompose(x.unwrap());
     let y_components = decompose(y.unwrap());
+    if (y_components.mag == 0) {
+        abort EDivisionByZero
+    };
     let remainder = x_components.mag % y_components.mag;
     wrap_components(Components { neg: x_components.neg, mag: remainder })
 }

[bidzyyys]

LGTM!

[ericnordelo]

I needed to declare additional error constants in the sd29x9_base module, but an EOverflow error was already declared in the sd29x9 file. On top of that, aborting with EOverflow from sd29x9_base is problematic, since constants cannot be imported in Move (and making a separate abort function is weird).

The whole idea of the design of the two modules is to make the type easier to read and understand for users, making the library as readable as possible. I understand that module system in Sui is hard to deal with, but while I see the issue with redeclaring a similar error in two modules, I think that's not worst than putting everything in a single big file, making it harder to read for our users.

I vote we keep it, and bring the discussion to the Sui team to see if they have concerns with any of the approaches or strong opinions.

[immrsd]

IMO splitting the type into 2 separate files and duplicating the constants and error codes makes it more difficult to reason about a type, not easier. In the initial design with 2 files most of the code of the sd29x9_base file was re-exporting the functions from sd29x9_base. And now we can get rid of those unnecessary 20 lines and avoid code duplication

[ericnordelo]

IMO splitting the type into 2 separate files and duplicating the constants and error codes makes it more difficult to reason about a type, not easier. In the initial design with 2 files most of the code of the sd29x9_base file was re-exporting the functions from sd29x9_base. And now we can get rid of those unnecessary 20 lines and avoid code duplication

Is not 20 extra lines, is just the 20 lines the user need to look to understand the type and the features, he only need to go to the other file to see each function implementation, which is usually not what users want. In this context, the alternative to removing the twenty lines is making users have to go through a few hundred/thousands lines long file if they want ot see what is avaiable in the type. I really don't think that's worth removing a few errors.

If there are other strong reasons like maybe error discoverability/tracking in the UI, maybe then is worth considering making the library harder to read, and that's why I suggest bringing Sui into the discussion. But I don't feel there should be that strong issues with errors justifying losing readability.

[bidzyyys]

@immrsd can we close that PR?

[bidzyyys]

IMHO we should close this PR for now.