[Performance] Don't go into main loop if it is last period for vesting and unlock #141
Conversation
|
@sjoshisupra I think this is trickier than I expected since I need to write separate test cases to cover whether the remaining amount is divisible or not. I have the implementation passed all the test cases now. I will write additional test cases later. |
There was a problem hiding this comment.
If schedule is 2/10, 3/10, 1/10, then add two test cases
- Directly fast forward to a point from the beginning where 5/10 would be vested
- Directly fast forward to a case from the beginning where 7/10 would be vested
In both cases above that exactly needed amount is vested and nothing more.
| if ((x_raw as u256) % (y_raw as u256) != 0) { | ||
| result = result + 1; | ||
| }; |
There was a problem hiding this comment.
Why is this logic needed here? When you divide two fixed point values and want to return a fixed point value, no scaling is needed. I don't see any reason why if the remainder is non-zero, we should add 1 to the result.
There was a problem hiding this comment.
Also check that divisor is non zero before performing the division.
aptos-move/framework/supra-framework/sources/vesting_without_staking.move
Outdated
Show resolved
Hide resolved
|
|
||
| /// Returns x / y. The result cannot be greater than MAX_U128. | ||
| public fun divide(x: FixedPoint64, y: FixedPoint64): FixedPoint64 { | ||
| let x_raw = get_raw_value(x); | ||
| let y_raw = get_raw_value(y); | ||
| // If it is divisable, return the result. If not, return the result + 1. | ||
| let result = (x_raw as u256) / (y_raw as u256); | ||
| if ((x_raw as u256) % (y_raw as u256) != 0) { | ||
| result = result + 1; | ||
| }; | ||
| assert!(result <= MAX_U128, ERATIO_OUT_OF_RANGE); | ||
| create_from_raw_value((result as u128)) | ||
| } |
There was a problem hiding this comment.
I don't think this division is correct. If both numbers have 64 bit allotted for fractional value, after division, the fraction would disappear. I believe x_raw << 64 would be required before performing the division to ensure result still has 64 bit fractional bits.
There was a problem hiding this comment.
I think we don't even need it right now as we did not use this anymore. Do we need to keep this function?
…taking.move Co-authored-by: Saurabh Joshi <[email protected]>
Test cases are added. 5/10 needs an addition of 2 and 7/10 needs an addition of 3 for the assertions. This behavior is also consistent with dev branch. |
| contract_address) * 2); | ||
| vest(contract_address); | ||
| vested_amount = vested_amount + fraction(shareholder_share, 5, 10); | ||
| assert!(coin::balance<SupraCoin>(shareholder_address) + 2 == vested_amount, 0); |
There was a problem hiding this comment.
Why is this + 2 required in assert? @axiongsupra
There was a problem hiding this comment.
@sjoshisupra The actual vested value for each iteration is 199, 299. So in total would be 498 while the calculated vested amount is 500.
| contract_address) * 4); | ||
| vest(contract_address); | ||
| vested_amount = vested_amount + fraction(shareholder_share, 7, 10); | ||
| assert!(coin::balance<SupraCoin>(shareholder_address) + 3 == vested_amount, 0); |
There was a problem hiding this comment.
@axiongsupra why is this + 3 required here? Given that 2/10, 3/10, 1/10 can multiply perfectly with 1000.
There was a problem hiding this comment.
The actual vested value is 199, 299, 99, 99 so in total is 696 while the calculated vested amount is 699 in vested_amount = vested_amount + fraction(shareholder_share, 7, 10)
No description provided.