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

[Aptos Framework][Vesting] Pay out the accumulated rewards relatively to vesting's remaining grant instead of staking contract's principal #6106

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

movekevin
Copy link
Contributor

@movekevin movekevin commented Jan 7, 2023

Description

Using the staking contract’s principal is indirect and the vesting contract can use it remaining grant amount as the base to get the actual amount of accumulated rewards so far. Here’s an example to make this clearer:

  1. Vesting contract has 100 APT and 1 staking participant (to keep it simple). 100 APT is also the remaining amount. The underlying staking contract has a principal of 100 APT.
  2. The stake pool earns 50 APT.
  3. Before calculating outstanding rewards that belong to the vesting pool, vesting::unlock_rewards first requests commission to be paid out to the operator, which is 5 APT (10% of 50 APT earned). Remaining total active stake is 145 APT, all belonging to the vesting pool.
  4. Since there’s been no tokens vested yet, the current vesting::unlock_rewards would calculate outstanding rewards to be 145 (total active in stake pool) - 100 (remaining_grant) = 45 to be distributed to staking contract participants.

Test Plan

Unit tests

// Request commission first to make it easier to calculate what's the accumulated rewards that belong to the
// vesting pool.
let operator = vesting_contract.staking.operator;
staking_contract::request_commission(contract_signer, contract_address, operator);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this since unlock_stake already does a request_commission internally? isn't the accumulated_rewards = total_active_stake - remaining_grant - commission_amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion, thanks! Definitely a simpler and less redundant computation. Updated.

@zekun000
Copy link
Contributor

zekun000 commented Jan 7, 2023

a completely orthogonal question, after some period of vesting, isn't it possible that the remaining grant drops below the minimal staking amount?

@movekevin movekevin force-pushed the vesting branch 2 times, most recently from 5d5d273 to e6b537b Compare January 7, 2023 10:04
@movekevin
Copy link
Contributor Author

a completely orthogonal question, after some period of vesting, isn't it possible that the remaining grant drops below the minimal staking amount?

Yes this is already a known potential issue in the far future. However, by then, we'll likely have gone through a couple more iterations and add some more features such as adding stake. Another alternative direction is to move the funds to another contract built on top of delegations.

… to vesting's remaining grant instead of staking contract's principal
let operator = vesting_contract.staking.operator;
let (total_active_stake, _, commission_amount) =
staking_contract::staking_contract_amounts(contract_address, operator);
// Accumulated rewards, excluding unpaid commission, that entirely belongs to the shareholders.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment could be clearer about the difference between this and the accumulated rewards returned from the staking_contract_amounts call

let commission_on_staker_rewards = (with_rewards(staker_rewards) - staker_rewards) / 10;
staker_rewards = with_rewards(staker_rewards) - commission_on_staker_rewards;
commission = with_rewards(commission) + commission_on_staker_rewards;
distribute(contract_address);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the expected reward for shareholder here? could we add an assert to make sure the calculation is correct?

@movekevin movekevin enabled auto-merge (rebase) January 12, 2023 12:51
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7490 TPS, 5202 ms latency, 7800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f
compatibility::simple-validator-upgrade::single-validator-upgrade : 4575 TPS, 9043 ms latency, 11000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f
compatibility::simple-validator-upgrade::half-validator-upgrade : 4641 TPS, 9023 ms latency, 11800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6622 TPS, 5949 ms latency, 11400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 9d791bc9f7d10c8c10ce0c18d8c03eeda6a70c4f

performance benchmark with full nodes : 6604 TPS, 6008 ms latency, 8700 ms p99 latency,(!) expired 80 out of 2820320 txns
Test Ok

@movekevin movekevin merged commit 05eabd7 into main Jan 12, 2023
@movekevin movekevin deleted the vesting branch January 12, 2023 13:39
@aptos-labs aptos-labs deleted a comment from Cho202 Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants