Add tests for the vesting update procedure #17297
Conversation
|
There are still more test variants I could add (explicitly simulating the vesting times of accounts, for instance) but it's otherwise complete. I wanted to have this up for review and perhaps merging before I kept going. |
| @@ -67,7 +67,7 @@ module As_record = struct | |||
| ; vesting_period : 'slot_span | |||
| ; vesting_increment : 'amount | |||
| } | |||
| [@@deriving equal, hlist, fields, annot] | |||
| [@@deriving equal, hlist, fields, annot, sexp_of] | |||
There was a problem hiding this comment.
Could we use yojson instead of sexp?
EDIT: looks like this is enforced by Janestreet's quickcheck. I've ran into this in other parts of our code, so unfortunately, no.
| that won't take ages to finish *) | ||
| let gen_small_timing = | ||
| (* At 90s slot time, this slot will occur 375 years after slot 0 *) | ||
| let vesting_end_max = UInt32.(div max_int (of_int 32)) in |
There was a problem hiding this comment.
Why 32, is this just a random constant to shift upper boundary to a period human care while making it proportional to UInt32.max_int?
There was a problem hiding this comment.
I should document that. Yes, I just chose some small-ish value that would result in a vesting schedule that's comfortably within the guarantees provided by the MIP.
| let final_vesting_slot (t : Account_timing.as_record) = | ||
| (* timing_final_vesting_slot assumes vesting_increment is non-zero, but we | ||
| want to be able to handle such accounts here. *) | ||
| if Amount.(equal t.vesting_increment zero) then t.cliff_time |
There was a problem hiding this comment.
Could we move this logic inside Account.timing_final_vesting_slot so it deals with corner case as well?
There was a problem hiding this comment.
Probably. I can move it in this PR.
There was a problem hiding this comment.
Not crucial, though. we could do it in subsequent PRs if needed.
There was a problem hiding this comment.
Sure, might be better not to complicate this one
| hardfork_slot | ||
| + of_int 2 | ||
| * ( t.vesting_period | ||
| - ((hardfork_slot - t.cliff_time) mod t.vesting_period) )) |
There was a problem hiding this comment.
Okay, this is calculating the time difference of the closest vesting slot to the HF slot. Double the span and add back the HF slot to get the new slot of closest vesting. Make sense.
| then Some (timing, hardfork_slot, vesting_end) | ||
| else None ) | ||
| in | ||
| let total_vesting_slots = |
There was a problem hiding this comment.
This is a span, not number of slots it seems? if it's number of slot we need to add 1.
There was a problem hiding this comment.
It's the number of slots, but the account isn't considered actively vesting at vesting_end, so this should be the length of the interval [hardfork_slot, vesting_end).
The new tests ensure that the vesting parameter update equations in the slot reduction MIP preserve the guarantees included in the MIP, namely that the minimum balance at the hardfork slot itself is unchanged, and funds unlock at the same system times as they would have had the hardfork not occurred.
cjjdespres
force-pushed
the
cjjdespres/vesting-schedule-test
branch
from
8be8a9c to
23549d7
Compare
|
!ci-build-me |
|
I think that should be everything, other than a few items for follow-up PRs. |
Explain your changes:
Tests have been added to ensure that the vesting parameter update equations in the slot reduction MIP preserve the guarantees included in the MIP, namely that the minimum balance at the hardfork slot itself is unchanged, and funds unlock at the same system times as they would have had the hardfork not occurred.
Explain how you tested your changes:
I ran the tests manually and confirmed that they all passed.
Checklist:
(not applicable, I think)