tests: Mollusk StakeLifecycle, Deactivate tests (2/9) #221
Conversation
rustopian
changed the title
rustopian
force-pushed
the
mollusk2-deactivate
branch
2 times, most recently
from
ac97793 to
80abfd2
Compare
rustopian
force-pushed
the
mollusk2-deactivate
branch
from
80abfd2 to
f6de449
Compare
rustopian
force-pushed
the
mollusk2-deactivate
branch
from
f6de449 to
1293590
Compare
| let stake = Pubkey::new_unique(); | ||
| let stake_account = AccountSharedData::new_data_with_space( | ||
| ctx.rent_exempt_reserve / 2, | ||
| &StakeStateV2::Uninitialized, | ||
| StakeStateV2::size_of(), | ||
| &id(), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
What if we had an API like:
let (addr, account) = ctx.stake_account()
.rent(ctx.rent_exempt_reserve / 2)
.build()| .vote_account | ||
| .as_ref() | ||
| .map(|(pk, _)| pk) | ||
| .expect("vote_account required for this lifecycle") |
There was a problem hiding this comment.
This sort of gives me a feeling that we aren't using the type system enough if it's possible to run a test with an incorrect setup state. Some ideas...
What about instead of choosing a lifecycle up front, you start uninitialized and transition via methods:
let stake = ctx
.stake() // -> StakeBuilder<Uninitialized>
.fund(amount)
.initialize(auth, lockup) // -> StakeBuilder<Initialized>
.delegate(vote) // requires vote here
.activate_epoch(); // -> StakeBuilder<Active>or have builders for the different lifecycles:
ctx.uninitialized_stake()
ctx.initialized_stake().lockup(...)
ctx.activating_stake(vote).staked_amount(...)
ctx.active_stake(vote).staked_amount(...)But in general I feel like we may be trying to serve all instruction case parameters at once in the current design.
| /// Verify that removing any signer from the instruction causes MissingRequiredSignature error | ||
| fn verify_all_signers_required( |
There was a problem hiding this comment.
Nice, this is cool that it runs by default
| }; | ||
|
|
||
| // Initialize if needed | ||
| if self >= StakeLifecycle::Initialized { |
There was a problem hiding this comment.
Will the >= comparisons sort of guarantee that a stake account goes through the entire lifecycle from the beginning? Hitting many state transitions along the way?
| Activating, | ||
| Active, | ||
| Deactivating, | ||
| Deactive, |
| /// | ||
| /// This function re-serializes the stake history sysvar from mollusk.sysvars.stake_history | ||
| /// every time it's called, ensuring that any updates to the stake history are reflected in the accounts. | ||
| pub fn add_sysvars( |
There was a problem hiding this comment.
I'm on the fence with this helper. Is it better to automatically populate needed accounts or have the test writer enumerate them at the site of the instruction builder? Hmm.
| /// Create a vote account with VoteStateV4 | ||
| pub fn create_vote_account() -> AccountSharedData { | ||
| let space = VoteStateV4::size_of(); | ||
| let lamports = Rent::default().minimum_balance(space); | ||
| let vote_state = VoteStateVersions::new_v4(VoteStateV4::default()); | ||
| let data = bincode::serialize(&vote_state).unwrap(); | ||
|
|
||
| Account::create(lamports, data, solana_sdk_ids::vote::id(), false, u64::MAX).into() | ||
| } |
There was a problem hiding this comment.
I kinda think it would help to put all account builders in a separate dir
|
|
||
| #[test_case(false; "activating")] | ||
| #[test_case(true; "active")] | ||
| fn test_deactivate(activate: bool) { |
There was a problem hiding this comment.
Not for this PR, but when we later migrate this instruction, I think it would help to revisit these tests and likely break them up into specific cases versus a single test with multiple inside.
| @@ -0,0 +1,128 @@ | |||
| #![allow(clippy::arithmetic_side_effects)] | |||
There was a problem hiding this comment.
Is this not covered via the parent mod.s allow?
| .execute( | ||
| DeactivateBuilder::new() | ||
| .stake(stake) | ||
| .stake_authority(staker) | ||
| .instruction(), |
There was a problem hiding this comment.
If we had per-instruction builders, we could essentially could pull all of the work passing ctx defaults into the instructions to be internal to the builder. If we did that, I think we could make individual tests like 50% smaller. Maybe I am still enamored by the token-wrap pattern of having tests like this small:
#[test]
fn test_incorrect_escrow_owner() {
let incorrect_escrow_owner = Pubkey::new_unique();
WrapBuilder::default()
.unwrapped_escrow_owner(incorrect_escrow_owner)
.check(Check::err(TokenWrapError::EscrowOwnerMismatch.into()))
.execute();
}2 participants
Part 2 of 9 in this project.
(diff is cumulative)
Problem
solana-program-test is on its slow way out.
Solution
Convert these tests to Mollusk, including simple but perfectly equivalent stake tracking.
This part migrates
Deactivatetests and updates to fullStakeLifecyclemanagement. It does not yet add stake tracking when warping across slots; that is added in the next PR.Included:
StakeLifecyclemanagementDeactivatetests moved from program_test.rs to deactivate.rsThis does NOT migrate the tests from the newer stake_instruction.rs, which already uses Mollusk.