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

[LUM-583] - Handle re-delegate assets from a disabled validator #29

Merged
merged 20 commits into from
Jun 29, 2023

Conversation

Ricardo-Remy
Copy link
Contributor

@Ricardo-Remy Ricardo-Remy commented May 31, 2023

Issue / Context

In the current implementation of cosmos millions there is no mechanism in place that allows to re-delegate assets from a misbehaving validator to other validators.
This PR introduces the ability to disable validator(s) by governance proposal and consequently re-delegate their assets to other active validators of the pool.

Implemented solution

  • Use of valSet diff in current UpdatePool handler to trigger redelegate of disabled validator(s) to active validators
  • Even split redelegation of bondedAmount from disabled to enabled validators
  • Unit tests with multiple validators setup

@Ricardo-Remy Ricardo-Remy marked this pull request as draft June 1, 2023 20:01
@Ricardo-Remy Ricardo-Remy changed the base branch from rc/v1.5 to master June 5, 2023 17:14
@Ricardo-Remy Ricardo-Remy changed the base branch from master to rc/v1.5 June 5, 2023 17:21
@Ricardo-Remy Ricardo-Remy marked this pull request as ready for review June 8, 2023 12:11
Copy link
Contributor

@lebascou lebascou left a comment

Choose a reason for hiding this comment

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

Implementation looks ok to me but I think we should keep things a bit more simple for now.
See my comments to be discussed (cc @Segfaultd).

x/millions/types/pool.go Outdated Show resolved Hide resolved
x/millions/keeper/callbacks_redelegate.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lebascou lebascou left a comment

Choose a reason for hiding this comment

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

Almost there but I made comments critical to the implementation regarding the bonded amount state we keep in the store.

proto/lum-network/millions/pool.proto Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/callbacks_redelegate.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lebascou lebascou left a comment

Choose a reason for hiding this comment

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

LGTM - a few cosmetic comments

x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Show resolved Hide resolved
x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
x/millions/types/pool.go Outdated Show resolved Hide resolved
…elegateToActiveValidators and helper functions
Copy link
Contributor

@lebascou lebascou left a comment

Choose a reason for hiding this comment

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

Still LGTM !
I spotted one potential critical mistake though.

x/millions/keeper/keeper_pool.go Outdated Show resolved Hide resolved
@lebascou lebascou added the ready to merge 👌 PR ready to be merged label Jun 28, 2023
* rc/v1.5: (58 commits)
  Fix consensus module registration
  Migrate consensus params and properly register keeper
  Add format for import ordering
  Add new NewMsgServerImpl for beam module
  Add RegisterMsgServer for RegisterServices in x/airdrop/module.go
  Add RegisterMsgServer for RegisterServices in x/millions/module.go
  Formatting
  Fixed ICA callbacks handlers registration
  Fix usage between port owner name and port ID
  Fixed ignite gov module config
  Add comment
  IBC Stack revamp
  Redundant implementation
  Install more dependencies
  Remove capabilities for ownership transfer
  Unused ClaimCapability anymore
  Use store key for consistence
  Move back proto to their original path for backward compatibility
  Revert "Fix codecs registration"
  Fix codecs registration
  ...
* rc/v1.5:
  Fix import ordering
  Update metadata for EditDeposit and ValidateBasic static check
  Fix linting errors
  Fix format check
  Add edit deposit feature
@lebascou lebascou merged commit 09f2a90 into rc/v1.5 Jun 29, 2023
5 checks passed
@lebascou lebascou deleted the feature/LUM-583-rc/v1.5 branch June 29, 2023 14:10
@lebascou lebascou mentioned this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge 👌 PR ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants