Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 83 additions & 1 deletion crates/float/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use revm::interpreter::interpreter::EthInterpreter;
use revm::primitives::{address, fixed_bytes};
use revm::{Context, MainBuilder, MainContext, SystemCallEvm};
use std::cell::RefCell;
use std::ops::{Add, Div, Mul, Sub};
use std::ops::{Add, Div, Mul, Neg, Sub};
use std::thread::AccessError;
use thiserror::Error;

Expand Down Expand Up @@ -232,6 +232,16 @@ impl Float {
})
}

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))
})
}
Comment on lines +235 to +243
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! 👍


pub fn abs(self) -> Result<Float, FloatError> {
let Float(a) = self;
let calldata = DecimalFloat::absCall { a }.abi_encode();
Expand Down Expand Up @@ -303,6 +313,20 @@ impl Div for Float {
}
}

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))
})
}
}
Comment on lines +316 to +328
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.


#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -531,6 +555,64 @@ mod tests {
}
}

#[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");
}
Comment on lines +558 to +574
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.


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());
}
}
Comment on lines +576 to +583
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!

🐰
♪ ♫ ♪
◕ ◕


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(),
);
}
}
Comment on lines +585 to +614
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.


proptest! {
#[test]
fn test_abs_no_minus_sign(float in reasonable_float()) {
Expand Down