Skip to content
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

[Performance] Don't go into main loop if it is last period for vesting and unlock #141

Merged
merged 16 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions aptos-move/framework/aptos-stdlib/sources/fixed_point64.move
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ module aptos_std::fixed_point64 {
ensures result.value == x.value + y.value;
}

/// 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;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check that divisor is non zero before performing the division.

assert!(result <= MAX_U128, ERATIO_OUT_OF_RANGE);
create_from_raw_value((result as u128))
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't even need it right now as we did not use this anymore. Do we need to keep this function?


/// Multiply a u128 integer by a fixed-point number, truncating any
/// fractional part of the product. This will abort if the product
/// overflows.
Expand Down Expand Up @@ -85,6 +98,17 @@ module aptos_std::fixed_point64 {
(val * multiplier.value) >> 64
}

public fun multiply_u128_return_fixpoint64(val: u128, multiplier: FixedPoint64): FixedPoint64 {
// The product of two 128 bit values has 256 bits, so perform the
// multiplication with u256 types and keep the full 256 bit product
// to avoid losing accuracy.
let unscaled_product = (val as u256) * (multiplier.value as u256);
// Check whether the value is too large.
assert!(unscaled_product <= MAX_U128, EMULTIPLICATION);
create_from_raw_value((unscaled_product as u128))
}


/// Divide a u128 integer by a fixed-point number, truncating any
/// fractional part of the quotient. This will abort if the divisor
/// is zero or if the quotient overflows.
Expand Down
10 changes: 10 additions & 0 deletions aptos-move/framework/move-stdlib/sources/fixed_point32.move
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ module std::fixed_point32 {
(val * multiplier.value) >> 32
}

public fun multiply_u64_return_fixpoint32(val: u64, multiplier: FixedPoint32): FixedPoint32 {
// The product of two 64 bit values has 128 bits, so perform the
// multiplication with u128 types and keep the full 128 bit product
// to avoid losing accuracy.
let unscaled_product = (val as u128) * (multiplier.value as u128);
// Check whether the value is too large.
assert!(unscaled_product <= MAX_U64, EMULTIPLICATION);
create_from_raw_value((unscaled_product as u64))
}

/// Divide a u64 integer by a fixed-point number, truncating any
/// fractional part of the quotient. This will abort if the divisor
/// is zero or if the quotient overflows.
Expand Down
180 changes: 167 additions & 13 deletions aptos-move/framework/supra-framework/sources/pbo_delegation_pool.move
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,6 @@ module supra_framework::pbo_delegation_pool {
use supra_framework::staking_config;
use supra_framework::timestamp;
use supra_framework::multisig_account;
#[test_only]
use aptos_std::debug;

const MODULE_SALT: vector<u8> = b"supra_framework::pbo_delegation_pool";

Expand Down Expand Up @@ -1774,24 +1772,22 @@ module supra_framework::pbo_delegation_pool {
let last_unlocked_period = unlock_schedule.last_unlock_period;
let schedule_length = vector::length(&unlock_schedule.schedule);
let cfraction = unlock_schedule.cumulative_unlocked_fraction;
while (last_unlocked_period < unlock_periods_passed
&& fixed_point64::less(cfraction, one)) {
let next_fraction =
if (schedule_length <= last_unlocked_period) {
*vector::borrow(&unlock_schedule.schedule, schedule_length - 1)
} else {
*vector::borrow(&unlock_schedule.schedule, last_unlocked_period)
};
while (last_unlocked_period < unlock_periods_passed && fixed_point64::less(cfraction, one)
&& last_unlocked_period < schedule_length) {
let next_fraction = *vector::borrow(&unlock_schedule.schedule, last_unlocked_period);
cfraction = fixed_point64::add(cfraction, next_fraction);

last_unlocked_period = last_unlocked_period + 1;
};

if (last_unlocked_period < unlock_periods_passed && fixed_point64::less(cfraction, one)) {
let final_fraction= *vector::borrow(&unlock_schedule.schedule, schedule_length - 1);
// Acclerate calculation to current period and don't update last_unlocked_period since it is not used anymore
cfraction = fixed_point64::add(cfraction, fixed_point64::multiply_u128_return_fixpoint64((unlock_periods_passed - last_unlocked_period as u128), final_fraction));
cfraction = fixed_point64::min(cfraction, one);
};
unlock_schedule.cumulative_unlocked_fraction = cfraction;
unlock_schedule.last_unlock_period = unlock_periods_passed;
let unlockable_amount = cached_unlockable_balance(delegator_addr, pool_address);
amount <= unlockable_amount

}

/// Unlock `amount` from the active + pending_active stake of `delegator` or
Expand Down Expand Up @@ -9313,4 +9309,162 @@ module supra_framework::pbo_delegation_pool {
);
assert!(unlock_coin, 20);
}

#[test(supra_framework = @supra_framework, validator = @0x123, delegator = @0x010)]
// Testing whether fast forward is working as expected
public entry fun test_unlocking_principle_stake_success_can_fastforward(
supra_framework: &signer, validator: &signer, delegator: &signer
) acquires DelegationPoolOwnership, DelegationPool, GovernanceRecords, BeneficiaryForOperator, NextCommissionPercentage {
initialize_for_test(supra_framework);
account::create_account_for_test(signer::address_of(validator));
let delegator_address = signer::address_of(delegator);
let delegator_address_vec = vector[delegator_address];
let principle_stake = vector[100 * ONE_SUPRA];
let coin = stake::mint_coins(100 * ONE_SUPRA);
let principle_lockup_time = 7776000; // 3 month cliff
let multisig = generate_multisig_account(validator, vector[@0x12134], 2);

initialize_test_validator(
validator,
0,
true,
true,
0,
delegator_address_vec,
principle_stake,
coin,
option::some(multisig),
vector[2, 3, 1],
10,
principle_lockup_time,
LOCKUP_CYCLE_SECONDS // monthly unlocking
);
let validator_address = signer::address_of(validator);
let pool_address = get_owned_pool_address(validator_address);

// after 2 month unlock reward
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// 3 month
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// after 4 months
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// It's acceptable to round off 9 because this coin will remain locked and won't be transferred anywhere.
let unlock_coin =
can_principle_unlock(
delegator_address,
pool_address,
(20 * ONE_SUPRA) - 9
);
assert!(unlock_coin, 11);

// after 5 months
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
let unlock_coin =
can_principle_unlock(
delegator_address,
pool_address,
(50 * ONE_SUPRA) - 9
);
assert!(unlock_coin, 12);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);

// after 11 months
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
let unlock_coin =
can_principle_unlock(delegator_address, pool_address, 100 * ONE_SUPRA);
assert!(unlock_coin, 18);
}

