[staking] use object id to identify a staking pool instead of validator address #8371
Conversation
emmazzz
requested review from
lxfind,
tzakian,
sblackshear,
damirka and
longbowlu
emmazzz
requested review from
randall-Mysten and
ronny-mysten
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
emmazzz
force-pushed
the
check_pool_id
branch
from
787a07c to
f4e6bcc
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
f4e6bcc to
7e6712e
Compare
There was a problem hiding this comment.
Overall looks good! Just the one question around the removal of starting_epoch in the StakingPool, and possibly a new test is needed to make sure the staking_pool_mapping is updated correctly is needed before landing.
Not sure what the process is if this is a breaking change but I'm sure you know what it is if there is one, so please land responsibly :)
| validator_address: address, | ||
| /// The epoch at which this pool started operating. Should be the epoch at which the validator became active. | ||
| starting_epoch: u64, | ||
| struct StakingPool has key, store { |
There was a problem hiding this comment.
Is there a reason for the starting_epoch to also be removed here? It seems like something we'd want to keep in but I'm probably misunderstanding something :) Was the tuple of (address, u64) being used as some kind of UID for the staking pool before this?
There was a problem hiding this comment.
It does look like the starting_epoch needs to be kept because wallet / explorer depends on it to calculate APY. I'm adding it back.
Was the tuple of (address, u64) being used as some kind of UID for the staking pool before this?
yup
| @@ -193,7 +193,6 @@ module sui::delegation_tests { | |||
| assert!(vector::length(&staked_sui_ids) == 1, 0); | |||
| let staked_sui = test_scenario::take_from_sender_by_id(scenario, *vector::borrow(&staked_sui_ids, 0)); | |||
| assert!(staking_pool::staked_sui_amount(&staked_sui) == 50, 0); | |||
| assert!(staking_pool::validator_address(&staked_sui) == VALIDATOR_ADDR_2, 0); | |||
There was a problem hiding this comment.
Nit: can/should this assertion be replaced with a check that the validator address in the staking_pool_mappings in the ValidatorSet object is equal to VALIDATOR_ADDR_2?
More generally, are there any tests that should have the additional check about the correctness of the staking_pool_mapping? If not it may be worthwhile writing one to make sure that it stays in-synch.
There was a problem hiding this comment.
Absolutely. I'm working on testing now.
emmazzz
force-pushed
the
check_pool_id
branch
from
7e6712e to
80bb103
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
80bb103 to
a8def39
Compare
| ({ fields }) => | ||
| fields.delegation_staking_pool.fields.validator_address === | ||
| validatorAddress | ||
| ({ fields }) => fields.delegation_staking_pool.fields.id.id === pool_id |
There was a problem hiding this comment.
@Jibz1 does this look okay?
There was a problem hiding this comment.
Can we derive the validator meta data from the staking pool id?
There was a problem hiding this comment.
I added a Table staking_pool_mappings storing mappings from staking pool id to validator address so I think the answer is yes. It uses dynamic fields though.
There was a problem hiding this comment.
I see there are a few other places where validator_address is being used so I'm keeping it in the staked sui for now. We can work on removing these usages.
emmazzz
force-pushed
the
check_pool_id
branch
from
a8def39 to
485f97f
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
485f97f to
195ef37
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
195ef37 to
241745e
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
241745e to
a883908
Compare
emmazzz
force-pushed
the
check_pool_id
branch
from
a883908 to
a63449a
Compare
Since we may make validator account pubkey rotatable and thus validator address changeable, it makes sense to use staking pool's object id to identify a staking pool instead.