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

Overflow on fraction of duration #370

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

maneva3
Copy link
Collaborator

@maneva3 maneva3 commented Jul 17, 2024

No description provided.

@maneva3 maneva3 requested a review from gmanev7 July 17, 2024 13:17
Copy link
Member

@gmanev7 gmanev7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed only finance without liability and interest modules
once address the findings , on the next iteration I'll continue with the rest

platform/contracts/treasury/src/error.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/duration.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/error.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/fraction.rs Outdated Show resolved Hide resolved
use crate::fractionable::Fractionable;
assert_eq!(
Coin::<SuperGroupTestC1>::new(30),
Coin::<SuperGroupTestC1>::new(3).safe_mul(&Percent::from_percent(1000))
Fractionable::<u32>::checked_mul(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[everywhere] not sure we need the extended syntax when calling checked_mul. More concise and readable is if we call it on the first operand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the original code cannot be compiled?

platform/packages/finance/src/fractionable/duration.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/fractionable/mod.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/fractionable/percent.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/price/mod.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/price/mod.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/coin/mod.rs Show resolved Hide resolved
platform/packages/finance/src/error.rs Outdated Show resolved Hide resolved
use crate::fractionable::Fractionable;
assert_eq!(
Coin::<SuperGroupTestC1>::new(30),
Coin::<SuperGroupTestC1>::new(3).safe_mul(&Percent::from_percent(1000))
Fractionable::<u32>::checked_mul(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the original code cannot be compiled?

platform/packages/finance/src/duration.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/duration.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/percent/mod.rs Outdated Show resolved Hide resolved
platform/packages/finance/src/fraction.rs Outdated Show resolved Hide resolved
.try_into()
.expect("overflow computing a fraction of duration")
let d128: u128 = self.into();
Fractionable::<Coin<C>>::checked_mul(d128, fraction)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified that the direct calling syntax does not work? We did not add more implementations so d128.checked_mul(fraction) should work fine

Percent::from_permille(self.units().safe_mul(ratio))
Fractionable::<Units>::checked_mul(self.units(), ratio).map(Self::from_permille)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, I have to clue why the former calling syntax is not applicable anymore

@maneva3 maneva3 force-pushed the overflow_on_fraction_of_duration branch 2 times, most recently from 1de3926 to e91e7a1 Compare August 8, 2024 10:25
where
A: Fractionable<U>;
A: Fractionable<U> + Display + Clone;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not require Display + Clone

Copy link
Member

@gmanev7 gmanev7 Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the motivation for that?

@maneva3 maneva3 force-pushed the overflow_on_fraction_of_duration branch 2 times, most recently from f879640 to b384923 Compare August 19, 2024 14:05
@maneva3 maneva3 force-pushed the overflow_on_fraction_of_duration branch 2 times, most recently from f2d5176 to 3e867a0 Compare September 18, 2024 06:44
@maneva3 maneva3 force-pushed the overflow_on_fraction_of_duration branch from 5b81c33 to 2eeda67 Compare September 20, 2024 07:29
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 this pull request may close these issues.

2 participants