expose whether conversion was lossy or not #159
Conversation
WalkthroughCore float conversions now return a (value, lossless) tuple; WASM/JS bindings expose structured result types ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JS as JS Caller
participant WASM as Float (wasm_bindgen)
participant Core as Float Core
JS->>WASM: toFixedDecimalLossy(decimals)
WASM->>Core: to_fixed_decimal_lossy(self, decimals)
Core-->>WASM: (U256 value, bool lossless)
WASM-->>JS: ToFixedDecimalLossyResult { value: String, lossless }
rect rgba(0,128,128,0.08)
note right of WASM: New structured return includes lossless flag
end
sequenceDiagram
autonumber
actor JS as JS Caller
participant WASM as Float (wasm_bindgen)
participant Core as Float Core
JS->>WASM: fromFixedDecimalLossy(value: BigInt, decimals)
WASM->>Core: from_fixed_decimal_lossy(U256(value), decimals)
Core-->>WASM: (Float, bool lossless)
WASM-->>JS: FromFixedDecimalLossyResult { float: Float, lossless }
rect rgba(0,128,128,0.08)
note right of WASM: New WASM entry returns Float plus lossless flag
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-15T20:41:13.554ZApplied to files:
📚 Learning: 2025-06-18T09:10:41.740ZApplied to files:
📚 Learning: 2025-07-03T11:20:50.456ZApplied to files:
🧬 Code graph analysis (1)crates/float/src/js_api.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/float/src/js_api.rs (1)
174-176: Doc nit: parameter type is bigint, not stringUpdate doc to match the JS API.
Apply this diff:
- /// * `value` - The fixed-point decimal value as a `string`. + /// * `value` - The fixed-point decimal value as a `bigint`.crates/float/src/lib.rs (1)
2005-2032: Robustness tests: from_fixed_decimal_lossyGood coverage for lossless cases and an explicit lossy U256::MAX case. Consider adding the int256::max boundary case as explicitly lossless (matches on-chain logic).
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/float/src/js_api.rs(3 hunks)crates/float/src/lib.rs(7 hunks)test_js/float.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T14:54:24.240Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Applied to files:
test_js/float.test.tscrates/float/src/lib.rs
🧬 Code graph analysis (3)
test_js/float.test.ts (1)
crates/float/src/js_api.rs (1)
float(19-21)
crates/float/src/lib.rs (2)
crates/float/src/evm.rs (1)
execute_call(51-56)crates/float/src/js_api.rs (3)
from(37-39)float(19-21)lossless(24-26)
crates/float/src/js_api.rs (1)
crates/float/src/lib.rs (3)
from(1243-1245)from(1249-1251)from_fixed_decimal_lossy(153-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
🔇 Additional comments (8)
crates/float/src/js_api.rs (2)
10-27: Good JS-friendly result wrapper for fromFixedDecimalLossyShape and getters are appropriate for wasm_bindgen. LGTM.
241-249: toFixedDecimalLossy now returns structured result (value + lossless)Return shape matches the PR objective and tests. LGTM.
Please confirm downstream TypeScript definitions are regenerated so "ToFixedDecimalLossyResult" appears in the emitted d.ts.
test_js/float.test.ts (2)
44-49: Covers fromFixedDecimalLossy lossless pathAssertions read well and exercise the new API surface. LGTM.
50-56: Covers toFixedDecimalLossy lossy and lossless pathsGood checks for both value and lossless flag. LGTM.
Add a boundary test for fromFixedDecimalLossy at the exact threshold known to be lossless (uint256(int256::max)), to guard regression on the contract boundary check. Based on learnings.
Proposed test snippet:
it('fromFixedDecimalLossy boundary is lossless at int256::max', () => { // int256::max = 2**255 - 1 const int256Max = (1n << 255n) - 1n; const { float, lossless } = Float.fromFixedDecimalLossy(int256Max, 0); expect(lossless).toBe(true); // sanity: round-trip lossless toFixedDecimalLossy(0) should give back the same decimal string const r = float.toFixedDecimalLossy(0); expect(r.error).toBeUndefined(); expect(r.value!.value).toBe(int256Max.toString()); expect(r.value!.lossless).toBe(true); });Based on learnings
Also applies to: 58-64
crates/float/src/lib.rs (4)
153-161: Core API now returns (Float, bool) for from_fixed_decimal_lossyDecoding tuple and propagation look correct. LGTM.
188-197: Core API now returns (U256, bool) for to_fixed_decimal_lossyTuple wiring to decoded fields looks correct. LGTM.
2036-2085: Robustness tests: to_fixed_decimal_lossyCovers both lossy and lossless examples with clear expectations. LGTM.
2089-2112: Property test asserts lossless on from_fixed_decimal_lossy and conditional lossiness on to_fixed_decimal_lossyNice property-based coverage; conditions match spec. LGTM.
| #[wasm_bindgen(js_name = "fromFixedDecimalLossy")] | ||
| pub fn from_fixed_decimal_lossy_wasm( | ||
| value: BigInt, | ||
| decimals: u8, | ||
| ) -> Result<FromFixedDecimalLossyResult, JsValue> { | ||
| let value_str: String = value | ||
| .to_string(10) | ||
| .map(|s| s.into()) | ||
| .map_err(|e| JsValue::from(&e))?; | ||
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; | ||
| let (float, lossless) = Float::from_fixed_decimal_lossy(val, decimals) | ||
| .map_err(|e| JsValue::from_str(&e.to_string()))?; | ||
| Ok(FromFixedDecimalLossyResult { float, lossless }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify BigInt → String conversion and error mapping
Current map/map_err is redundant; use the same concise pattern as elsewhere.
Apply this diff:
- let value_str: String = value
- .to_string(10)
- .map(|s| s.into())
- .map_err(|e| JsValue::from(&e))?;
+ let value_str: String = value.to_string(10)?.into();📝 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.
| #[wasm_bindgen(js_name = "fromFixedDecimalLossy")] | |
| pub fn from_fixed_decimal_lossy_wasm( | |
| value: BigInt, | |
| decimals: u8, | |
| ) -> Result<FromFixedDecimalLossyResult, JsValue> { | |
| let value_str: String = value | |
| .to_string(10) | |
| .map(|s| s.into()) | |
| .map_err(|e| JsValue::from(&e))?; | |
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; | |
| let (float, lossless) = Float::from_fixed_decimal_lossy(val, decimals) | |
| .map_err(|e| JsValue::from_str(&e.to_string()))?; | |
| Ok(FromFixedDecimalLossyResult { float, lossless }) | |
| } | |
| #[wasm_bindgen(js_name = "fromFixedDecimalLossy")] | |
| pub fn from_fixed_decimal_lossy_wasm( | |
| value: BigInt, | |
| decimals: u8, | |
| ) -> Result<FromFixedDecimalLossyResult, JsValue> { | |
| let value_str: String = value.to_string(10)?.into(); | |
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; | |
| let (float, lossless) = Float::from_fixed_decimal_lossy(val, decimals) | |
| .map_err(|e| JsValue::from_str(&e.to_string()))?; | |
| Ok(FromFixedDecimalLossyResult { float, lossless }) | |
| } |
🤖 Prompt for AI Agents
In crates/float/src/js_api.rs around lines 99 to 112, streamline the
BigInt→String conversion by removing the redundant map call and mapping directly
on to_string; replace the current value.to_string(10).map(|s|
s.into()).map_err(|e| JsValue::from(&e))? with a single call that returns the
String and maps the error once (e.g. let value_str =
value.to_string(10).map_err(|e| JsValue::from(&e))?); keep the subsequent
U256::from_str and error mappings unchanged.
There was a problem hiding this comment.
@0xgleb, thank you for applying the fix! The simplified conversion is much cleaner. 🎉
/)_/)
(^.^)
/>💻
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/float/src/js_api.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#115
File: test/src/lib/LibDecimalFloat.decimal.t.sol:122-126
Timestamp: 2025-08-29T14:54:24.240Z
Learning: In test/src/lib/LibDecimalFloat.decimal.t.sol, the fromFixedDecimalLossy function converts uint256(type(int256).max) losslessly because the boundary check is `value > uint256(type(int256).max)`, not `>=`. Values exactly equal to type(int256).max still take the lossless conversion path.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#111
File: test/src/lib/LibDecimalFloat.div.t.sol:48-51
Timestamp: 2025-08-26T16:50:11.113Z
Learning: In LibDecimalFloat, packLossless internally asserts that the packing operation is lossless, so there's no need to manually check the lossless flag when using packLossless instead of packLossy.
🧬 Code graph analysis (1)
crates/float/src/js_api.rs (1)
crates/float/src/lib.rs (3)
from(1243-1245)from(1249-1251)from_fixed_decimal_lossy(153-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (macos-latest, rainix-sol-legal)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
🔇 Additional comments (2)
crates/float/src/js_api.rs (2)
32-37: LGTM!The struct is well-defined with appropriate trait derives and macro usage for WASM/JS interop.
235-244: LGTM!The function correctly unpacks the tuple result from
to_fixed_decimal_lossy, converts theU256value to a string, and wraps both the value and lossless flag in the result struct. The implementation aligns with the PR objective to expose lossiness information.
| #[wasm_bindgen] | ||
| pub struct FromFixedDecimalLossyResult { | ||
| float: Float, | ||
| lossless: bool, | ||
| } | ||
|
|
||
| #[wasm_bindgen] | ||
| impl FromFixedDecimalLossyResult { | ||
| #[wasm_bindgen(getter)] | ||
| pub fn float(&self) -> Float { | ||
| self.float | ||
| } | ||
|
|
||
| #[wasm_bindgen(getter)] | ||
| pub fn lossless(&self) -> bool { | ||
| self.lossless | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding trait derives for consistency and ergonomics.
ToFixedDecimalLossyResult (lines 32-37) has derives for Serialize, Deserialize, Debug, Clone, PartialEq, and Tsify, plus impl_wasm_traits!. FromFixedDecimalLossyResult lacks these traits, which may limit debugging, testing, and serialization capabilities.
If the Float type supports these traits, apply this diff to add them:
+#[derive(Debug, Clone, PartialEq)]
#[wasm_bindgen]
pub struct FromFixedDecimalLossyResult {
float: Float,
lossless: bool,
}If serialization is needed for JS interop consistency, consider also adding Serialize, Deserialize, and Tsify (assuming Float supports them).
📝 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.
| #[wasm_bindgen] | |
| pub struct FromFixedDecimalLossyResult { | |
| float: Float, | |
| lossless: bool, | |
| } | |
| #[wasm_bindgen] | |
| impl FromFixedDecimalLossyResult { | |
| #[wasm_bindgen(getter)] | |
| pub fn float(&self) -> Float { | |
| self.float | |
| } | |
| #[wasm_bindgen(getter)] | |
| pub fn lossless(&self) -> bool { | |
| self.lossless | |
| } | |
| } | |
| #[derive(Debug, Clone, PartialEq)] | |
| #[wasm_bindgen] | |
| pub struct FromFixedDecimalLossyResult { | |
| float: Float, | |
| lossless: bool, | |
| } | |
| #[wasm_bindgen] | |
| impl FromFixedDecimalLossyResult { | |
| #[wasm_bindgen(getter)] | |
| pub fn float(&self) -> Float { | |
| self.float | |
| } | |
| #[wasm_bindgen(getter)] | |
| pub fn lossless(&self) -> bool { | |
| self.lossless | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/float/src/js_api.rs around lines 13 to 30, FromFixedDecimalLossyResult
is missing the same trait derives and wasm helper impls as
ToFixedDecimalLossyResult; add derives (e.g., Serialize, Deserialize, Debug,
Clone, PartialEq, Tsify) and call impl_wasm_traits! for
FromFixedDecimalLossyResult (only if the contained Float type supports those
traits), ensuring the struct keeps #[wasm_bindgen] for JS export and that any
feature flags/imports needed for Serialize/Deserialize/Tsify are enabled.
There was a problem hiding this comment.
not needed, only gonna be used from ts/js
There was a problem hiding this comment.
@0xgleb, understood! Since FromFixedDecimalLossyResult is only consumed by TypeScript/JavaScript and not used for Rust-side operations, the additional trait derives aren't necessary.
✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#159
File: crates/float/src/js_api.rs:13-30
Timestamp: 2025-10-15T20:41:13.554Z
Learning: In the rain.math.float codebase, wasm_bindgen structs that are only returned to TypeScript/JavaScript (like `FromFixedDecimalLossyResult`) don't need Rust trait derives (Debug, Clone, PartialEq, Serialize, Deserialize, Tsify) beyond what's required for JS interop. Only add these derives when the struct is used for Rust-side operations like serialization, debugging, or testing.
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.
🧠 Learnings used
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:316-328
Timestamp: 2025-06-18T09:10:41.740Z
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.
| let value_str: String = value.to_string(10).map_err(|e| JsValue::from(&e))?.into(); | ||
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Standardize error conversion patterns.
Lines 101 and 102 use different error conversion approaches:
- Line 101:
JsValue::from(&e) - Line 102:
JsValue::from_str(&e.to_string())
For consistency with similar functions (e.g., from_fixed_decimal_js at line 189), use a uniform pattern:
- let value_str: String = value.to_string(10).map_err(|e| JsValue::from(&e))?.into();
+ let value_str: String = value.to_string(10)?.into();
let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?;📝 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.
| let value_str: String = value.to_string(10).map_err(|e| JsValue::from(&e))?.into(); | |
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; | |
| let value_str: String = value.to_string(10)?.into(); | |
| let val = U256::from_str(&value_str).map_err(|e| JsValue::from_str(&e.to_string()))?; |
🤖 Prompt for AI Agents
In crates/float/src/js_api.rs around lines 101 to 102, the error conversions are
inconsistent: change the map_err on line 101 to use
JsValue::from_str(&e.to_string()) so both lines use the same pattern
(map_err(|e| JsValue::from_str(&e.to_string()))?), matching the style used in
from_fixed_decimal_js at line 189; ensure both error mappings return a
JsValue::from_str of the error's to_string().
There was a problem hiding this comment.
@0xgleb, understood. I'll leave the error conversion as is.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
fixes #158
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Documentation
Tests