Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change FixedPoint to only support From<U128> and adjust tests #127

Closed
dpaiton opened this issue Jun 5, 2024 · 1 comment
Closed

Change FixedPoint to only support From<U128> and adjust tests #127

dpaiton opened this issue Jun 5, 2024 · 1 comment
Assignees

Comments

@dpaiton
Copy link
Member

dpaiton commented Jun 5, 2024

Modify the U256 from impl to check if the value fits in a U128. If so, do the conversion. Else (value is > U128::max) throw an error.

Adjust the tests to use U128::max instead of U256::max everywhere.

We should make sure to add docstrings somewhere to indicate to the user that FixedPoint has max U128 support.

@ryangoree ryangoree self-assigned this Jul 28, 2024
ryangoree added a commit that referenced this issue Aug 13, 2024
Related to: #127

(replaces wip pr: #184)

# Description

This refactors the `FixedPoint` type to be generic with implementations
for `i128`, `u128`, `I256`, and `U256`. For the most part, the library
can be used the same, but with a type param, eg:

```diff
- fn foo() -> FixedPoint {}
+ fn foo() -> FixedPoint<U256> {}
```

However, the API has been expanded and there's plenty code reduction we
can do in future PRs. It's probably also worth rethinking argument types
now that the library can support negative numbers and be bound by
different limits.

## Expanded API

I wrote doc comments throughout to aid in adoption, but below is a
skeleton containing most of the new API to get a quick sense:

```rs
pub struct FixedPoint<T: FixedPointValue> {
    raw: T,
    decimals: u8,
}

impl<T: FixedPointValue> FixedPoint<T> {
    pub const MIN: Self;
    pub const MAX: Self;

    // Constructors //
    pub fn new<V: Into<T>>(value: V) -> Self;
    pub fn try_from<V: TryInto<T> + Debug>(value: V) -> Result<Self>;
    pub fn from_sign_and_abs(sign: FixedPointSign, abs: U256) -> Result<Self>;
    pub fn from_dec_str(s: &str) -> Result<Self>;
    pub fn saturate_sign(sign: FixedPointSign) -> Self;
    pub fn zero() -> Self;
    pub fn one(&self) -> Self;

    // Getters //
    pub fn raw(&self) -> T;
    pub fn decimals(&self) -> u8;
    pub fn sign(&self) -> FixedPointSign;

    // Predicates //
    pub fn is_negative(&self) -> bool;
    pub fn is_positive(&self) -> bool;
    pub fn is_zero(&self) -> bool;

    // Conversion to other FixedPoint types //
    pub fn change_type<U: FixedPointValue + TryFrom<T>>(self) -> Result<FixedPoint<U>>;

    // Conversion to unsigned & signed ethers types //
    pub fn to_u256(self) -> Result<U256>;
    pub fn to_i256(self) -> Result<I256>;

    // Conversion to unsigned and signed std types //
    pub fn to_u128(self) -> Result<u128>;
    pub fn to_i128(self) -> Result<i128>;

    // Formatting //
    pub fn to_scaled_string(&self) -> String;

    // Math //
    pub fn abs(&self) -> Self;
    pub fn unsigned_abs(&self) -> FixedPoint<U256>; // `U256` to avoid overflow.
    pub fn abs_diff(&self, other: Self) -> FixedPoint<U256>;
    pub fn mul_div_down(self, other: Self, divisor: Self) -> Self;
    pub fn mul_div_up(self, other: Self, divisor: Self) -> Self;
    pub fn mul_down(self, other: Self) -> Self;
    pub fn mul_up(self, other: Self) -> Self;
    pub fn div_down(self, other: Self) -> Self;
    pub fn div_up(self, other: Self) -> Self;
    pub fn pow(self, y: Self) -> Result<Self>;
}

// Direct conversions between primitive types and FixedPoint for any
// `FixedPointValue` that can be converted to and from the primitive type.
conversion_impls!(i8, u8, i16, u16, i32, u32, i64, u64, isize, usize, [u8; 32], bool);

// Value //

/// A value that can be used to perform fixed-point math.
pub trait FixedPointValue: /* ... */ {
    const MIN: Self;
    const MAX: Self;
    const MAX_DECIMALS: u8 = 18;

    fn is_signed() -> bool;
    fn is_negative(&self) -> bool;
    fn is_positive(&self) -> bool;
    fn is_zero(&self) -> bool;
    fn flip_sign(self) -> Self;
    fn flip_sign_if(self, condition: bool) -> Self;
    fn abs(self) -> Self;
    fn unsigned_abs(self) -> U256;

    pub fn from_u256(u: U256) -> Result<Self>;
    pub fn from_u128(u: u128) -> Result<Self>;
    
    pub fn to_u256(self) -> Result<U256>;
    pub fn to_u128(self) -> Result<u128>;
}

// Conversions traits //

pub trait Fixed: FixedPointValue {
    fn fixed(self) -> FixedPoint<Self>;
}

// Add `.fixed()` to all types that implement `FixedPointValue`.
impl<T: FixedPointValue> Fixed for T {}

pub trait ToFixed: Sized + Debug {
    fn to_fixed<T: FixedPointValue + From<Self>>(self) -> FixedPoint<T>;
    fn try_to_fixed<T: FixedPointValue + TryFrom<Self>>(self) -> Result<FixedPoint<T>>;
}

// Add `.to_fixed()` & `.try_to_fixed()` to all sized types that implement
// `Debug`.
impl<T: Sized + Debug> ToFixed for T {}

// Macros //

macro_rules! uint256 {}    // -> `U256`
macro_rules! int256 {}     // -> `I256`
macro_rules! fixed {}      // -> `FixedPoint<T>` (where `T` is inferred)
macro_rules! fixed_u256 {} // -> `FixedPoint<U256>`
macro_rules! fixed_i256 {} // -> `FixedPoint<I256>`
macro_rules! fixed_u128 {} // -> `FixedPoint<u128>`
macro_rules! fixed_i128 {} // -> `FixedPoint<i128>`
```
@ryangoree
Copy link
Member

ryangoree commented Aug 13, 2024

The catalyst for this issue was the fact that we couldn't divide a FixedPoint instance that was > U256::MAX / 1e18. The proposed solution was to use smaller inputs that wouldn't overflow the U256 when scaled up by 1e18.

With #186 landing, this issue has been resolved and the lib uses U512 as an intermediary type in mul_div_down & mul_div_up to avoid unexpected overflows given valid inputs. It also makes the FixedPoint type generic so we can support both U256 and u128.

This has a few implications:

  1. We don't have to change to a u128 anymore just to avoid this error.
  2. The solidity still has this limitation, so tests will still need to limit max values to U256::MAX / 1e18 like so:
    /// The maximum number that can be divided by another in the Solidity
    /// implementation.
    fn max_sol_numerator() -> FixedPoint<U256> {
    (U256::MAX / uint256!(1e18)).into()
    }

    let max = max_sol_numerator();
    let a = rng.gen_range(0.into()..=max);
    let b = rng.gen_range(0.into()..=max);
    let actual = panic::catch_unwind(|| a.div_down(b));
    match mock_fixed_point_math
    .div_down(a.raw(), b.raw())
    .call()
    .await
  3. Where we still want tighter bounds, we can swap FixedPoint<U256> with FixedPoint<u128>, FixedPoint<i128>, or FixedPoint<I256>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants