add_recoverable_error + bump version to 0.9.0 #28

malik672 · 2024-01-08T19:09:23Z

switched from unrecoverable error to recoverable errors, to improve error handling
added to error.rs file for custom errors
bump version
fixed issue #26

shuhuiluo · 2024-01-08T19:12:44Z

.DS_Store

Add to .gitignore.

shuhuiluo · 2024-01-09T10:31:02Z

src/entities/token.rs

+    ) -> Result<Self, Error> {
+        if chain_id == 0 {
+            return Err(Error::ChainIdError { field: "CHAIN_ID" });
+        }
+        Ok(Self {


I don't think the chain_id == 0 edge case is worth handling. Returning a Result also complicates the syntax.

src/error.rs

shuhuiluo · 2024-01-09T10:55:09Z

src/error.rs

+    #[error("incorrect: {0}")]
+    Incorrect(String),


Other(String)?

shuhuiluo · 2024-01-09T11:36:58Z

src/entities/fractions/price.rs

+    pub fn adjusted_for_decimals(&self) -> Result<Fraction, Error> {
+        self.as_fraction()? * self.meta.scalar.clone()


Suggested change

pub fn adjusted_for_decimals(&self) -> Result<Fraction, Error> {

self.as_fraction()? * self.meta.scalar.clone()

pub fn adjusted_for_decimals(&self) -> Fraction {

self.as_fraction() * self.meta.scalar.clone()

shuhuiluo · 2024-01-09T11:39:07Z

src/entities/token.rs

@@ -92,6 +98,7 @@ macro_rules! token {
            None,
            None,
        )
+        .expect("failed to create token")


Suggested change

.expect("failed to create token")

src/entities/token.rs

shuhuiluo · 2024-01-09T11:39:18Z

src/entities/token.rs

@@ -114,6 +122,7 @@ macro_rules! token {
            None,
            None,
        )
+        .expect("failed to create token")


Suggested change

.expect("failed to create token")

shuhuiluo · 2024-01-09T11:41:22Z

src/utils/compute_price_impact.rs

+            .divide(&quoted_output_amount),
+        Err(e) => Err(e),
+    };
+    let price_impact_clone = price_impact?.clone();


It doesn't need to be cloned.

yeah i know, but i placed it incase price_impact was ever used again in the same closure

shuhuiluo · 2024-01-09T11:52:37Z

src/entities/fractions/fraction.rs

@@ -165,7 +182,7 @@ impl<M: Clone> PartialOrd<Self> for FractionLike<M> {
 }

 impl<M: Clone> Add for FractionLike<M> {
-    type Output = Self;
+    type Output = Result<Self, Error>;


FractionTrait::new only fails when denominator is zero, which is not possible here.

shuhuiluo · 2024-01-09T11:53:01Z

src/entities/fractions/fraction.rs

@@ -186,7 +203,7 @@ impl<M: Clone> Add for FractionLike<M> {
 }

 impl<M: Clone> Sub for FractionLike<M> {
-    type Output = Self;
+    type Output = Result<Self, Error>;


FractionTrait::new only fails when denominator is zero, which is not possible here.

shuhuiluo · 2024-01-09T11:53:24Z

src/entities/fractions/fraction.rs

@@ -207,7 +224,7 @@ impl<M: Clone> Sub for FractionLike<M> {
 }

 impl<M: Clone> Mul for FractionLike<M> {
-    type Output = Self;
+    type Output = Result<Self, Error>;


FractionTrait::new only fails when denominator is zero, which is not possible here.

shuhuiluo · 2024-01-09T12:00:56Z

src/entities/fractions/fraction.rs

@@ -57,21 +64,23 @@ where
    }

    // Returns the remainder after floor division as a new fraction
-    fn remainder(&self) -> Self {
+    fn remainder(&self) -> Result<Self, Error> {


FractionTrait::new only fails when denominator is zero, which is not possible here.

shuhuiluo · 2024-01-10T10:26:34Z

.DS_Store

Remove from git indexing.

shuhuiluo · 2024-01-10T10:26:40Z

src/.DS_Store

Remove from git indexing.

shuhuiluo · 2024-01-10T10:27:38Z

src/entities/fractions/currency_amount.rs

+    pub fn from_raw_amount(
+        currency: T,
+        raw_amount: impl Into<BigInt>,
+    ) -> Result<CurrencyAmount<T>, Error> {


Suggested change

) -> Result<CurrencyAmount<T>, Error> {

) -> Result<Self, Error> {

nit

shuhuiluo · 2024-01-10T10:27:47Z

src/entities/fractions/currency_amount.rs

@@ -40,23 +49,23 @@ impl<T: CurrencyTrait> CurrencyAmount<T> {
        currency: T,
        numerator: impl Into<BigInt>,
        denominator: impl Into<BigInt>,
-    ) -> CurrencyAmount<T> {
+    ) -> Result<CurrencyAmount<T>, Error> {


Suggested change

) -> Result<CurrencyAmount<T>, Error> {

) -> Result<Self, Error> {

nit

shuhuiluo · 2024-01-10T10:30:37Z

src/entities/fractions/fraction.rs

@@ -108,23 +115,29 @@ where
    }

    // Helper method for converting any superclass back to a simple Fraction
-    fn as_fraction(&self) -> Fraction {
+    fn as_fraction(&self) -> Result<Fraction, Error> {


Can you give an example when this will error?

shuhuiluo · 2024-01-10T10:36:01Z

src/entities/fractions/currency_amount.rs

+            multiplied.clone().numerator().clone(),
+            multiplied.clone().denominator().clone(),


I don't get the purpose of these clones. It affects nothing but performance.

fraction::new takes ownership of the data the only way to do this i know is to clone

numerator and denominator take &self. They don't need a fresh instance of Fraction. Only the BigInt numerator and denominator need to be cloned. The code was working fine.

shuhuiluo · 2024-01-10T10:44:53Z

src/entities/fractions/currency_amount.rs

+        println!("{:?}", amount.clone().to_exact());
+        // assert_eq!(amount.clone().to_exact(), "123456");


Why is it commented out?

lmao, forgot I ever did something like this

shuhuiluo · 2024-01-10T11:13:46Z

src/entities/fractions/price.rs

+            return Err(Error::NotEqual("the comparison are not equal".to_owned()));
+        }
+
+        let fraction = (self.as_fraction()? * other.as_fraction()?)?.clone();


Suggested change

let fraction = (self.as_fraction()? * other.as_fraction()?)?.clone();

let fraction = self.as_fraction() * other.as_fraction();

shuhuiluo · 2024-01-10T11:13:58Z

src/entities/fractions/price.rs

+        {
+            return Err(Error::NotEqual("the comparison are not equal".to_owned()));
+        }
+        let fraction = (self.as_fraction()? * currency_amount.as_fraction()?)?.clone();


Suggested change

let fraction = (self.as_fraction()? * currency_amount.as_fraction()?)?.clone();

let fraction = self.as_fraction() * currency_amount.as_fraction();

i actually wanted to do this, but somehow, they are all linked way to the top, the easiest and possible way to solve this that i saw was to remove this

impl<T: CurrencyTrait> CurrencyAmount<T> { // Constructor method for creating a new currency amount fn new( currency: T, numerator: impl Into<BigInt>, denominator: impl Into<BigInt>, ) -> Result<Self, Error> { let numerator = numerator.into(); let denominator = denominator.into(); // Ensure the amount does not exceed MAX_UINT256 if !numerator.div_floor(&denominator).le(&MAX_UINT256) { return Err(Error::MaxUint); } let exponent = currency.decimals(); FractionTrait::new( numerator, denominator, CurrencyMeta { currency, decimal_scale: BigUint::from(10u64).pow(exponent as u32), }, ) }

Not sure we're on the same page. What I suggested was making as_fraction and mul return Self instead of Result. Rationale:

FractionLike::new only errors if the denominator is 0.

Any existing FractionLike must have nonzero denominator.

Converting any FractionLike to a base Fraction is impossible to error, which means we can safely unwrap FractionTrait::new in as_fraction and return Self.

When adding, subtracting or multiplying two FractionLike, the denominator also cannot be zero. Therefore we should return Self instead of Result for these ops for the sake of readability, dev experience and performance.

the fraction::new should not return an errror instead, in my opinion it should panic

It would make the code a lot cleaner. For example in Solidity, dividing by 0 is a panic.

shuhuiluo · 2024-01-10T19:42:59Z

target was pushed?

malik672 · 2024-01-10T19:47:58Z

@shuhuiluo yeah mistake, just pruned the branch

add_recoverable_error + bump version to 0.8.0

5a03a78

malik672 requested a review from shuhuiluo January 8, 2024 19:09

fix linter issue

758cb12

shuhuiluo requested changes Jan 9, 2024

View reviewed changes

shuhuiluo linked an issue Jan 10, 2024 that may be closed by this pull request

Improve error handling #26

Closed

malik672 added 2 commits January 10, 2024 09:12

implement suggested fixes

dc5747c

implement suggested fixes

f6724ac

malik672 force-pushed the add_recoverable_error branch from 5724e17 to f6724ac Compare January 10, 2024 08:14

malik672 requested a review from shuhuiluo January 10, 2024 08:53

shuhuiluo requested changes Jan 10, 2024

View reviewed changes

fixed + bump to 0.9.0

3f313f2

malik672 changed the title ~~add_recoverable_error + bump version to 0.8.0~~ add_recoverable_error + bump version to 0.9.0 Jan 10, 2024

malik672 requested a review from shuhuiluo January 10, 2024 18:49

malik672 closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add_recoverable_error + bump version to 0.9.0 #28

add_recoverable_error + bump version to 0.9.0 #28

malik672 commented Jan 8, 2024

shuhuiluo Jan 8, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

malik672 Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 9, 2024

shuhuiluo Jan 10, 2024

malik672 Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

malik672 Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

malik672 Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo Jan 10, 2024

malik672 Jan 10, 2024 •

edited by shuhuiluo

Loading

shuhuiluo Jan 10, 2024

malik672 Jan 10, 2024

malik672 Jan 10, 2024

shuhuiluo Jan 10, 2024

shuhuiluo commented Jan 10, 2024

malik672 commented Jan 10, 2024

		pub fn adjusted_for_decimals(&self) -> Result<Fraction, Error> {
		self.as_fraction()? * self.meta.scalar.clone()

	) -> Result<CurrencyAmount<T>, Error> {
	) -> Result<Self, Error> {

		multiplied.clone().numerator().clone(),
		multiplied.clone().denominator().clone(),

		println!("{:?}", amount.clone().to_exact());
		// assert_eq!(amount.clone().to_exact(), "123456");

	let fraction = (self.as_fraction()? * other.as_fraction()?)?.clone();
	let fraction = self.as_fraction() * other.as_fraction();

	let fraction = (self.as_fraction()? * currency_amount.as_fraction()?)?.clone();
	let fraction = self.as_fraction() * currency_amount.as_fraction();

add_recoverable_error + bump version to 0.9.0 #28

add_recoverable_error + bump version to 0.9.0 #28

Conversation

malik672 commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malik672 Jan 10, 2024 • edited by shuhuiluo Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuhuiluo commented Jan 10, 2024

malik672 commented Jan 10, 2024

malik672 Jan 10, 2024 •

edited by shuhuiluo

Loading