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

fix: keyholder check should use HistoricalActiveEpochs #5325

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

dandanlen
Copy link
Collaborator

Pull Request

Closes: PRO-1721

Checklist

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

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

This is more reliable than using HistoricalAuthorities because the latter is not cleared when an epoch expires.

Preivously is was possible for a validator to be considered active for an expired epoch if the epochs were expired out-of-order (see updated test case).

This is more reliable than using HistoricalAuthorities because the latter is not cleared when an epoch expires.

Preivously is was possible for a validator to be considered active for an expired epoch if the epochs were expired out-of-order (see updated test case).
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (ab7104b) to head (53afa64).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
state-chain/pallets/cf-funding/src/lib.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5325    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        488     488            
  Lines      84898   84766   -132     
  Branches   84898   84766   -132     
======================================
- Hits       60376   60191   -185     
- Misses     21822   21874    +52     
- Partials    2700    2701     +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Janislav Janislav left a comment

Choose a reason for hiding this comment

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

Much clearer 👍

@dandanlen dandanlen added this pull request to the merge queue Oct 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024
@dandanlen dandanlen added this pull request to the merge queue Oct 11, 2024
Merged via the queue into main with commit ddb52f8 Oct 11, 2024
49 checks passed
@dandanlen dandanlen deleted the fix/simpler-keyholder-check branch October 11, 2024 08:25
syan095 added a commit that referenced this pull request Oct 14, 2024
…lana-ccm

* origin/main:
  ci: upgrade action version to supress deprecation warnings ⚙️ (#5330)
  feat: handle rotation tx construction failures (#5307)
  test(bouncer): add test for new utility (#5324)
  fix: keyholder check should use `HistoricalActiveEpochs` (#5325)
  feat: BTC contract swap encoding (#5311)
dandanlen added a commit that referenced this pull request Oct 30, 2024
This is more reliable than using HistoricalAuthorities because the latter is not cleared when an epoch expires.

Preivously is was possible for a validator to be considered active for an expired epoch if the epochs were expired out-of-order (see updated test case).
dandanlen added a commit that referenced this pull request Oct 31, 2024
This is more reliable than using HistoricalAuthorities because the latter is not cleared when an epoch expires.

Preivously is was possible for a validator to be considered active for an expired epoch if the epochs were expired out-of-order (see updated test case).
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.

2 participants