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

Chore: explicitly reset vault rotator state on ceremony restarts #3907

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Currently when we restart from keygen or handover (as opposed to "aborting"), vault rotator states don't get fully reset. I don't think it is actually a problem since we always overwrite values before starting to use them, but as discussed with Dan, it is better to reset them explicitly the moment they become stale.

I renamed abort_vault_rotation to reset_vault_rotation since it we now use it in situations other than aborts, and removed the VaultRotationAborted to avoid extra noise (it was only added recently and seems redundant considering that we already have RotationAborted in the validator pallet).

This is because this function needs to be called any time we restart from
any rotation stage (e.g. keygen or handover), not just when we abort.
@msgmaxim msgmaxim force-pushed the feat/reset-vault-rotator branch 2 times, most recently from 9d3ea83 to 2e5a9bb Compare August 29, 2023 02:19
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #3907 (2d96fdf) into main (7911f33) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##            main   #3907   +/-   ##
=====================================
- Coverage     71%     71%   -0%     
=====================================
  Files        366     366           
  Lines      56762   56772   +10     
  Branches   56762   56772   +10     
=====================================
- Hits       40540   40533    -7     
- Misses     14213   14222    +9     
- Partials    2009    2017    +8     
Files Changed Coverage Δ
state-chain/traits/src/lib.rs 52% <ø> (ø)
state-chain/pallets/cf-validator/src/lib.rs 74% <100%> (+<1%) ⬆️
state-chain/pallets/cf-vaults/src/tests.rs 97% <100%> (ø)
state-chain/pallets/cf-vaults/src/vault_rotator.rs 73% <100%> (-<1%) ⬇️
...-chain/runtime/src/chainflip/all_vaults_rotator.rs 97% <100%> (ø)
state-chain/traits/src/mocks/vault_rotator.rs 94% <100%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@syan095 syan095 left a comment

Choose a reason for hiding this comment

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

LGTM

@dandanlen dandanlen merged commit 6c8d696 into main Aug 29, 2023
42 checks passed
@dandanlen dandanlen deleted the feat/reset-vault-rotator branch August 29, 2023 07:57
syan095 added a commit that referenced this pull request Aug 29, 2023
…on-integration

* origin/main:
  fix: is_qualified should be called for all checks (#3910)
  🍒 cherry-pick: `preinst` script updates (#3917)
  Chore: explicitly reset vault rotator state on ceremony restarts (#3907)
  feat: shorter protocol id (#3906)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants