enable 1 sol minimum delegation#240
Conversation
2501babe
force-pushed
the
20260203_min1sol
branch
from
2870437 to
efbb998
Compare
2501babe
force-pushed
the
20260203_min1sol
branch
from
efbb998 to
5d5c366
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks great! This will be the easiest way to make the change on the program side.
Just some nits on the tests
program/tests/stake_instruction.rs
Outdated
| // 1: destination is fully funded: | ||
| // - old behavior: any split amount is OK | ||
| // - new behavior: split amount must be at least the minimum delegation | ||
| ( | ||
| rent_exempt_reserve + minimum_delegation, | ||
| 1, | ||
| expected_results[0].clone(), | ||
| Err(StakeError::InsufficientDelegation.into()), | ||
| ), | ||
| // if destination is only short by 1 lamport, then... | ||
| // 2: if destination is only short by 1 lamport, then... | ||
| // - old behavior: split amount can be 1 lamport | ||
| // - new behavior: split amount must be at least the minimum delegation | ||
| ( | ||
| rent_exempt_reserve + minimum_delegation - 1, | ||
| 1, | ||
| expected_results[1].clone(), | ||
| Err(StakeError::InsufficientDelegation.into()), | ||
| ), |
There was a problem hiding this comment.
I don't think we have success cases anymore, so what do you think about adding a test case for:
(
rent_exempt_reserve,
minimum_delegation,
Ok(()),
),
There was a problem hiding this comment.
4 is that triple, its easy to miss since its a one-liner. the array wasnt all possible successes, just things that succeeded with the old feature gate disabled and failed with it enabled
There was a problem hiding this comment.
Bah, sorry about that, you're right
program/tests/stake_instruction.rs
Outdated
| // - old behavior: any split amount is OK | ||
| // - new behavior: split amount must be at least the minimum delegation |
There was a problem hiding this comment.
nit: can you remove the "old behavior" comment?
There was a problem hiding this comment.
ef641c7 also i removed that saturating sub because even in the old case the comment was in error, minimum_delegation was 1, not 0
There was a problem hiding this comment.
Yeah that kind of confused me, thanks!
program/tests/stake_instruction.rs
Outdated
| // - old behavior: split amount can be 1 lamport | ||
| // - new behavior: split amount must be at least the minimum delegation |
There was a problem hiding this comment.
nit: same here, can you remove the "old behavior" comment?
program/tests/stake_instruction.rs
Outdated
| // - new behavior: split amount must be at least the minimum delegation | ||
| ( | ||
| rent_exempt_reserve + minimum_delegation, | ||
| 1, |
There was a problem hiding this comment.
nit: How about making this minimum_delegation - 1 to go with the comment? There's a bit of overlap with case 5, but this would be specific for the split amount
2 participants
this pr sets minimum delegation to 1 sol
my first version of this was >500 lines of test changes. i implemented a cargo feature flag, committed a 1lamp program binary, and testcased everything to run against both 1lamp and 1sol
however i realize this is all pointless. there will never be a moment where we are running this (presumably 5.0.0) release with a 1lamp minimum. so then i switched to testing everything against this and a tagged 4.0.0 binary. but this is also pointless: the test proof of 4.0.0 is tests passing at the 4.0.0 tag commit!
in other words this is all we have to do and im explaining why we dont have to do more in this repo. we will have to validate against agave and stake pools. but this is not a blocker on landing this pr, its a blocker on deploying a program with this commit in it. ill open a separate issue to track that work, it will constitute like 99% of what we actually have to do to ship this
closes #73