fix withdraw underflow #20104
fix withdraw underflow #20104
Conversation
longbowlu
requested review from
amnn,
lxfind,
damirka,
emmazzz,
manolisliolios and
mystenmark
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
| @@ -371,8 +375,15 @@ module sui_system::staking_pool { | |||
| /// Called at epoch boundaries to process pending stake withdraws requested during the epoch. | |||
| /// Also called immediately upon withdrawal if the pool is inactive. | |||
| fun process_pending_stake_withdraw(pool: &mut StakingPool) { | |||
| pool.sui_balance = pool.sui_balance - pool.pending_total_sui_withdraw; | |||
| pool.pool_token_balance = pool.pool_token_balance - pool.pending_pool_token_withdraw; | |||
| pool.sui_balance = | |||
There was a problem hiding this comment.
How can you end up in a scenario where the pending_total_sui_withdraw is actually more than the balance?
There was a problem hiding this comment.
check test_process_pending_stake_withdraw_no_underflow_in_safe_mode and line 661-666
There was a problem hiding this comment.
I see, my more precise question is why is this even possible (and why do we accept it as being possible instead of figuring out how to prevent it in the first place?). Accepting it is fine by me, I mainly try to understand why we do.
crates/sui-framework/packages/sui-system/sources/staking_pool.move
Outdated
Show resolved
Hide resolved
| @@ -698,6 +698,13 @@ module sui_system::sui_system { | |||
| self.set_epoch_for_testing(epoch_num) | |||
| } | |||
|
|
|||
| // NEVER remove `#[test_only]` | |||
| #[test_only] | |||
| public(package) fun increment_epoch_for_testing(wrapper: &mut SuiSystemState) { | |||
There was a problem hiding this comment.
I think you can reuse the function above set_epoch_for_testing.
There was a problem hiding this comment.
i used this and decided with @emmazzz that this may be helpful when writing advancing-epoch tests. But you are right we could make the test work without this. I can remove it if you prefer
| #[test_only] | ||
| public(package) fun set_epoch_for_testing(self: &mut SuiSystemStateInnerV2, epoch_num: u64) { | ||
| self.epoch = epoch_num | ||
| } | ||
|
|
||
| // NEVER remove `#[test_only]` | ||
| #[test_only] | ||
| public(package) fun increment_epoch_for_testing(self: &mut SuiSystemStateInnerV2) { |
There was a problem hiding this comment.
ditto, are you sure you need increment if you have set_...?
crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move
Outdated
Show resolved
Hide resolved
crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move
Outdated
Show resolved
Hide resolved
crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move
Outdated
Show resolved
Hide resolved
longbowlu
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
longbowlu
force-pushed
the
fix-withdraw-underflow
branch
from
10c3378 to
0b817b1
Compare
longbowlu
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.