-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/apy adjustments #6
base: master
Are you sure you want to change the base?
Conversation
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.
I'd also unit test the functions because the math is tricky. But in general treat my comments as suggestions.
src/app/api/pool/summary/route.ts
Outdated
]; | ||
|
||
/* | ||
This code calculates the annual reward based on block height. |
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.
Technically it's not a reward but subsidy. Reward = subsidy + tx fees.
src/app/api/pool/summary/route.ts
Outdated
@@ -34,6 +34,89 @@ async function getPoolsRecursive(offset: number, pools: any = []): Promise<any> | |||
}, []); | |||
} | |||
|
|||
const rewards = [ |
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.
- We call it "emission table" which is more precise term.
- I see 2 more duplicates of this table.
src/app/api/pool/summary/route.ts
Outdated
const get_annual_reward = (block_height: number) => { | ||
const current = rewards.findIndex((r) => r.start <= block_height && r.end > block_height); | ||
const current_reward = rewards[current]?.reward; | ||
const current_reward_end = rewards[current]?.end || 1_000_000_000_000_000; |
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 sure that this will behave correctly during the Infinity
. Seems like line 116 will give negative result.
return current_reward_part + future_reward_part; | ||
} | ||
|
||
const get_annual_reward = (block_height: number) => { |
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.
Is it a copy-paste of function in route.ts? Can it be reused somehow?
@@ -79,7 +131,7 @@ export function Table() { | |||
const filterer = (item: any) => { | |||
if (hideNonProfitPools) { | |||
if (item.margin_ratio === 1) return false; | |||
if (item.cost_per_block === 202) return false; | |||
if (item.cost_per_block >= 151) return false; |
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.
It's better to introduce a function like block_subsidy_at_height
@@ -113,17 +165,20 @@ export function Table() { | |||
const injectApy = (stakingAmount: any) => (pool: any) => { |
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.
Is it amountToDelegate
?
// APY estimation | ||
const blocks_per_day = probability_per_second * 24 * 60 * 60; | ||
const part = stakingAmount / (stakingAmount + pool.delegations_amount + pool.staker_balance); | ||
const reward_per_block = (reward - pool.cost_per_block) * (1 - pool.margin_ratio) * part; | ||
const apy = ((reward_per_block * blocks_per_day * 365) / stakingAmount) * 100; // * 100 to display percent | ||
// const reward_per_block = (reward - pool.cost_per_block) * (1 - pool.margin_ratio) * part; |
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 feels like half-encapsulated. I'd move part
calculation inside and also give it a proper name because it's very confusing right now.
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.
part
can't be moved inside since it's required below to make a label
const part_label = (part * 100).toFixed(2);
fad2c13
to
b71d570
Compare
return total; | ||
} | ||
|
||
export const block_subsidy_at_height = (block_height: number) => { |
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.
you can now use it in the functions below
@@ -0,0 +1,57 @@ | |||
export const EMISSION_TABLE = [ | |||
{ start: 0 , end: 262800 , reward: 202 }, |
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.
I think typical notation is [begin; end) and [first; last] where end
is always exclusive. That would avoid confusion on whether reward for 262800 is 202 or 151
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.
What about
{ range: [0, 262800], subsidy: 202 },
{ range: [262801, 525600], subsidy: 151 },
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.
[first; last] also good to me
return current_subsidy_part + future_subsidy_part; | ||
} | ||
|
||
export const get_annual_subsidy_delegator = (block_height: number, pool: any, part: any) => { |
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.
I'd leave a comment what part
means or find a better name for it
expect(get_annual_subsidy(1)).toEqual(53085549); // 202 * 262799 + 151 * 1 | ||
expect(get_annual_subsidy(10)).toEqual(53085090); // 202 * 262790 + 151 * 10 | ||
expect(get_annual_subsidy(262800)).toEqual(39682800); // 151 * 262800 | ||
expect(get_annual_subsidy(262801)).toEqual(39682762); // 151 * 262799 + 113 |
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.
testing delegators logic would be nice
export const get_annual_subsidy = (block_height: number) => { | ||
const current = EMISSION_TABLE.findIndex((r) => r.start <= block_height && r.end >= block_height); | ||
const current_subsidy = EMISSION_TABLE[current]?.reward; | ||
const current_subsidy_end = EMISSION_TABLE[current]?.end || Infinity; |
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.
I still think that it's incorrect to use Infinity here because you subtract it on line 44. Also there is no test for such case to prove otherwise.
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.
that's correct, fixing that case
e83d09e
to
c99f31d
Compare
c99f31d
to
a92ef0e
Compare
No description provided.