Conversation
Contributor
WalkthroughThis change refactors the Rust float library by modularizing error handling and EVM execution logic into separate modules. It introduces comprehensive documentation and examples for all public methods of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Float
participant EVM (execute_call)
participant DecimalFloatContract
User->>Float: invoke method (e.g., add, from_fixed_decimal)
Float->>EVM (execute_call): encode calldata, call execute_call
EVM (execute_call)->>DecimalFloatContract: execute contract call with calldata
DecimalFloatContract-->>EVM (execute_call): return output or revert
EVM (execute_call)-->>Float: decode output or error
Float-->>User: return Result or FloatError
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/al/lo/alloy, error: Permission denied (os error 13) Caused by: ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/float/Cargo.toml(1 hunks)crates/float/src/error.rs(1 hunks)crates/float/src/evm.rs(1 hunks)crates/float/src/lib.rs(20 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:447-461
Timestamp: 2025-06-17T10:11:32.740Z
Learning: In the rainlanguage/rain.math.float repository, the user 0xgleb prefers using .unwrap() over ? in tests because it provides clearer stack traces showing the exact line where panics occurred, making debugging easier.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#30
File: test/src/lib/LibDecimalFloat.gt.t.sol:33-36
Timestamp: 2025-04-25T03:58:01.307Z
Learning: In the rain.math.float library, all values of `Float` (which is a type alias for bytes32) are considered valid and can be safely used with methods like gt(), lt(), or eq() without causing reverts.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#46
File: crates/float/proptest-regressions/lib.txt:1-3
Timestamp: 2025-06-11T09:03:20.771Z
Learning: The file `crates/float/proptest-regressions/lib.txt` is automatically generated by proptest and should not be manually edited.
crates/float/Cargo.toml (2)
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#46
File: crates/float/proptest-regressions/lib.txt:1-3
Timestamp: 2025-06-11T09:03:20.771Z
Learning: The file `crates/float/proptest-regressions/lib.txt` is automatically generated by proptest and should not be manually edited.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#46
File: .github/workflows/rainix.yaml:50-50
Timestamp: 2025-06-11T10:19:16.675Z
Learning: In the rain.math.float CI workflow, the `nix develop -c rainix-sol-prelude` step is required for Rust jobs as well, because Rust code uses JSON ABI artifacts generated during the Solidity prelude to drive `alloy::sol!` type generation.
crates/float/src/error.rs (8)
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#46
File: crates/float/src/lib.rs:112-153
Timestamp: 2025-06-11T09:07:00.591Z
Learning: Alloy generates helpers to decode full ABI-encoded error payloads, but it doesn’t generate utilities to map raw 4-byte error selectors to error types; manual mapping is required when a contract returns only the selector.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#30
File: test/src/lib/LibDecimalFloat.gt.t.sol:33-36
Timestamp: 2025-04-25T03:58:01.307Z
Learning: In the rain.math.float library, all values of `Float` (which is a type alias for bytes32) are considered valid and can be safely used with methods like gt(), lt(), or eq() without causing reverts.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#68
File: crates/float/src/lib.rs:134-135
Timestamp: 2025-06-21T11:07:19.190Z
Learning: In Alloy primitives library, B256 is a type alias for FixedBytes<32> (pub type B256 = FixedBytes<32>), making them completely interchangeable in type usage. Changing between these types is not a breaking change.
crates/float/src/lib.rs (10)
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#64
File: src/concrete/DecimalFloat.sol:0-0
Timestamp: 2025-06-17T10:02:01.394Z
Learning: In the rain.math.float project, the Float type is designed with a specific bit layout: 224-bit signed coefficient and 32-bit signed exponent, represented as 32 bytes total. The LibDecimalFloat.unpack function returns values that are guaranteed to fit within int224 and int32 ranges due to this internal representation, making explicit range checks before casting unnecessary.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#30
File: test/src/lib/LibDecimalFloat.gt.t.sol:33-36
Timestamp: 2025-04-25T03:58:01.307Z
Learning: In the rain.math.float library, all values of `Float` (which is a type alias for bytes32) are considered valid and can be safely used with methods like gt(), lt(), or eq() without causing reverts.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:558-574
Timestamp: 2025-06-18T09:20:39.225Z
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#46
File: crates/float/proptest-regressions/lib.txt:1-3
Timestamp: 2025-06-11T09:03:20.771Z
Learning: The file `crates/float/proptest-regressions/lib.txt` is automatically generated by proptest and should not be manually edited.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:447-461
Timestamp: 2025-06-17T10:11:32.740Z
Learning: In the rainlanguage/rain.math.float repository, the user 0xgleb prefers using .unwrap() over ? in tests because it provides clearer stack traces showing the exact line where panics occurred, making debugging easier.
crates/float/src/evm.rs (4)
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Learnt from: thedavidmeister
PR: rainlanguage/rain.math.float#30
File: test/src/lib/LibDecimalFloat.gt.t.sol:33-36
Timestamp: 2025-04-25T03:58:01.307Z
Learning: In the rain.math.float library, all values of `Float` (which is a type alias for bytes32) are considered valid and can be safely used with methods like gt(), lt(), or eq() without causing reverts.
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.
🧬 Code Graph Analysis (1)
crates/float/src/lib.rs (1)
crates/float/src/evm.rs (1)
execute_call(34-63)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
🔇 Additional comments (8)
crates/float/Cargo.toml (1)
32-32: LGTM!Adding
anyhowas a dev dependency is appropriate for the error handling in test examples.crates/float/src/evm.rs (1)
15-63: Well-structured EVM execution abstraction.The module effectively encapsulates EVM execution logic with thread-safe access patterns and comprehensive error handling. The separation of concerns aligns well with the refactoring objectives.
crates/float/src/error.rs (1)
1-64: Excellent error handling design.The error module provides comprehensive error types with proper use of
thiserrorfor derived implementations. TheTryFromimplementation for error selector conversion is clean and maintainable.crates/float/src/lib.rs (5)
25-181: Consider handling documentation additions in a separate PR.Based on previous feedback, you prefer to handle documentation additions and improvements in separate issues rather than inline with feature PRs. While the documentation added here is comprehensive and well-written, you may want to split it into a dedicated documentation PR.
350-484: Excellent implementation of arithmetic traits.The arithmetic trait implementations properly return
Resulttypes for error handling and align with your preference for using standard Rust traits. The consistent use ofexecute_callmaintains good separation of concerns.
486-640: Great addition of utility methods.The new utility methods (
frac,floor,min,max,is_zero) expand the API with commonly needed functionality. The implementations are consistent with the existing pattern and include thorough documentation.
642-672: Proper implementation of the Neg trait.The
Negtrait implementation correctly returns aResultand uses theminusCallfrom the contract, maintaining consistency with other operations.
685-687: Good adherence to test conventions.The test correctly uses
.unwrap()instead of?operator, which aligns with your preference for clearer stack traces in test failures.
Comment on lines
+38
to
+43
| let result = LOCAL_EVM.try_with(|evm| { | ||
| let evm = &mut *evm.borrow_mut(); | ||
| let result_and_state = evm.transact_system_call_finalize(FLOAT_ADDRESS, calldata)?; | ||
|
|
||
| Ok::<_, FloatError>(result_and_state.result) | ||
| })??; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a comment to clarify the error propagation pattern.
The double ? operator pattern on line 43 might be confusing to readers. The first ? propagates any AccessError from try_with, and the second propagates the FloatError from the closure.
pub(crate) fn execute_call<F, T>(calldata: Bytes, process_output: F) -> Result<T, FloatError>
where
F: FnOnce(Bytes) -> Result<T, FloatError>,
{
+ // try_with can fail with AccessError if the thread local is already borrowed
+ // The inner closure can fail with FloatError from EVM execution
let result = LOCAL_EVM.try_with(|evm| {
let evm = &mut *evm.borrow_mut();
let result_and_state = evm.transact_system_call_finalize(FLOAT_ADDRESS, calldata)?;
Ok::<_, FloatError>(result_and_state.result)
})??;📝 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
| let result = LOCAL_EVM.try_with(|evm| { | |
| let evm = &mut *evm.borrow_mut(); | |
| let result_and_state = evm.transact_system_call_finalize(FLOAT_ADDRESS, calldata)?; | |
| Ok::<_, FloatError>(result_and_state.result) | |
| })??; | |
| pub(crate) fn execute_call<F, T>(calldata: Bytes, process_output: F) -> Result<T, FloatError> | |
| where | |
| F: FnOnce(Bytes) -> Result<T, FloatError>, | |
| { | |
| // try_with can fail with AccessError if the thread local is already borrowed | |
| // The inner closure can fail with FloatError from EVM execution | |
| let result = LOCAL_EVM.try_with(|evm| { | |
| let evm = &mut *evm.borrow_mut(); | |
| let result_and_state = evm.transact_system_call_finalize(FLOAT_ADDRESS, calldata)?; | |
| Ok::<_, FloatError>(result_and_state.result) | |
| })??; | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In crates/float/src/evm.rs around lines 38 to 43, add a comment above the line
with the double `?` operators explaining that the first `?` handles propagation
of `AccessError` from `try_with`, and the second `?` propagates the `FloatError`
from the closure. This will clarify the error handling flow for future readers.
Contributor
Author
There was a problem hiding this comment.
this doesn't seem to match the description of try_with
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@0xgleb, you're absolutely right to question my description. Let me check the actual behavior of try_with to provide an accurate explanation.
🏁 Script executed:
#!/bin/bash
# Search for the try_with method usage and documentation
rg "try_with" -A 5 -B 5
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Look at the error types and From implementations to understand the conversion
rg "AccessError" -A 3 -B 3
# Check the FloatError enum to see the Access variant
rg "enum FloatError" -A 10
Length of output: 1614
🌐 Web query:
Rust std::thread::LocalKey try_with method documentation behavior AccessError
💡 Result:
The std::thread::LocalKey::try_with method in Rust provides a way to safely access the value associated with a thread-local storage (TLS) key and gracefully handle cases where the value is not available. Here is a detailed description of its behavior, with particular focus on the AccessError type:
Behavior of try_with:
try_withaccepts a closure and attempts to execute it with a reference to the thread-local value, if that value is accessible.- It returns a
Result<R, AccessError>, whereRis the return type of the closure.
When does try_with return an AccessError?
AccessErroris returned if the thread-local value is not accessible, specifically if the thread-local key has already been destroyed (for example, during or after the thread's destruction phase, such as in a destructor)[10][4][1].- This is different from a typical panic: it allows you to handle the error without unwinding the stack.
- Note: If the key’s initializer itself panics while being called,
try_withwill panic rather than returnAccessError;AccessErroris only used for the destroyed/unavailable state[10].
AccessError details:
AccessErroris a non-exhaustive struct, meaning in future versions it could have additional variants or fields[10][4].- Its debug formatter displays as
AccessError, and its display formatter outputs "already destroyed"[10][4][1]. - It implements
Debug,Display,Clone,Copy,Eq,PartialEq, andError[10][4][1].
Example Usage:
thread_local! {
static FOO: String = String::from("hello");
}
let result = FOO.try_with(|val| val.len());
// result: Result<usize, std::thread::AccessError>If FOO is still valid, this will return Ok(length). If it is already destroyed (e.g., during TLS destructor), it returns Err(AccessError)[10].
Summary Table:
| Condition | try_with Result |
|---|---|
| Key accessible and available | Ok(value) |
| Key has been destroyed / already inaccessible | Err(AccessError) |
| Key initializer panics during initialization | Panic (does not return Err) |
Relevant documentation snippets:
This function will return an [
AccessError] if the key has been destroyed (typically, this only happens if you try to access a TLS value in a destructor)[10].
AccessErrordisplay: "already destroyed"[10][4][1].
In summary, LocalKey::try_with is the correct way to safely query a TLS value and can be used to avoid panics in drop-time code where the key might have been destroyed. If you need access to the value and can’t tolerate failure, use the with method, which panics on error instead[10][4].
Citations:
- 1: https://moshg.github.io/rust-std-ja/src/std/thread/local.rs.html
- 2: std::thread::LocalKey not found rust-lang/rust-analyzer#16486
- 3: thread 'main' panicked at 'called
Result::unwrap()on anErrvalue: ParseFloatError { kind: Invalid }', crates/proc_macro_srv/src/rustc_server.rs:514:32 stack backtrace: rust-lang/rust-analyzer#5974 - 4: https://github.com/rust-lang/rust/blob/master/library/std/src/thread/local.rs
- 5: https://users.rust-lang.org/t/why-do-methods-on-thread-localkey-take-static-self/115960
- 6: https://teaclave.apache.org/api-docs/sgx-sdk/sgx_tstd/thread/struct.LocalKey.html
- 7: Access to thread locals isn't inlined across crates rust-lang/rust#25088
- 8: https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/std/thread/struct.LocalKey.html
- 9: https://doc.rust-lang.org/std/macro.thread_local.html
- 10: https://doc.rust-lang.org/src/std/thread/local.rs.html
@0xgleb, you're absolutely correct! My description was inaccurate.
Looking at the documentation, try_with returns an AccessError when the thread-local key has been destroyed (typically during thread destruction/cleanup), not when it's already borrowed. The borrowing conflicts would be handled by the borrow_mut() call inside the closure, which would panic rather than return an error.
The correct explanation for the double ? pattern is:
- First
?: ConvertsAccessErrorfromtry_with(when the thread-local key is destroyed) toFloatError::Accessvia theFromimplementation - Second
?: Propagates theFloatErrorfrom the inner closure
So a more accurate comment would be:
// try_with can fail with AccessError if the thread local key has been destroyed
// The inner closure can fail with FloatError from EVM execution
let result = LOCAL_EVM.try_with(|evm| {
// ... rest of the code
})??;Thank you for catching that mistake! The original suggestion about borrowing was incorrect.
🧠 Learnings used
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:447-461
Timestamp: 2025-06-17T10:11:32.740Z
Learning: In the rainlanguage/rain.math.float repository, the user 0xgleb prefers using .unwrap() over ? in tests because it provides clearer stack traces showing the exact line where panics occurred, making debugging easier.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
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.
Contributor
Author
There was a problem hiding this comment.
I think it's clear enough that when two question marks are used that that's due to nested Result types
Contributor
There was a problem hiding this comment.
@0xgleb, you're absolutely right! The double ? pattern is indeed a well-established Rust idiom that experienced developers will immediately recognize as handling nested Result types. Adding a comment would be redundant for such a common pattern.
Thank you for the correction on the try_with behavior earlier as well - it's a good reminder to verify the specifics before making assumptions.
🐰✨
✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#70
File: crates/float/src/evm.rs:38-43
Timestamp: 2025-07-03T11:20:50.441Z
Learning: In the rainlanguage/rain.math.float codebase, the user 0xgleb prefers not to add explanatory comments for well-established Rust idioms like the double `?` pattern, as these are self-explanatory to experienced Rust developers and don't need over-commenting.
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#58
File: crates/float/src/lib.rs:201-232
Timestamp: 2025-06-16T13:14:38.431Z
Learning: In the rain.math.float Rust crate, the `execute_call` function already provides sufficient abstraction for EVM contract calls by handling execution boilerplate, error handling, and result processing. Individual methods like `lt`, `eq`, `gt` only need to handle their specific call encoding and result decoding, making further abstraction unnecessary.
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.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#59
File: crates/float/src/lib.rs:447-461
Timestamp: 2025-06-17T10:11:32.740Z
Learning: In the rainlanguage/rain.math.float repository, the user 0xgleb prefers using .unwrap() over ? in tests because it provides clearer stack traces showing the exact line where panics occurred, making debugging easier.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: src/concrete/DecimalFloat.sol:175-182
Timestamp: 2025-06-16T13:17:28.513Z
Learning: In the rainlanguage/rain.math.float codebase, there's an established naming convention where functions accepting a `Float` type parameter consistently use `float` as the parameter name, even though it shadows the type name. This pattern is used throughout `LibDecimalFloat.sol` and should be maintained for consistency in related contracts like `DecimalFloat.sol`.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#58
File: crates/float/src/lib.rs:382-401
Timestamp: 2025-06-16T13:19:32.009Z
Learning: In the rainlanguage/rain.math.float codebase, variable shadowing in property tests like test_lt_eq_gt_with_add is acceptable when it represents a logical progression of the same conceptual value being tested through different states.
Learnt from: 0xgleb
PR: rainlanguage/rain.math.float#57
File: crates/float/src/lib.rs:558-574
Timestamp: 2025-06-18T09:20:39.225Z
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.
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.
findolor
approved these changes
Jul 3, 2025
hardyjosh
approved these changes
Jul 3, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Make it easier to use and maintain the Rust library
Solution
Add docs and refactor
Checks
By submitting this for review, I'm confirming I've done the following: