Extract U256Ext implementations into the numbers crate #4030
Conversation
squadgazzz
changed the title
Summary of ChangesHello @squadgazzz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the handling of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good refactoring that centralizes the U256Ext trait and its implementations into the numbers crate, reducing code duplication. However, I've identified a significant issue with potential precision loss and overflow in the checked_mul_f64 method, which I've flagged as high severity. I've also included a medium-severity suggestion to improve code consistency. Overall, the direction is great, but these points should be addressed to ensure correctness and maintainability.
| fn checked_mul_f64(&self, factor: f64) -> Option<Self> { | ||
| const CONVERSION_FACTOR: f64 = 1_000_000_000_000_000_000.; | ||
| let factor_as_u256 = PrimitiveU256::from((factor * CONVERSION_FACTOR) as u128); | ||
| let conversion_factor_u256 = PrimitiveU256::from(CONVERSION_FACTOR as u128); | ||
| let multiplied = self | ||
| .checked_mul(factor_as_u256)? | ||
| .checked_div(conversion_factor_u256)?; | ||
| Some(multiplied) | ||
| } |
There was a problem hiding this comment.
Similar to the implementation for AlloyU256, this implementation of checked_mul_f64 can lead to precision loss or overflow. The explicit cast as u128 can be lossy or overflow if factor * CONVERSION_FACTOR exceeds u128::MAX. Also, negative factors will be cast to 0. Using BigRational is a much safer approach and was suggested in a TODO in the original code.
fn checked_mul_f64(&self, factor: f64) -> Option<Self> {
if factor.is_sign_negative() {
return None;
}
let self_rational = self.to_big_rational();
let factor_rational = BigRational::from_f64(factor)?;
let result_rational = self_rational * factor_rational;
Self::from_big_rational(&result_rational).ok()
}There was a problem hiding this comment.
That probably makes sense. But I will address it in a separate PR to avoid any logic changes here.
| fn to_big_int(&self) -> BigInt { | ||
| crate::conversions::u256_to_big_int(self) | ||
| } |
There was a problem hiding this comment.
For consistency with the AlloyU256 implementation and to make the impl block more self-contained, consider implementing to_big_int directly instead of delegating to crate::conversions::u256_to_big_int. The logic is simple and can be inlined here.
fn to_big_int(&self) -> BigInt {
BigInt::from_biguint(num::bigint::Sign::Plus, self.to_big_uint())
}
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
LGTM, we don't need to change the div_ceil because I'm not 100% sure they match, though I'd like it very much
| fn checked_ceil_div(&self, other: &Self) -> Option<Self> { | ||
| self.checked_add(other.checked_sub(1.into())?)? | ||
| .checked_div(*other) | ||
| } |
There was a problem hiding this comment.
This looks like div_ceil but implemented in a weird way
https://docs.rs/ruint/1.17.2/ruint/struct.Uint.html#method.div_ceil
There was a problem hiding this comment.
Yeah, it is just a copy-paste from the old code. Will try to address it in a separate PR.
| fn from_big_int(input: &BigInt) -> Result<Self> { | ||
| anyhow::ensure!(input.sign() != num::bigint::Sign::Minus, "negative"); | ||
| Self::from_big_uint(input.magnitude()) | ||
| } | ||
|
|
||
| fn from_big_uint(input: &BigUint) -> Result<Self> { | ||
| let bytes = input.to_bytes_be(); | ||
| anyhow::ensure!(bytes.len() <= 32, "too large"); | ||
| Ok(AlloyU256::from_be_slice(&bytes)) | ||
| } |
There was a problem hiding this comment.
Eventually, we could replace this with the official implementation
https://github.com/recmo/uint/blob/5bd4cff6ae3960591d750cdd7356e24aa086b67a/src/support/num_bigint.rs
m-sz
left a comment
m-sz
left a comment
There was a problem hiding this comment.
LGTM, thanks. This is a very welcome change.
# Conflicts: # crates/autopilot/src/domain/competition/winner_selection.rs
Description
As noted in #4010, several U256Ext utility implementations are scattered across different crates in the codebase, some of which are duplicated. This PR moves all of them into a single U256Ext trait under the
numberscrate. No logic changes. As a result, easier to keep everything in one place, no duplicated logic.