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.

Sign up for GitHub

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

remove another potential abort in staking_pool #7571

Merged
merged 2 commits into from
Jan 23, 2023
Merged

remove another potential abort in staking_pool #7571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d3f6da
remove another potential abort in staking_pool
emmazzz Jan 22, 2023
8a5a7eb
switch order of processing delegation and withdraw
emmazzz Jan 23, 2023
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
12 changes: 6 additions & 6 deletions crates/sui-cost/tests/snapshots/empirical_transaction_cost__good_snapshot-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ expression: common_costs_estimate
---
{
"MergeCoin": {
"computation_cost": 7820,
"storage_cost": 11259,
"computation_cost": 7824,
"storage_cost": 11264,
"storage_rebate": 0
},
"Publish": {
"computation_cost": 8680,
"storage_cost": 12462,
"computation_cost": 8683,
"storage_cost": 12467,
"storage_rebate": 0
},
"SharedCounterAssertValue": {
Expand All @@ -29,8 +29,8 @@ expression: common_costs_estimate
"storage_rebate": 0
},
"SplitCoin": {
"computation_cost": 7798,
"storage_cost": 11226,
"computation_cost": 7802,
"storage_cost": 11232,
"storage_rebate": 0
},
"TransferPortionSuiCoin": {
Expand Down
5 changes: 5 additions & 0 deletions crates/sui-framework/docs/staking_pool.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<b>use</b> <a href="epoch_time_lock.md#0x2_epoch_time_lock">0x2::epoch_time_lock</a>;
<b>use</b> <a href="linked_table.md#0x2_linked_table">0x2::linked_table</a>;
<b>use</b> <a href="locked_coin.md#0x2_locked_coin">0x2::locked_coin</a>;
<b>use</b> <a href="math.md#0x2_math">0x2::math</a>;
<b>use</b> <a href="object.md#0x2_object">0x2::object</a>;
<b>use</b> <a href="sui.md#0x2_sui">0x2::sui</a>;
<b>use</b> <a href="table_vec.md#0x2_table_vec">0x2::table_vec</a>;
Expand Down Expand Up @@ -960,6 +961,10 @@ time in <code>request_withdraw_stake</code>.
<b>if</b> (total_sui_withdraw_amount &gt;= principal_withdraw_amount)
total_sui_withdraw_amount - principal_withdraw_amount
<b>else</b> 0;
// This may happen when we are withdrawing everything from the pool and
// the rewards pool <a href="balance.md#0x2_balance">balance</a> may be less than reward_withdraw_amount.
// TODO: FIGURE OUT EXACTLY WHY THIS CAN HAPPEN.
reward_withdraw_amount = <a href="math.md#0x2_math_min">math::min</a>(reward_withdraw_amount, <a href="balance.md#0x2_balance_value">balance::value</a>(&pool.rewards_pool));
<a href="balance.md#0x2_balance_decrease_supply">balance::decrease_supply</a>(
&<b>mut</b> pool.delegation_token_supply,
withdrawn_pool_tokens
Expand Down
3 changes: 2 additions & 1 deletion crates/sui-framework/docs/validator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,10 +732,11 @@ Process pending delegations and withdraws, called at the end of the epoch.


<pre><code><b>public</b>(<b>friend</b>) <b>fun</b> <a href="validator.md#0x2_validator_process_pending_delegations_and_withdraws">process_pending_delegations_and_withdraws</a>(self: &<b>mut</b> <a href="validator.md#0x2_validator_Validator">Validator</a>, ctx: &<b>mut</b> TxContext) {
<a href="staking_pool.md#0x2_staking_pool_process_pending_delegations">staking_pool::process_pending_delegations</a>(&<b>mut</b> self.delegation_staking_pool, ctx);
<b>let</b> reward_withdraw_amount = <a href="staking_pool.md#0x2_staking_pool_process_pending_delegation_withdraws">staking_pool::process_pending_delegation_withdraws</a>(
&<b>mut</b> self.delegation_staking_pool, ctx);
self.metadata.next_epoch_delegation = self.metadata.next_epoch_delegation - reward_withdraw_amount;
<a href="staking_pool.md#0x2_staking_pool_process_pending_delegations">staking_pool::process_pending_delegations</a>(&<b>mut</b> self.delegation_staking_pool, ctx);
// TODO: consider bringing this <b>assert</b> back when we are more confident.
// <b>assert</b>!(<a href="validator.md#0x2_validator_delegate_amount">delegate_amount</a>(self) == self.metadata.next_epoch_delegation, 0);
}
</code></pre>
Expand Down
5 changes: 5 additions & 0 deletions crates/sui-framework/sources/governance/staking_pool.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module sui::staking_pool {
use std::vector;
use sui::table_vec::{Self, TableVec};
use sui::linked_table::{Self, LinkedTable};
use sui::math;

friend sui::validator;
friend sui::validator_set;
Expand Down Expand Up @@ -352,6 +353,10 @@ module sui::staking_pool {
if (total_sui_withdraw_amount >= principal_withdraw_amount)
total_sui_withdraw_amount - principal_withdraw_amount
else 0;
// This may happen when we are withdrawing everything from the pool and
// the rewards pool balance may be less than reward_withdraw_amount.
// TODO: FIGURE OUT EXACTLY WHY THIS CAN HAPPEN.
reward_withdraw_amount = math::min(reward_withdraw_amount, balance::value(&pool.rewards_pool));
balance::decrease_supply(
&mut pool.delegation_token_supply,
withdrawn_pool_tokens
Expand Down
3 changes: 2 additions & 1 deletion crates/sui-framework/sources/governance/validator.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,11 @@ module sui::validator {

/// Process pending delegations and withdraws, called at the end of the epoch.
public(friend) fun process_pending_delegations_and_withdraws(self: &mut Validator, ctx: &mut TxContext) {
staking_pool::process_pending_delegations(&mut self.delegation_staking_pool, ctx);
let reward_withdraw_amount = staking_pool::process_pending_delegation_withdraws(
&mut self.delegation_staking_pool, ctx);
self.metadata.next_epoch_delegation = self.metadata.next_epoch_delegation - reward_withdraw_amount;
staking_pool::process_pending_delegations(&mut self.delegation_staking_pool, ctx);
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 want to add a TODO here so that one day we could add the assert back? Or is it never going to be guaranteed down the road?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add a TODO here. I think this assert does hold today but I don't want to risk having it here and failing advance_epoch for next_epoch_delegation which doesn't matter too much now.

// TODO: consider bringing this assert back when we are more confident.
// assert!(delegate_amount(self) == self.metadata.next_epoch_delegation, 0);
}

Expand Down