-
Notifications
You must be signed in to change notification settings - Fork 5
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
apy #945
base: main
Are you sure you want to change the base?
apy #945
Conversation
This reverts commit 79f5f40.
- fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing screenshots on the PR
crates/subgraph/src/utils/mod.rs
Outdated
mod order_id; | ||
mod slice_list; | ||
|
||
pub use order_id::*; | ||
pub use slice_list::*; | ||
|
||
pub const DAY: u64 = 60 * 60 * 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using u64 for times is kinda dangerous because it's really easy for someone to mistake whether it is seconds, millis, nanos, etc.
why not just use the time functionality in rust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used rust chrono::TimeDelta
import { formatUnits } from 'viem'; | ||
import OrderApy from './OrderAPY.svelte'; | ||
|
||
function formatApyToPercentage(value: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have an ad hoc function for this instead of a standard way to format numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to a relevant place and made it to be more general, so can be used elsewhere when there is a nee to convert a bigint to a percentage
@thedavidmeister btw can show each individual token vault apy in its own denomination ofc, in a sep table/chart if that's desirable, apart from the order's whole APY |
} | ||
|
||
/// Returns annual rate as 18 point decimals as I256 | ||
pub fn annual_rate(start: u64, end: u64) -> I256 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this function just take two 18 decimal values and calculate the rate?
the way it is now, it complects a bunch of stuff
- converting u64 to i256
- rescaling decimal 0 to decimal 18
- ad hoc inlined 18 decimal devision
Ok(self | ||
.input_as_18_decimals()? | ||
.get_absolute() | ||
.saturating_mul(one_18().get_absolute()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as below
pub fn inverse_ratio(&self) -> Result<U256, ParseNumberError> { | ||
Ok( | ||
TryInto::<U256>::try_into(self.output_as_18_decimals()?.get_signed().saturating_neg())? | ||
.saturating_mul(one_18().get_absolute()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as below
} | ||
|
||
/// Converts this trade's output to 18 point decimals in U256/I256 | ||
pub fn output_as_18_decimals(&self) -> Result<ParseUnits, ParseNumberError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic basically duplicates the input one
.and_then(|amount| { | ||
to_18_decimals( | ||
ParseUnits::U256(amount), | ||
vault_balance_change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not using impl
(!starting_capital.is_zero()) | ||
.then_some( | ||
vol.net_vol | ||
.saturating_mul(one_18().get_signed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline rescaling
Motivation
resolves #908
Solution
Checks
By submitting this for review, I'm confirming I've done the following: