Rounding improvements for log2 and log256

[immrsd]

Summary by CodeRabbit

• Refactor
  • Streamlined internal logarithm rounding calculations by consolidating rounding logic into unified functions for improved maintainability.

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

[coderabbitai]

Walkthrough

Refactored logarithm rounding logic in macros.move by replacing boolean decision functions (log2_should_round_up, log256_should_round_up) with new direct-result helpers (round_log2_to_nearest, round_log256_to_nearest) that return the rounded logarithm value directly, consolidating rounding behavior into single functions.

Changes

Cohort / File(s)	Summary
Logarithm rounding refactor math/core/sources/internal/macros.move	Replaced boolean rounding decision functions with direct-result helpers. Removed log2_should_round_up and log256_should_round_up; added round_log2_to_nearest (returns u16) and round_log256_to_nearest (returns u8). Updated log2 and log256 macro implementations to route rounding through the new helpers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify that round_log2_to_nearest and round_log256_to_nearest maintain equivalent rounding logic as the replaced boolean helpers
• Confirm macro implementations correctly apply the returned rounded values in conditional logic (floor_log vs. floor_log + 1)
• Ensure type consistency between new functions and their callers (u16 and u8 return types)

Possibly related PRs

• #48: Directly related—both PRs modify the same macros.move rounding logic and replace log256_should_round_up with round_log256_to_nearest
• #46: Related—both PRs modify log2 rounding decision helpers, with this PR consolidating the logic into round_log2_to_nearest

Suggested reviewers

• bidzyyys
• ericnordelo

Poem

🐰 A rabbit hops through rounding's song,
Where booleans danced far too long,
Now nearest friends in u16 delight,
Direct results! The code feels right! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is entirely empty. The template requires sections for issue reference, change description, context, and a PR checklist, none of which are present.	Add a comprehensive description including the issue number, detailed explanation of the rounding improvements, context for the changes, and complete the PR checklist items (Tests, Documentation, Changelog).

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Rounding improvements for log2 and log256' clearly and concisely summarizes the main changes, which involve refactoring rounding logic in these two mathematical functions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/code-improvements

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e61daa9 and 295b607.

📒 Files selected for processing (1)

• math/core/sources/internal/macros.move (8 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u8.move:50-57
Timestamp: 2025-11-04T14:38:12.309Z
Learning: In math/core/sources/{u8,u16,u32,u64,u128}.move files, the mul_shr implementations correctly ignore the overflow flag from macros::mul_shr! and rely on try_as_*() to detect when the result doesn't fit in the target type. Only u256.move needs to explicitly check the overflow flag because u256 operations can exceed u256 capacity.
<!-- [/add_learning]

📚 Learning: 2025-11-04T14:38:12.309Z

Learnt from: ericnordelo
Repo: OpenZeppelin/contracts-sui PR: 40
File: math/core/sources/u8.move:50-57
Timestamp: 2025-11-04T14:38:12.309Z
Learning: In math/core/sources/{u8,u16,u32,u64,u128}.move files, the mul_shr implementations correctly ignore the overflow flag from macros::mul_shr! and rely on try_as_*() to detect when the result doesn't fit in the target type. Only u256.move needs to explicitly check the overflow flag because u256 operations can exceed u256 capacity.
<!-- [/add_learning]

Applied to files:

• math/core/sources/internal/macros.move

🔇 Additional comments (4)

math/core/sources/internal/macros.move (4)

277-298: Excellent refactoring of log2 rounding logic.

The consolidation of rounding decision and result computation into round_log2_to_nearest is cleaner than the previous approach. The function now returns the rounded result directly rather than a boolean flag, reducing intermediate variables and improving code clarity.

---

316-338: Improved type safety with appropriate use of u8 for log256.

The refactoring correctly uses u8 for floor_log256 instead of the previous u16, which is more appropriate since log₂₅₆ of u256::MAX is at most 31. The type narrowing on line 325 (removing the cast to u16) and the subsequent computation on line 326 are both correct and safe.

---

701-722: Well-implemented nearest-rounding logic for log2.

The function correctly implements the midpoint test by comparing value² with 2^(2·floor_log + 1). The fast/slow path selection on line 704 properly guards against overflow:

• Fast path: Used when threshold_exp < 256 and value² ≤ u256::MAX
• Slow path: Uses u512 arithmetic for larger values

The tie-breaking behavior (using ≥ on lines 709 and 719) correctly rounds half up.

---

744-750: Clean and efficient implementation for log256 nearest rounding.

The function correctly implements the midpoint test by comparing value with 2^(8·floor_log + 4). The implementation is simpler than round_log2_to_nearest because:

• For u256 values, floor_log ∈ [0, 31]
• Maximum threshold_exp = 8·31 + 4 = 252, which safely fits in a single u256 shift

The bounds are verified by the comment on lines 745-746, and the logic correctly rounds half up using the ≥ comparison on line 749.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.26%. Comparing base (e61daa9) to head (295b607).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
- Coverage   95.35%   95.26%   -0.09%     
==========================================
  Files          13       13              
  Lines        1227     1204      -23     
  Branches      350      340      -10     
==========================================
- Hits         1170     1147      -23     
  Misses         40       40              
  Partials       17       17              

Flag	Coverage Δ
contracts/access	53.50% <ø> (ø)
math/core	94.73% <100.00%> (-0.11%)	⬇️

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.