Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Add test to check destination for stake splitting#23303

Merged
brooksprumo merged 5 commits intosolana-labs:masterfrom
brooksprumo:minimum-stake/test-split
Feb 24, 2022
Merged

Add test to check destination for stake splitting#23303
brooksprumo merged 5 commits intosolana-labs:masterfrom
brooksprumo:minimum-stake/test-split

Conversation

@brooksprumo
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo commented Feb 23, 2022 •
edited
Loading

Problem

There was missing test coverage checking the destination account balance when splitting stake accounts.

Summary of Changes

Add the test 😊

@brooksprumo brooksprumo added the CI Pull Request is ready to enter CI label Feb 23, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 23, 2022
@CriesofCarrots CriesofCarrots mentioned this pull request Feb 23, 2022
Merged
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2022 •
edited
Loading

Codecov Report

Merging #23303 (495c70c) into master (c81dd60) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head 495c70c differs from pull request most recent head b3b0700. Consider uploading reports for the commit b3b0700 to get more accurate results

@@            Coverage Diff            @@
##           master   #23303     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         571      571             
  Lines      155738   155793     +55     
=========================================
- Hits       126746   126725     -21     
- Misses      28992    29068     +76

@brooksprumo brooksprumo marked this pull request as ready for review February 23, 2022 18:05
@brooksprumo brooksprumo force-pushed the minimum-stake/test-split branch 2 times, most recently from 454ebb3 to d0283b5 Compare February 23, 2022 18:11
@brooksprumo brooksprumo force-pushed the minimum-stake/test-split branch from d0283b5 to 495c70c Compare February 23, 2022 18:19
@brooksprumo brooksprumo mentioned this pull request Feb 23, 2022
Open
18 tasks
joncinque
joncinque previously approved these changes Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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 good to me!

Comment thread programs/stake/src/stake_state.rs Outdated
Comment thread programs/stake/src/stake_state.rs Outdated
CriesofCarrots
CriesofCarrots reviewed Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Agree with Jon's comments; otherwise lgtm

@mergify mergify Bot dismissed joncinque’s stale review February 24, 2022 00:30

Pull request has been modified.

CriesofCarrots
CriesofCarrots previously approved these changes Feb 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Sorry, one last tedious nit from me... can you spell out "dest" everywhere, please?
Then, r+!

@mergify mergify Bot dismissed CriesofCarrots’s stale review February 24, 2022 00:47

Pull request has been modified.

joncinque
joncinque approved these changes Feb 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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 good!

@brooksprumo brooksprumo merged commit fcaf01e into solana-labs:master Feb 24, 2022
@brooksprumo brooksprumo deleted the minimum-stake/test-split branch February 24, 2022 04:48
@brooksprumo
Copy link
Copy Markdown
Contributor Author

brooksprumo commented Feb 24, 2022

Thanks for the help, @joncinque @CriesofCarrots! These tests picked up an issue with my other PR where my refactor (previously) allowed splits with an amount of 0 lamports (which is not the current behavior). I appreciate your attention to detail 🙏

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022
@brooksprumo @jeffwashington
Add test to check destination for stake splitting (solana-labs#23303)
a348cb7
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@joncinque joncinque joncinque approved these changes

@CriesofCarrots CriesofCarrots CriesofCarrots left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brooksprumo @joncinque @CriesofCarrots @solana-grimes