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

Improve type safety in stake state module #2482

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

jstarry
Copy link

@jstarry jstarry commented Aug 8, 2024

Problem

Many stake state methods use catch-all match cases for remaining variants, so a new variant could be unintentionally unhandled.

Brought up by @behzadnouri in #2453 (comment)

Summary of Changes

Add explicit match cases for all stake state variants.

Fixes #

@jstarry jstarry requested a review from joncinque August 8, 2024 01:19
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

The change looks good to me! What do you think about taking it a step further and adding #![deny(clippy::wildcard_enum_match_arm)] at the top of the file to catch future occurrences?We don't do this anywhere else in the code, but it might make sense for sensitive modules like this one.

@jstarry
Copy link
Author

jstarry commented Aug 8, 2024

What do you think about taking it a step further and adding #![deny(clippy::wildcard_enum_match_arm)] at the top of the file to catch future occurrences?

Sounds like a great idea to me. We should probably use that more often in the codebase. I just updated the PR with that clippy config and an additional refactor to remove another instance of a wildcard arm

@jstarry
Copy link
Author

jstarry commented Aug 8, 2024

Needs a rebase on master

joncinque
joncinque previously approved these changes Aug 8, 2024
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jstarry jstarry force-pushed the refactor/stake-state-type-safety branch from d953435 to 2649c7b Compare August 8, 2024 23:59
@jstarry jstarry requested a review from joncinque August 9, 2024 00:00
@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label Aug 9, 2024
Copy link

mergify bot commented Aug 9, 2024

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge automerge Merge this Pull Request automatically once CI passes label Aug 9, 2024
@jstarry jstarry added the automerge automerge Merge this Pull Request automatically once CI passes label Aug 9, 2024
@mergify mergify bot merged commit 2792e91 into anza-xyz:master Aug 9, 2024
54 checks passed
@jstarry jstarry deleted the refactor/stake-state-type-safety branch August 9, 2024 01:40
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* improve type safety in stake state module

* feedback

* fix new method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants