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 stake redelegate #13358

Merged
merged 2 commits into from
Nov 6, 2020
Merged

Conversation

t-nelson
Copy link
Contributor

@t-nelson t-nelson commented Nov 3, 2020

Problem

Re-delegating a formerly delegated, but currently deactivated stake account, does not consider changes to the account balance

Summary of Changes

Use the current account balance rather than the previous delegation amount

WIP until activation is coordinated with #13357

@t-nelson
Copy link
Contributor Author

t-nelson commented Nov 3, 2020

Do you have time to take a peek at one more, @rwalker-com? 🙏

Copy link
Contributor

@rwalker-com rwalker-com left a comment

Choose a reason for hiding this comment

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

great job closing a hole you can throw a cat through

CriesofCarrots
CriesofCarrots previously approved these changes Nov 3, 2020
Copy link
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.

I'll take another look when you push your test changes, but fix lgtm!
It'll be good to get another winky comment in the code base as well ;)

@mergify mergify bot dismissed CriesofCarrots’s stale review November 3, 2020 05:10

Pull request has been modified.

@t-nelson
Copy link
Contributor Author

t-nelson commented Nov 3, 2020

Review's been addressed!

@t-nelson t-nelson added v1.3 work in progress This isn't quite right yet labels Nov 3, 2020
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #13358 (87d661f) into master (d08c323) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #13358   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         378      378           
  Lines       89990    90037   +47     
=======================================
+ Hits        73862    73914   +52     
+ Misses      16128    16123    -5     

@CriesofCarrots
Copy link
Contributor

This will need rebasing on #13394 (make sure it goes in stake_state.rs, as opposed to the legacy mod)

@t-nelson
Copy link
Contributor Author

t-nelson commented Nov 6, 2020

Rebased on #13394

@t-nelson t-nelson added automerge Merge this Pull Request automatically once CI passes and removed work in progress This isn't quite right yet labels Nov 6, 2020
@mergify mergify bot merged commit fe1e08b into solana-labs:master Nov 6, 2020
mergify bot added a commit that referenced this pull request Nov 6, 2020
* stake: Add redelegation failing test

(cherry picked from commit 491ad59)

* stake: Consider withdraws we redelegating

(cherry picked from commit fe1e08b)

Co-authored-by: Trent Nelson <trent@solana.com>
mergify bot added a commit that referenced this pull request Nov 6, 2020
* stake: Add redelegation failing test

(cherry picked from commit 491ad59)

* stake: Consider withdraws we redelegating

(cherry picked from commit fe1e08b)

Co-authored-by: Trent Nelson <trent@solana.com>
@t-nelson t-nelson deleted the fix-stake-redelegate branch November 6, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants