Skip to content

Expose minus and inv methods#57

Merged
hardyjosh merged 11 commits intomainfrom
feat/minus-and-inv
Jun 19, 2025
Merged

Expose minus and inv methods#57
hardyjosh merged 11 commits intomainfrom
feat/minus-and-inv

Conversation

@0xgleb
Copy link
Copy Markdown
Contributor

@0xgleb 0xgleb commented Jun 11, 2025

Motivation

We need to use Solidity Float code in Rust

Solution

Expose minus (via neg) and inv methods

Checks

By submitting this for review, I'm confirming I've done the following:

  • made this PR as small as possible
  • unit-tested any new functionality
  • linked any relevant issues or PRs
  • included screenshots (if this involves a front-end change)

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added support for negating floats and computing their multiplicative inverse.
  • Tests

    • Introduced tests to verify correct behavior of float negation, double negation, and inversion, including property-based checks for accuracy and formatting.

@0xgleb 0xgleb self-assigned this Jun 11, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 11, 2025

"""

Walkthrough

Two new functionalities were added to the Float struct: an inv method for multiplicative inversion and a Neg trait implementation for negation, both invoking Solidity contract calls and decoding results. Tests verify double negation correctness and that a float multiplied by its inverse approximates one.

Changes

File(s) Change Summary
crates/float/src/lib.rs Added inv method and Neg trait implementation to Float; included tests for negation and inversion.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Float
    participant EVM

    User->>Float: call neg()
    Float->>EVM: execute minusCall calldata
    EVM-->>Float: return negated value
    Float-->>User: return new Float instance

    User->>Float: call inv()
    Float->>EVM: execute invCall calldata
    EVM-->>Float: return inverse value
    Float-->>User: return new Float instance
Loading

Possibly related PRs

  • Expose mul & div #64: Adds Mul and Div trait implementations to Float, complementing this PR’s arithmetic operator enhancements.
  • Expose abs #59: Introduces an abs method to Float using Solidity contract calls, similar to the pattern used here.

Suggested reviewers

  • findolor
  • hardyjosh
    """
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@0xgleb 0xgleb linked an issue Jun 11, 2025 that may be closed by this pull request
@0xgleb 0xgleb force-pushed the feat/minus-and-inv branch from 91273d7 to 3241f59 Compare June 11, 2025 11:23
@0xgleb 0xgleb force-pushed the feat/minus-and-inv branch from 3241f59 to a5ca017 Compare June 16, 2025 13:21
@0xgleb 0xgleb force-pushed the feat/minus-and-inv branch from 4a7a5a3 to 33606f7 Compare June 16, 2025 15:44
@0xgleb 0xgleb mentioned this pull request Jun 16, 2025
4 tasks
@0xgleb 0xgleb marked this pull request as ready for review June 18, 2025 08:57
@0xgleb 0xgleb requested review from findolor and hardyjosh June 18, 2025 08:59
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af8cd4b and 70f5af7.

