Skip to content

Conversation

@ozgunozerk
Copy link
Collaborator

@ozgunozerk ozgunozerk commented Dec 29, 2025

Fixes #532

PR Checklist

  • Tests
  • Documentation

Notes: this issue fix required us to expose a new variant of mul_div without any rounding, because that functionality is used in a lot of places in WAD, and having this function as a helper in WAD does make less sense then having it in our fixed point library.

The best place to include that, was in the trait. Then, I've realized some inconsistencies between I256 and i128 libraries.

In short, changing the trait, and fixing the inconsistencies, led me to refactoring the fixed point library altogether.

Here are the summary of the changes:

  • trait implementations were not done in place, but were delegated to other functions. This led to naming problems, and also the helper functions were not separated properly. Additionally, this approach is not inline with other parts of our library (where we have the trait, and the storage files separately). So, I moved the implementations directly into the traits for i128 and i256
  • added 2 new methods to the traits, truncate variant for both checked and unchecked mul_div
  • added a variant to Rounding enum, which is Truncate
  • added muldiv and checked_muldiv standalone functions to i256 as well, because, any difference across i128 and i256 is confusing
  • the way we implement checked and non_checked functions in i256 was not consistent with i128, changed them as well
  • removed the fixed_point.rs test, because we don't have any function/method exposed from the mod.rs file anymore
  • renamed the trait to SorobanMulDiv, (previously SorobanFixedPoint), because we are not providing a suite of fixed point arithmetic math under that trait, but only the variants of mul_div

@ozgunozerk ozgunozerk requested a review from brozorec December 29, 2025 17:28
@ozgunozerk ozgunozerk self-assigned this Dec 29, 2025
@codecov
Copy link

codecov bot commented Dec 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.11%. Comparing base (46d15b6) to head (b46742b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   96.03%   96.11%   +0.07%     
==========================================
  Files          54       54              
  Lines        5140     5245     +105     
==========================================
+ Hits         4936     5041     +105     
  Misses        204      204              

☔ 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.

@ozgunozerk
Copy link
Collaborator Author

ozgunozerk commented Dec 29, 2025

I'll write additional tests for coverage, and make it 100% for this PR

Edit: done!

@ozgunozerk ozgunozerk changed the title refactor fixed point lib [L-02] - refactor fixed point lib Dec 30, 2025
Copy link
Collaborator

@brozorec brozorec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from some naming and docstring inconsistencies, great job 🙌

@ozgunozerk ozgunozerk merged commit e23aafa into main Dec 30, 2025
5 checks passed
@ozgunozerk ozgunozerk deleted the fixed-point-refactor branch December 30, 2025 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[L-02] - Wad::checked_mul and Wad::pow Rounding Behavior Disagrees With Documentation for Negative Results

3 participants