switch to the solidity formatter#126
Conversation
WalkthroughThe Float formatting API replaces the fixed 18-decimal export with a configurable-significant-figures API: a new Rust method Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JS as JS Caller
participant WASM as WASM Export<br/>(formatWithLimit)
participant Rust as Float::format_with_limit
participant ABI as DecimalFloat::formatCall
JS->>WASM: formatWithLimit(sig_figs_limit)
WASM->>Rust: format_with_limit(sig_figs_limit)
Rust->>ABI: formatCall(sigFigsLimit = sig_figs_limit)
ABI-->>Rust: formatted String / error
Rust-->>WASM: Result<String, FloatError>
WASM-->>JS: String / error
sequenceDiagram
autonumber
actor JS as JS Caller
participant WASM as WASM Export<br/>(format)
participant Rust as Float::format
participant Rust2 as Float::format_with_limit
JS->>WASM: format()
WASM->>Rust: format()
Rust->>Rust2: format_with_limit(18)
Rust2-->>Rust: Result<String, FloatError>
Rust-->>WASM: Result<String, FloatError>
WASM-->>JS: String / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)crates/float/src/lib.rs (1)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/float/src/lib.rs (1)
359-392: Validate sig_figs_limit (reject 0; document practical upper bound)
Unvalidated limits can cause contract reverts or oversized outputs. Enforce >0 at the Rust API and (optionally) document a sane max (e.g., 68 for 224-bit coeff).Apply:
- pub fn format_with_limit(self, sig_figs_limit: u32) -> Result<String, FloatError> { + pub fn format_with_limit(self, sig_figs_limit: std::num::NonZeroU32) -> Result<String, FloatError> { let Float(a) = self; let calldata = DecimalFloat::formatCall { a, - sigFigsLimit: U256::from(sig_figs_limit), + sigFigsLimit: U256::from(sig_figs_limit.get()), } .abi_encode();If you’d rather not change the signature, at minimum guard 0 and return a typed error.
crates/float/src/js_api.rs (2)
315-339: Add JS-side input validation for sig_figs_limit
Prevent 0 (and NaN/negatives coerced into u32) early; surface a clear error to callers.Apply:
#[wasm_export(js_name = "formatWithLimit")] - pub fn format_with_limit_js(&self, sig_figs_limit: u32) -> Result<String, FloatError> { - self.format_with_limit(sig_figs_limit) - } + pub fn format_with_limit_js(&self, sig_figs_limit: u32) -> Result<String, FloatError> { + let Some(limit) = std::num::NonZeroU32::new(sig_figs_limit) else { + return Err(FloatError::JsSysError("sig_figs_limit must be > 0".into())); + }; + self.format_with_limit(limit) + }If you keep the Rust API as u32, still perform this NonZero guard here and call the existing method.
117-145: Docs/types mismatch: use bigint (12345n), not string, in JS examples and return types
Current comments say “string” but the APIs use bigint. Fix examples to avoid confusing users.Apply:
- /// * `value` - The fixed-point decimal value as a `string`. + /// * `value` - The fixed-point decimal value as a `bigint`. @@ - /// const floatResult = Float.fromFixedDecimal("12345", 2); + /// const floatResult = Float.fromFixedDecimal(12345n, 2); @@ - /// * `Ok(String)` - The resulting fixed-point decimal value as a string. + /// * `Ok(bigint)` - The resulting fixed-point decimal value as a bigint. @@ - /// assert(result.value === "12345"); + /// assert(result.value === 12345n); @@ - /// * `value` - The fixed-point decimal value as a `string`. + /// * `value` - The fixed-point decimal value as a `bigint`. @@ - /// const floatResult = Float.fromFixedDecimalLossy("12345", 2); + /// const floatResult = Float.fromFixedDecimalLossy(12345n, 2); @@ - /// * `Ok(String)` - The resulting fixed-point decimal value as a string. + /// * `Ok(bigint)` - The resulting fixed-point decimal value as a bigint. @@ - /// assert(result.value === "1234"); + /// assert(result.value === 1234n);I can sweep the rest of this file for similar doc nits if you want a follow-up PR.
Also applies to: 146-177, 178-206, 218-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/float/src/js_api.rs(2 hunks)crates/float/src/lib.rs(4 hunks)test_js/float.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:233-242
Timestamp: 2025-06-17T10:17:56.205Z
Learning: In the rainlanguage/rain.math.float repository, the maintainer 0xgleb prefers to handle documentation additions and improvements in separate issues rather than inline with feature PRs.
🧬 Code graph analysis (1)
crates/float/src/lib.rs (1)
crates/float/src/evm.rs (1)
execute_call(51-56)
⏰ 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). (1)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
🔇 Additional comments (4)
crates/float/src/lib.rs (3)
394-394: No action.
1198-1199: Updated expectations to fixed-point form make sense with the new default
These align with format() delegating to 18 sig figs.Please confirm there’s no unintended switch to scientific notation at nearby magnitudes with the new formatter (e.g., 9999.9, 10000.1). If helpful I can add targeted property tests.
Also applies to: 1203-1204
338-357: Defaulting format() to 18 sig figs via delegation looks good
Thin wrapper is clean and keeps the default centralized. No lingeringformat18orformat18_jsreferences remain.crates/float/src/js_api.rs (1)
341-341: No action.
|
@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:
|
|
Oops, something went wrong! Please try again later. 🐰 💔 |
2 participants
Motivation
Solidity formatter was implemented
Solution
Switch to the proper formatting logic instead of using the temporary workaround
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests