-
Notifications
You must be signed in to change notification settings - Fork 79
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
Baseline bug fix (FIP-0081) #1557
Changes from 20 commits
102ee19
7b582ad
6da9f6a
e76fd96
ad45428
f4f11cd
63b7ecd
adf43f0
6825568
2d245ca
60d63d3
c19558e
1bd140e
a140b68
22cc3c8
330ef9d
06ff09c
bceeb2c
417f9e1
37c442d
252a358
8c969b4
e1836c0
7f09f47
1611afe
5c361a5
c5b2658
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,10 +38,122 @@ fn initial_pledge_clamped_at_one_attofil() { | |
&reward_estimate(), | ||
&power_estimate(), | ||
&circulating_supply(), | ||
// NOTE: setting this to zero preserves the original pledge definition (before baseline bug fix) | ||
ZenGround0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// so these inputs configure the function to return the original pledge | ||
0, | ||
0, | ||
); | ||
assert_eq!(TokenAmount::from_atto(1), initial_pledge); | ||
} | ||
|
||
// Pre-ramp where 'baseline power' dominates | ||
#[test] | ||
fn initial_pledge_pre_ramp_negative() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
-100, | ||
100, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1500).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
// Pre-ramp where 'baseline power' dominates | ||
#[test] | ||
fn initial_pledge_pre_ramp() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found these a bit hard to mentally verify. Could you please expand them, e.g. by using named vals for the parameters, and spell out in words the expected result. I suggest not using the consts defined for the "clamping" test – mixing and matching those with inline values couples the tests together and makes it hard to trace what the values are intended for. You can probably combine all these new cases into a single test for re-use. E.g. "With a fixed reward of zero and circulating supply of 1, a sector contributing 1/2 the current network QAP requires pledge of 1/2 * 30% of circulating supply => 15% of one token." Please add tests for:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only have a rudimentary understanding of the reasons behind pledges and FIP0081. My understanding is limited to this: We previously used equation A to calculate initial pledges, and now we're using 70% A + 30% B. If you want a more detailed explanation of the rules, we'll have to pull in one of the authors of FIP0081.
Done.
Done.
Already there.
Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The existing test is not exercising the pledge calculation properly, it's just testing a boundary condition where there is zero reward and supply. Please add a test for (ramp, duration) = (0, 0) and with reward/supply inputs similar to the other tests you added.
I don't see this test added. To be concrete, I'm looking to exercise where epochs_since_ramp start differs by zero and +/- 1 from the ramp duration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done.
Added a test with |
||
0, | ||
100, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1500).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
// On-ramp where 'baseline power' (85%). | ||
#[test] | ||
fn initial_pledge_on_ramp_mid() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
50, | ||
100, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1725).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
// Post-ramp where 'baseline power' has reduced effect (97%). | ||
#[test] | ||
fn initial_pledge_on_ramp_early() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
10, | ||
100, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1545).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
// Post-ramp, first epoch, pledge should be 97% 'baseline' + 3% simple. | ||
#[test] | ||
fn initial_pledge_on_ramp_step() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
1, | ||
10, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1545).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
// Post-ramp where 'baseline power' has reduced effect (70%). | ||
#[test] | ||
fn initial_pledge_post_ramp() { | ||
let initial_pledge = initial_pledge_for_power( | ||
&qa_sector_power(), | ||
&StoragePower::from(1u64 << 37), | ||
&reward_estimate(), | ||
&power_estimate(), | ||
&TokenAmount::from_whole(1), | ||
500, | ||
100, | ||
); | ||
assert_eq!( | ||
TokenAmount::from_atto(1) + TokenAmount::from_whole(1950).div_floor(10000), | ||
initial_pledge | ||
); | ||
} | ||
|
||
#[test] | ||
fn precommit_deposit_is_clamped_at_one_attofil() { | ||
let precommit_deposit = | ||
|
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.
nit: there's lots of other fixed point calculations floating around actors code so change this to something more precise either ONE_THOUSAND or GAMMA_FIXED_POINT_FACTOR
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.
Done.