Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Baseline bug fix (FIP-0081) #1557
Changes from 15 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
There are no files selected for viewing
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.
Please remove this TODO. Zero is a reasonable value for networks other than mainnet.
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.
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.
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.
The uses of max and min are clever, but maybe too much so? I'm finding it too hard to mentally verify the expected behaviour around cases like negative epochs_since_start and zero ramp duration. Tests will help with confidence here, but also an if/match construction could take out some of those edge cases clearly.
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.
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 way is definitely easier to follow thanks
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 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Added a test with
epochs_since_ramp={-1,0,1}
.