Rb staking pallet validator commission change event

[rossbulat]

This is a PR for #10802.

Update:

• Updated event to ValidatorPrefsUpdated to account for blocked and commission.

---

• A CommissionChanged event has been added to the staking pallet, which is deposited upon a successful validate() call.
• A commission_changed_event_works test has been added, that tests whether the correct event was deposited upon a new validate() call by controller account 10 / stash account 11.

Notes:
src/pallet/mod.rs line 987
prefs originally changed ownership when passed to do_add_validator. To keep the convention of calling an event after all updates, prefs was cloned for do_add_validator and consumed by the event deposit.

[kianenigma]

LGTM except the test being a bit superfluous.

[joepetrowski]

I don't really see the point of this. do_add_validate just inserts the validator into storage after all checks have passed, but the function args are never modified in any way (as can happen with e.g. bond if you try to bond more than your free balance).

This event is completely redundant because you can already look for the staking.validate extrinsic, check the system.Extrinsic{Success, Failure} event, and (if success), log the provided origin and prefs from the extrinsic data. As in, the function signature is this event.

[rossbulat]

This event is completely redundant because you can already look for the staking.validate extrinsic, check the system.Extrinsic{Success, Failure} event, and (if success), log the provided origin and prefs from the extrinsic data. As in, the function signature is this event.

The origin is the controller account though, not the validator node itself.
This was added for apps to easily detect changes in commission. We want something like

if(event.section == "staking" && event.method == "ValidatorPrefsUpdated") {
          const [validator_id, prefs] = event.data;
          ...
}

It would also be good for the node to not spew out every single extrinsic event to every single connection, but rather allow front end apps to choose which events they wish to receive, thus saving bandwidth and overall processing time.

I am not sure if work has been done on this, like giving your provider a list of events you want it to report. I can't see anything mentioned in Polkadot JS API docs.

[joepetrowski]

Exactly, there are currently no other options for subscribing to events. We have to subscribe like this.

This is just not at all true. You are assuming that there's one way to build an application - subscribe to events from a full node - and that the application has only one purpose - displaying events to a user. The back end to an application like 1kv doesn't need millisecond latency to display this to a user (it's making decisions on 6 or 24 hour timeframes). It could easily fetch each block and check for validate calls (or even just loop through the state of all ValidatorPrefs and see if any have changed since the last block.

But not just specific to this application, if information can be derived from the function signature and its success/failure, then emitting an event is redundant and wasteful.

[kianenigma]

Historically, I've been much more on @joepetrowski's side on this discussion that applications should try and decode the chain as much as possible, not the chain bending itself and spitting bite-size pieces of data for all sorts of applications.

It is true that you can easily scan blocks (both new and historical), and know exactly what commission changes happened by looking at extrinsics.

In fact, looking at the block transactions and their success status is better, because you are slightly less dependent on old state.

FYI, this is what @joepetrowski and I are proposing: https://polkadot.js.org/docs/api/cookbook/blocks#how-do-i-determine-if-an-extrinsic-succeededfailed

[rossbulat]

Yes, as things stand I am leaning towards this now too. Thanks for the useful insights.

[shawntabrizi]

@joepetrowski while you generally accurate that there is another (much more convoluted way to track this), it does not mean that having an event is not the right choice here.

Events are automatically indexed by most block explorers, and easy to subscribe to via most client libraries. The goal of events is to present information to user applications which are relevant, and I think that one of the fundamental events of the staking pallet is when a validator updates their commission.

So I really don't see the harm here in adding this event, and the overall story we want to tell here (connected to 1kv program), this would be the expected way for things to exist.

[joepetrowski]

Events are automatically indexed by most block explorers

So are extrinsics 🤷‍♂️

On one extreme, events (in general) are completely unnecessary because the runtime is stored on-chain, so if you want to know how a storage item got set, you can just sync the chain executing in Wasm and see where the storage item changed. Of course, that's a bit tricky and requires lots of tooling. Events are really convenient.

So on the other extreme, you could just emit an event for every state change. But we don't because that convenience does come at a price.

I think so far we've taken the approach that we should add events for things that are non-trivial to calculate. For example, with bond/bond_extra possibly bonding fewer tokens than specified, or with reserve_repatriated being used to transfer tokens from an identity requester to a registrar. These two examples also involve tracking something from A to B (tokens free to locked, or account to account).

This event is just telling you that some storage item was put, and sets a really bad pattern IMO. In this case, you already have an event (ExtrinsicSuccess) that tells you exactly the same thing: If it corresponds to a validate call, then this input arg was put. It's just as well to read the Validators storage item at every block height and check the stashes that interest you.

Its name is also misleading, as it doesn't account for new validators, who haven't updated their preferences but have merely set them as part of their intention to validate. In order to make it accurate, you either need to (a) put this in an else statement with if !Validators::<T>::contains_key(stash) and add a ValidatorPrefsSet event in the if, and/or (b) the application needs to index who is already a validator and what their prefs are (i.e. someone calling validate with the same prefs shouldn't raise any flags in 1kv). So I still don't see how this gets an application out of indexing.

Likewise, the function for chilling a stash just tells you when a stash is chilled, but doesn't tell you if it was a nominator or validator (or what their Prefs) were, so even with this event your application still needs to index who is a validator to match on stash here.

if chilled_as_validator || chilled_as_nominator {
	Self::deposit_event(Event::<T>::Chilled(stash.clone()));
}

[joepetrowski]

I really don't see the harm here in adding this event

I also don't see the harm of adding this on a practical level. validate probably gets called a handful of times per day, it's not going to affect the chain's performance at all. I just think it's mostly a rush response to a somewhat lively discussion in chat about validators changing their preferences and I think the off-chain solution is really easy in this case.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[shawntabrizi]

we are gonna follow up on this, talked to joe

[joepetrowski]

Would change the name to reflect the info it conveys, but otherwise OK.

[rossbulat]

Those name changes have been committed.

[kianenigma]

@rossbulat can you make this build so we can merge it?

[rossbulat]

No problem @kianenigma . All checks have passed.

[kianenigma]

bot merge