📒 Files selected for processing (1)
  • crates/float/src/lib.rs (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: git-clean
🔇 Additional comments (1)
crates/float/src/lib.rs (1)

554-570: Unit test looks good

test_minus_format thoroughly exercises parsing, negation twice, and the zero edge-case. No issues spotted.

Comment thread crates/float/src/lib.rs
Comment on lines +245 to +253
pub fn inv(self) -> Result<Self, FloatError> {
let Float(a) = self;
let calldata = DecimalFloat::invCall { a }.abi_encode();

execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::invCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 18, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Document/guard the zero case for inv

inv will revert when the value is zero, bubbling up as Err(FloatError::Revert(_)).
Two quick wins:

  1. Add a short doc-comment stating that inv fails for zero.
  2. (Optional) Early-return a custom error before the EVM call to avoid the round-trip cost.

No functional change required, but the extra clarity will prevent surprises for downstream users.

🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 245 to 253, add a short doc-comment to
the inv function stating that it will fail with an error if the value is zero.
Additionally, add a check at the start of the function to detect if self is zero
and return a custom FloatError early without making the EVM call, to avoid
unnecessary execution cost. This improves clarity and efficiency without
changing the function's behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've created a separate issue to document everything in the crate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@0xgleb That's a great approach! Creating a comprehensive documentation issue for the entire crate is much better than piecemeal fixes. It ensures consistency and completeness across all functions, not just the inv method. Thanks for taking the systematic approach to improve the crate's documentation! 👍

Comment thread crates/float/src/lib.rs Outdated
Comment on lines +235 to +243
pub fn minus(self) -> Result<Self, FloatError> {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();

execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Implement std::ops::Neg for idiomatic unary negation

minus works, but idiomatic Rust users will expect the unary - operator to be available. You can expose it trivially by implementing Neg and delegating to minus, keeping the method for explicit calls if you wish.

+use std::ops::Neg;
+
 impl Float {
@@
 }
 
+impl std::ops::Neg for Float {
+    type Output = Result<Self, FloatError>;
+
+    #[inline]
+    fn neg(self) -> Self::Output {
+        self.minus()
+    }
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub fn minus(self) -> Result<Self, FloatError> {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();
execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
use std::ops::Neg;
impl Float {
pub fn minus(self) -> Result<Self, FloatError> {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();
execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
}
impl std::ops::Neg for Float {
type Output = Result<Self, FloatError>;
#[inline]
fn neg(self) -> Self::Output {
self.minus()
}
}
🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 235 to 243, the minus method provides
unary negation but does not implement the standard Rust unary `-` operator. To
fix this, implement the std::ops::Neg trait for the Float type by defining the
neg function that calls the existing minus method and returns its result. This
will allow users to use the idiomatic `-` operator for negation while keeping
the minus method for explicit calls.

Comment thread crates/float/src/lib.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
crates/float/src/lib.rs (2)

235-243: Document & short-circuit zero handling in inv

inv will revert when the value is zero. The missing doc-comment and guard make the behaviour easy to miss and create needless EVM round-trips.

+    /// Returns the multiplicative inverse of this `Float`.
+    ///
+    /// ⚠️ Calling this on a value equal to `0` will fail with
+    /// `Err(FloatError::Revert(_))`.  Consider checking for zero
+    /// beforehand or rely on the early-return below to avoid the
+    /// contract call.
     pub fn inv(self) -> Result<Self, FloatError> {
         let Float(a) = self;
+        // Avoid the EVM call when the value is known to be zero.
+        if a == fixed_bytes!("0000000000000000000000000000000000000000000000000000000000000000") {
+            return Err(FloatError::Revert(Bytes::new()));
+        }
         let calldata = DecimalFloat::invCall { a }.abi_encode();

585-603: test_inv_prod still panics on zero – add an assumption

reasonable_float() can yield zero; inv() reverts in that case, so the unwrap() will panic and fail the property early.

 fn test_inv_prod(float in reasonable_float()) {
-    let inv = float.inv().unwrap();
+    let zero = Float::parse("0".to_string()).unwrap();
+    // `inv` is undefined for zero; skip such cases.
+    prop_assume!(!float.eq(zero).unwrap());
+
+    let inv = float.inv().unwrap();
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70f5af7 and 335b200.

📒 Files selected for processing (1)
  • crates/float/src/lib.rs (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: git-clean
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)

Comment thread crates/float/src/lib.rs
Comment on lines +316 to +328
impl Neg for Float {
type Output = Result<Self, FloatError>;

fn neg(self) -> Self::Output {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();

execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 18, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Public minus method still not exposed – PR objective only half met

The PR description promises to expose both minus and inv.
Only the Neg trait is implemented; there is no public minus function, so users cannot call foo.minus()? directly (required when a value is behind a reference or when method syntax is preferred to the - operator).

 impl Float {
+    /// Explicit unary minus wrapper (alias for `-self`).
+    pub fn minus(self) -> Result<Self, FloatError> {
+        self.neg()
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
impl Neg for Float {
type Output = Result<Self, FloatError>;
fn neg(self) -> Self::Output {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();
execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
}
impl Neg for Float {
type Output = Result<Self, FloatError>;
fn neg(self) -> Self::Output {
let Float(a) = self;
let calldata = DecimalFloat::minusCall { a }.abi_encode();
execute_call(Bytes::from(calldata), |output| {
let decoded = DecimalFloat::minusCall::abi_decode_returns(output.as_ref())?;
Ok(Float(decoded))
})
}
}
impl Float {
/// Explicit unary minus wrapper (alias for `-self`).
pub fn minus(self) -> Result<Self, FloatError> {
self.neg()
}
}
🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 316 to 328, the Neg trait is implemented
for Float but there is no public minus method exposed as promised. Add a public
method named minus to the Float impl block that internally calls the same logic
as the Neg trait implementation, returning Result<Self, FloatError>. This will
allow users to call foo.minus()? directly, supporting method syntax and usage on
references.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR description. Float implements Copy, so references are not an issue. .neg() can still be used when method syntax is preferred

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@0xgleb You're absolutely right! Your implementation approach is actually more idiomatic and well-designed:

  1. Neg trait implementation provides the minus functionality through the - operator, which is the standard Rust way
  2. Copy trait indeed eliminates any reference concerns since Float values can be copied cheaply
  3. Method syntax is available via .neg() when preferred over the operator

Your design properly exposes both promised functionalities:

  • minus: via Neg trait (-float or float.neg())
  • inv: via public method (float.inv())

This is cleaner than having redundant methods when traits already provide the functionality. Thanks for the clarification!

🐰✨ learns something new


✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:316-328
Timestamp: 2025-06-18T09:10:41.723Z
Learning: In the rain.math.float codebase, the user prefers implementing standard Rust traits (like Neg) rather than creating redundant public methods when the trait already provides the needed functionality. Float implements Copy, so reference usage with operators is not a concern.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (1)
crates/float/src/lib.rs (1)

235-243: Document / short-circuit the 0 case

inv delegates straight to the Solidity revert, so float.inv() panics for zero and still pays an EVM round-trip.
Add a quick guard (or at least a doc-comment) explaining the behaviour and returning a dedicated FloatError::DecimalFloat(DecimalFloatErrors::Log10Zero(_)) early.

+    /// Returns the multiplicative inverse.  
+    ///  
+    /// # Errors  
+    /// * Fails for `0` with `FloatError::DecimalFloat(DecimalFloatErrors::Log10Zero)`  
+    ///   (guarded locally to avoid an unnecessary EVM call).  
     pub fn inv(self) -> Result<Self, FloatError> {
         let Float(a) = self;
+
+        if a == fixed_bytes!("0000000000000000000000000000000000000000000000000000000000000000") {
+            return Err(FloatError::DecimalFloat(DecimalFloatErrors::Log10Zero(_)));
+        }
+
         let calldata = DecimalFloat::invCall { a }.abi_encode();
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 335b200 and 1c2e810.

📒 Files selected for processing (1)
  • crates/float/src/lib.rs (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:316-328
Timestamp: 2025-06-18T09:10:41.723Z
Learning: In the rain.math.float codebase, the user prefers implementing standard Rust traits (like Neg) rather than creating redundant public methods when the trait already provides the needed functionality. Float implements Copy, so reference usage with operators is not a concern.
crates/float/src/lib.rs (1)
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:316-328
Timestamp: 2025-06-18T09:10:41.723Z
Learning: In the rain.math.float codebase, the user prefers implementing standard Rust traits (like Neg) rather than creating redundant public methods when the trait already provides the needed functionality. Float implements Copy, so reference usage with operators is not a concern.
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: git-clean
🔇 Additional comments (1)
crates/float/src/lib.rs (1)

316-328: 👍 Idiomatic Neg implementation looks good

Implementing std::ops::Neg instead of a bespoke minus method keeps the API idiomatic and leverages operator syntax while still allowing .neg() method calls. No issues spotted.

Comment thread crates/float/src/lib.rs
Comment on lines +558 to +574
#[test]
fn test_minus_format() {
let float = Float::parse("-123.1234234625468391".to_string()).unwrap();
let negated = float.neg().unwrap();
let formatted = negated.format().unwrap();
assert_eq!(formatted, "123.1234234625468391");

let float = Float::parse(formatted).unwrap();
let negated = float.neg().unwrap();
let formatted = negated.format().unwrap();
assert_eq!(formatted, "-123.1234234625468391");

let float = Float::parse("0".to_string()).unwrap();
let negated = float.neg().unwrap();
let formatted = negated.format().unwrap();
assert_eq!(formatted, "0");
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 18, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider exercising the unary - operator in the new test

test_minus_format only calls .neg(). Adding a quick let formatted = (-float).unwrap().format()?; assertion would verify the operator path too and guard against accidental regression in the Neg impl.

🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 558 to 574, the test_minus_format
function only tests the .neg() method but does not test the unary - operator.
Add an assertion that uses the unary - operator on the Float instance, unwraps
the result, formats it, and compares it to the expected string. This will ensure
the Neg trait implementation is also tested and prevent regressions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is a Rust built-in, we don't need to test it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@0xgleb You're absolutely right! The unary - operator simply calls the neg() method when the Neg trait is implemented. Since there's no separate code path for the operator, testing .neg() directly is sufficient. Thanks for the clarification!

🐰
╭─────╮
│ ∩ ∩ │ learns something new
│ ︶ │
╰─────╯


✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:558-574
Timestamp: 2025-06-18T09:20:39.152Z
Learning: In Rust, when the `Neg` trait is implemented for a custom type, the unary `-` operator simply delegates to the `neg()` method. Testing the `neg()` method directly is sufficient since there's no separate code path for the operator.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread crates/float/src/lib.rs
Comment on lines +576 to +583
proptest! {
#[test]
fn test_minus_minus(float in arb_float()) {
let negated = float.neg().unwrap();
let renegated = negated.neg().unwrap();
prop_assert!(float.eq(renegated).unwrap());
}
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 18, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Property test passes but could use tighter equality

minus_minus checks structural equality via eq().
Because floating-point formatting is already available, adding a prop_assert_eq!(float.format()?, renegated.format()?); would fail with clearer diagnostics if edge cases slip through.

🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 576 to 583, the test_minus_minus
property test currently uses eq() for equality, which may not provide clear
diagnostics on failure. To improve this, add a prop_assert_eq! comparing the
formatted strings of float and renegated using float.format()? and
renegated.format()? respectively. This will provide tighter equality checks and
clearer failure messages for edge cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

.eq is fine. if diagnostics are required, additional arguments can be passed into the macro

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@0xgleb You're absolutely right! The prop_assert! macro does accept additional arguments for custom diagnostics when needed. Your current approach with .eq() is perfectly valid and maintains the semantic meaning of the test. Thanks for the clarification!

🐰
♪ ♫ ♪
◕ ◕

Comment thread crates/float/src/lib.rs
Comment on lines +585 to +614
proptest! {
#[test]
fn test_inv_prod(float in reasonable_float()) {
let zero = Float::parse("0".to_string()).unwrap();
prop_assume!(!float.eq(zero).unwrap());

let inv = float.inv().unwrap();
let product = (float * inv).unwrap();
let one = Float::parse("1".to_string()).unwrap();

// Allow for minor rounding errors introduced by the lossy
// `inv` implementation. We consider the property to
// hold if the product is within `±1e-37` of 1.

let eps = Float::parse("1e-37".to_string()).unwrap();
let one_plus_eps = (one + eps).unwrap();
let one_minus_eps = (one - eps).unwrap();

let within_upper = !product.gt(one_plus_eps).unwrap();
let within_lower = !product.lt(one_minus_eps).unwrap();

prop_assert!(
within_upper && within_lower,
"float: {}, inv: {}, product: {} (not within ±ε)",
float.show_unpacked().unwrap(),
inv.show_unpacked().unwrap(),
product.show_unpacked().unwrap(),
);
}
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 18, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

test_inv_prod can panic on extreme inputs

float.inv().unwrap() (and the subsequent multiplication) will panic if inv or * overflows or reverts (e.g., very small | very large numbers).
Skip such cases to keep the property test stable.

-            let inv = float.inv().unwrap();
-            let product = (float * inv).unwrap();
+            prop_assume!(float.inv().is_ok());
+            let inv = float.inv().unwrap();
+
+            prop_assume!((float * inv).is_ok());
+            let product = (float * inv).unwrap();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
proptest! {
#[test]
fn test_inv_prod(float in reasonable_float()) {
let zero = Float::parse("0".to_string()).unwrap();
prop_assume!(!float.eq(zero).unwrap());
let inv = float.inv().unwrap();
let product = (float * inv).unwrap();
let one = Float::parse("1".to_string()).unwrap();
// Allow for minor rounding errors introduced by the lossy
// `inv` implementation. We consider the property to
// hold if the product is within `±1e-37` of 1.
let eps = Float::parse("1e-37".to_string()).unwrap();
let one_plus_eps = (one + eps).unwrap();
let one_minus_eps = (one - eps).unwrap();
let within_upper = !product.gt(one_plus_eps).unwrap();
let within_lower = !product.lt(one_minus_eps).unwrap();
prop_assert!(
within_upper && within_lower,
"float: {}, inv: {}, product: {} (not within ±ε)",
float.show_unpacked().unwrap(),
inv.show_unpacked().unwrap(),
product.show_unpacked().unwrap(),
);
}
}
proptest! {
#[test]
fn test_inv_prod(float in reasonable_float()) {
let zero = Float::parse("0".to_string()).unwrap();
prop_assume!(!float.eq(zero).unwrap());
+ // Skip cases where inv() would Err (e.g. overflow/revert on extreme values)
+ prop_assume!(float.inv().is_ok());
let inv = float.inv().unwrap();
+
+ // Skip cases where multiplication would Err (e.g. result overflow)
+ prop_assume!((float * inv).is_ok());
let product = (float * inv).unwrap();
let one = Float::parse("1".to_string()).unwrap();
// Allow for minor rounding errors introduced by the lossy
// `inv` implementation. We consider the property to
// hold if the product is within `±1e-37` of 1.
let eps = Float::parse("1e-37".to_string()).unwrap();
let one_plus_eps = (one + eps).unwrap();
let one_minus_eps = (one - eps).unwrap();
let within_upper = !product.gt(one_plus_eps).unwrap();
let within_lower = !product.lt(one_minus_eps).unwrap();
prop_assert!(
within_upper && within_lower,
"float: {}, inv: {}, product: {} (not within ±ε)",
float.show_unpacked().unwrap(),
inv.show_unpacked().unwrap(),
product.show_unpacked().unwrap(),
);
}
}
🤖 Prompt for AI Agents
In crates/float/src/lib.rs around lines 585 to 614, the test_inv_prod function
calls float.inv().unwrap() and performs multiplication that can panic on extreme
inputs due to overflow or revert. To fix this, add prop_assume! checks to skip
cases where inv() or multiplication would fail, ensuring the property test only
runs on stable inputs and avoids panics.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that's why reasonable_float is used for value generation instead of arb_float

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@0xgleb You're absolutely right! I see that reasonable_float() constrains the generated values to a much more reasonable range (-10^18 to 10^18) compared to arb_float() which uses arbitrary coefficients and exponents. This design choice effectively prevents the extreme cases that could cause inv() or multiplication to overflow/revert, making my suggested prop_assume! checks unnecessary.

Good call on using the appropriate generator to avoid edge cases while still getting good property test coverage.

@0xgleb 0xgleb added the enhancement New feature or request label Jun 18, 2025
@hardyjosh hardyjosh merged commit d3cfae3 into main Jun 19, 2025
8 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Jul 22, 2025
4 tasks
This was referenced Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose minus and inv methods

3 participants