#[test(supra_framework = @supra_framework, validator = @0x123, delegator = @0x010)]
// Testing whether fast forward is working as expected
public entry fun test_unlocking_principle_stake_success_can_fastforward_nondivisable(
supra_framework: &signer, validator: &signer, delegator: &signer
) acquires DelegationPoolOwnership, DelegationPool, GovernanceRecords, BeneficiaryForOperator, NextCommissionPercentage {
initialize_for_test(supra_framework);
account::create_account_for_test(signer::address_of(validator));
let delegator_address = signer::address_of(delegator);
let delegator_address_vec = vector[delegator_address];
let principle_stake = vector[113 * ONE_SUPRA];
let coin = stake::mint_coins(113 * ONE_SUPRA);
let principle_lockup_time = 7776000; // 3 month cliff
let multisig = generate_multisig_account(validator, vector[@0x12134], 2);

initialize_test_validator(
validator,
0,
true,
true,
0,
delegator_address_vec,
principle_stake,
coin,
option::some(multisig),
vector[2, 3, 1],
10,
principle_lockup_time,
LOCKUP_CYCLE_SECONDS // monthly unlocking
);
let validator_address = signer::address_of(validator);
let pool_address = get_owned_pool_address(validator_address);

// after 2 month unlock reward
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// 3 month
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// after 4 months
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();

// After three mounth cliff and one extra mouth, 2/10 of the principle stake (113) = 22.6 can be unlocked. minus 9 for rounding off.
let unlock_coin =
can_principle_unlock(
delegator_address,
pool_address,
(22 * ONE_SUPRA) - 9
);
assert!(unlock_coin, 11);

// after 5 months, 5/10 of the principle stake (113) = 56.5 can be unlocked. minus 9 for rounding off.
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
let unlock_coin =
can_principle_unlock(
delegator_address,
pool_address,
(55 * ONE_SUPRA) - 9
);
assert!(unlock_coin, 12);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);

// after 11 months
timestamp::fast_forward_seconds(LOCKUP_CYCLE_SECONDS);
end_aptos_epoch();
let unlock_coin =
can_principle_unlock(delegator_address, pool_address, 113 * ONE_SUPRA);
assert!(unlock_coin, 18);
}
}
Loading
Loading