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

Minor fix and refactoring to staking #3279

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Minor fix and refactoring to staking #3279

merged 4 commits into from
Dec 4, 2024

Conversation

NingLin-P
Copy link
Member

The first commit fixes a minor issue by moving the epoch reward of the unlocked operator to the treasury instead of dropping silently.

The rest of the commits are refactoring to remove unnecessary logic and cleanup the code.

Code contributor checklist:

…r to the treasury

Signed-off-by: linning <linningde25@gmail.com>
This field is used to temporarily hold the reward and then move to the current_total_stake,
this commit directly move the reward to current_total_stake

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
By using idiomatic Entry and remove the useless weight_balance_cache as there is no repeat
opertator in the operator_weights BTreeMap

Signed-off-by: linning <linningde25@gmail.com>
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

@NingLin-P NingLin-P added this pull request to the merge queue Dec 4, 2024
@@ -141,6 +150,10 @@ pub(crate) fn operator_take_reward_tax_and_stake<T: Config>(
})?;
}

if !to_treasury.is_zero() {
mint_into_treasury::<T>(to_treasury).ok_or(TransitionError::MintBalance)?;
Copy link
Member

Choose a reason for hiding this comment

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

can't we just make this function handle the edge case instead. Its easy to miss otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

Done here #3285

crates/pallet-domains/src/staking.rs Show resolved Hide resolved
let share_price = SharePrice::new::<T>(total_shares, total_stake);

// calculate and subtract total withdrew shares from previous epoch
let (total_stake, total_shares) = if !operator.withdrawals_in_epoch.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

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

I remeber @nazar-pc mentioning about preferring immutable vars over mutable one. easy to reason with IMHO

Copy link
Member Author

Choose a reason for hiding this comment

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

I like immutable vars most of the time, but I don't think it is suitable for this particular case.

Merged via the queue into main with commit d4f51d4 Dec 4, 2024
8 checks passed
@NingLin-P NingLin-P deleted the refactor-staking branch December 4, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Subspace execution refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants