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

refactor: simplify logics in FP set rotation #188

Merged
merged 12 commits into from
Oct 16, 2024

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Oct 14, 2024

Resolves #145
Resolves #191 (fuzzed locally and it's not flaky anymore)

This PR simplifies the logics in FP set rotation, including:

  • Moving event emitting to msg handlers whereever possible (mostly for jailing/unjailing).
  • Removing the handling of special case len(events) == 0. This is not necessary and can be handled altogether in ProcessAllPowerDistUpdateEvents with same complexity.
  • Splitting recordVotingPowerAndCache into two functions, one for recording voting power table/cache and the other for emitting events/hooks for Fp state update.
  • Some minor abstraction for separating event emitting and voting power rotation algorithm.

Will do a second round of refactoring in another PR, together with #72

@SebastianElvis SebastianElvis marked this pull request as ready for review October 15, 2024 10:06
@SebastianElvis SebastianElvis requested a review from a team as a code owner October 15, 2024 10:06
@SebastianElvis SebastianElvis requested review from Lazar955, samricotta, gitferry, KonradStaniec and a team and removed request for a team October 15, 2024 10:06
Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Lgtm! @jrwbabylonlab fyi, seems we will change slashing status change to a tx event

x/btcstaking/keeper/finality_providers.go Outdated Show resolved Hide resolved
@SebastianElvis
Copy link
Member Author

fyi, seems we will change slashing status change to a tx event

Oh is having FP state change as block event mandatory for BE?

@gitferry
Copy link
Member

Oh is having FP state change as block event mandatory for BE?

I don't think it's mandatory but let's hear from @jrwbabylonlab

Copy link
Member

@Lazar955 Lazar955 left a comment

Choose a reason for hiding this comment

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

lgtm

@KonradStaniec
Copy link
Collaborator

Oh is having FP state change as block event mandatory for BE?

Not really mandatory, but property we wanted to preserve is that one type of event - EventFinalityProviderStatusChange is always the same type. As we cannot make all status changes transaction events, we decided to make them block events.

@SebastianElvis
Copy link
Member Author

Alright, for the sake of simplifying BE development I moved back the slashed/jailed FP events back, but added some helper functions for emitting these events. In addition, I tightened the interfaces exposed in power_dist_change.go. Now this file has <500 LoCs, only contains code for voting power update, and only exposes 3 public functions.

@SebastianElvis SebastianElvis merged commit 35610ce into main Oct 16, 2024
20 checks passed
@SebastianElvis SebastianElvis deleted the refactor-voting-power-dist branch October 16, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants