Skip to content

Conversation

@ozgunozerk
Copy link
Collaborator

Fixes #499

PR Checklist

  • Tests
  • Documentation

Also restructures the math library

@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 96.26168% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.06%. Comparing base (e812f7a) to head (d9c2ad9).

Files with missing lines Patch % Lines
...ckages/contract-utils/src/math/i256_fixed_point.rs 90.00% 3 Missing ⚠️
packages/contract-utils/src/math/mod.rs 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
+ Coverage   95.02%   95.06%   +0.03%     
==========================================
  Files          77       77              
  Lines        5309     5408      +99     
==========================================
+ Hits         5045     5141      +96     
- Misses        264      267       +3     

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

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.

great job with the re-org 🙌 just couple of nit picks and improving the coverage for checked_mul_div_floor

/// `WAD_SCALE`.
pub fn checked_mul(self, e: &Env, rhs: Wad) -> Option<Wad> {
// Use fixed_mul_floor to handle phantom overflow transparently
let result = self.0.fixed_mul_floor(e, &rhs.0, &WAD_SCALE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use checked_fixed_mul_floor so that it returns None instead of panicking on overflow?

}
}

/// Performs floor(x * y / z)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Performs ceil(x * y / z)

let zero = I256::from_i32(env, 0);
let r = x.mul(y);

if z.clone() == zero {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of cloning, wouldn't it be better to dereference z here and in the next fn?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

Power Function

3 participants