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

Refactors the OnStakingUpdate trait #2839

Closed
wants to merge 4 commits into from

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Jan 2, 2024

This PR refactors the OnStakingUpdate trait to make its usage simpler and less error prone in the context of pallet-staking and the pallet-stake-tracker. The main goal is to make readily available more information/data required by the OnStakingUpdate listener (e.g. new stake on on_stake_update) through the interface methods.

With this refactor, the event emitter needs to explicitly call the events with more information about the event. The advantage is that this new interface design prevents potential issues with data sync, e.g. the event emitter does not necessarily need to update the staking state before emitting the events and the OnStakingUpdate implementor does not need to rely as much on the staking interface, making the interface less error prone.

Examples

Currently, on the OnStakingUpdate implemetor, we have

impl OnStakingUpdate<AccountId, Balance> for X {

  // snip..
 
  // the implementor will most likely need to fetch the nominations of the new nominator.
  fn on_nominator_add(_who: &AccountId) {

    // if the event emitter did not update the nominations at this point, the data is out of sync.
    let nominations = stakingInterface::get_nominations(); 
  }
}

with the new interface design, the implementor will have all the most important information implicitly in the method interface:

impl OnStakingUpdate<AccountId, Balance> for X {

  // snip..

  fn on_nominator_add(_who: &AccountId, nominations: Vec<AccountId>) {
    // no need to call into staking interface or other storage to fetch the nominations of the new nominator.
  }
}

Although the new design is less error prone, it is more opinionated and the design is highly affected by how the staking pallet will use this interface. Evidently, it is not possible to cover all the inputs that all the OnStakingUpdate implementors may need (depending on the use case), but the refactor aims at striking a good balance between safety for the usage in the context of staking and generality.

@gpestana gpestana added the T2-pallets This PR/Issue is related to a particular pallet. label Jan 2, 2024
@gpestana gpestana requested a review from Ank4n January 2, 2024 17:40
@gpestana gpestana self-assigned this Jan 2, 2024
@gpestana gpestana requested review from a team January 2, 2024 17:40
@gpestana gpestana marked this pull request as draft January 2, 2024 17:40
@gpestana
Copy link
Contributor Author

gpestana commented Jan 2, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Jan 2, 2024

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4828478 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-50c88eec-097c-4945-878f-b18099a2ab05 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jan 2, 2024

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4828478 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4828478/artifacts/download.

@gpestana gpestana marked this pull request as ready for review January 2, 2024 23:26
@gpestana gpestana changed the title Refactors OnStakingUpdate Refactors the OnStakingUpdate trait Jan 2, 2024
Copy link

@rossbulat rossbulat left a comment

Choose a reason for hiding this comment

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

LGTM, useful additions.

@@ -123,15 +133,15 @@ pub trait OnStakingUpdate<AccountId, Balance> {
/// Fired when someone sets their intention to validate.
///
/// Note validator preference changes are not communicated, but could be added if needed.
fn on_validator_add(_who: &AccountId) {}
fn on_validator_add(_who: &AccountId, _self_stake: Option<Stake<Balance>>) {}

/// Fired when an existing validator updates their preferences.
///
/// Note validator preference changes are not communicated, but could be added if needed.
fn on_validator_update(_who: &AccountId) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this also need old and new self stake params?

fn on_stake_update(_who: &AccountId, _prev_stake: Option<Stake<Balance>>) {}
fn on_stake_update(
_who: &AccountId,
_prev_stake: Option<Stake<Balance>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have both _prev_stake and _stake as same type? Either Stake<Balance> or Option<Stake<Balance>>. If Stake<Balance>, we can pass a zero value.

@gpestana
Copy link
Contributor Author

Closing as these changes have been included in the stake tracker PR #1933 and only make sense in that context.

@gpestana gpestana closed this Jan 15, 2024
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* fix on-demand parachain relay behavior during target chain reorgs

* fix compilation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants