-
Notifications
You must be signed in to change notification settings - Fork 296
feat(jito): implements staking deactivate #6664
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
base: master
Are you sure you want to change the base?
Conversation
I accidentally closed the old PR when I renamed the branch. |
Done.
Created sub interface.
Added JSDoc. I kept the name the same because it's a sync version of |
6b9b7a2
to
2d1450e
Compare
2d1450e
to
817c917
Compare
txBuilder.sign({ key: wallet.prv }); | ||
txBuilder.sign({ key: stakeAccount.prv }); | ||
txBuilder.sign({ key: transferAuthority.prv }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transaction needs 3 signatures (see https://github.com/jito-foundation/jito-stake-unstake-reference/blob/master/hooks/useManualUnstake.ts or https://github.com/solana-program/stake-pool/blob/main/clients/js-legacy/src/index.ts).
I'm thinking of generating unstakingAddress
and transferAuthorityAddress
in wallet-platform.
stakePoolData: { | ||
managerFeeAccount: testData.JITO_STAKE_POOL_DATA_PARSED.managerFeeAccount.toString(), | ||
poolMint: testData.JITO_STAKE_POOL_DATA_PARSED.poolMint.toString(), | ||
validatorList: testData.JITO_STAKE_POOL_DATA_PARSED.validatorList.toString(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information needs to come from on chain. I'm thinking of parsing getAccountInfo
RPC call in staking-service
.
.stakingAddress(JITO_STAKE_POOL_ADDRESS) | ||
.unstakingAddress(stakeAccount.pub) | ||
.jitoParams({ | ||
validatorAddress: testData.JITO_STAKE_POOL_VALIDATOR_ADDRESS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator needs to be selected from data on chain. I'm thinking of parsing getAccountInfo
RPC call in staking-service
and implementing https://github.com/solana-program/stake-pool/blob/main/clients/js-legacy/src/utils/stake.ts.
try { | ||
let decodedInstruction: DecodedCloseAccountInstruction | undefined; | ||
if (instruction.programId.toString() !== TOKEN_2022_PROGRAM_ID.toString()) { | ||
decodedInstruction = decodeCloseAccountInstruction(instruction); | ||
} else { | ||
decodedInstruction = decodeCloseAccountInstruction(instruction, TOKEN_2022_PROGRAM_ID); | ||
} | ||
if (decodedInstruction && decodedInstruction.data.instruction === 9) { | ||
return 'CloseAssociatedTokenAccount'; | ||
} | ||
} catch (e) { | ||
// ignore error and continue to check for other instruction types | ||
const decodedInstruction = decodeInstruction(instruction, instruction.programId); | ||
const instructionTypeMap: Map<TokenInstruction, ValidInstructionTypes> = new Map(); | ||
instructionTypeMap.set(TokenInstruction.CloseAccount, 'CloseAssociatedTokenAccount'); | ||
instructionTypeMap.set(TokenInstruction.Burn, 'Burn'); | ||
instructionTypeMap.set(TokenInstruction.MintTo, 'MintTo'); | ||
instructionTypeMap.set(TokenInstruction.Approve, 'Approve'); | ||
instructionTypeMap.set(TokenInstruction.TransferChecked, 'TokenTransfer'); | ||
const validInstruction = instructionTypeMap.get(decodedInstruction.data.instruction); | ||
if (validInstruction === undefined) { | ||
throw new Error(`Unsupported token instruction type ${decodedInstruction.data.instruction}`); | ||
} | ||
|
||
// Check for burn instructions (instruction code 8) | ||
try { | ||
let burnInstruction: DecodedBurnInstruction; | ||
if (instruction.programId.toString() !== TOKEN_2022_PROGRAM_ID.toString()) { | ||
burnInstruction = decodeBurnInstruction(instruction); | ||
} else { | ||
burnInstruction = decodeBurnInstruction(instruction, TOKEN_2022_PROGRAM_ID); | ||
} | ||
if (burnInstruction && burnInstruction.data.instruction === 8) { | ||
return 'Burn'; | ||
} | ||
} catch (e) { | ||
// ignore error and continue to check for other instruction types | ||
} | ||
|
||
// Check for mint instructions (instruction code 7) | ||
try { | ||
let mintInstruction: DecodedMintToInstruction; | ||
if (instruction.programId.toString() !== TOKEN_2022_PROGRAM_ID.toString()) { | ||
mintInstruction = decodeMintToInstruction(instruction); | ||
} else { | ||
mintInstruction = decodeMintToInstruction(instruction, TOKEN_2022_PROGRAM_ID); | ||
} | ||
if (mintInstruction && mintInstruction.data.instruction === 7) { | ||
return 'MintTo'; | ||
} | ||
} catch (e) { | ||
// ignore error and continue to check for other instruction types | ||
} | ||
|
||
// Default to TokenTransfer for other token instructions | ||
return 'TokenTransfer'; | ||
return validInstruction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was refactored so that I could add the Approve
instruction easily.
2b1eed4
to
2af4aec
Compare
export interface StakingDeactivate { | ||
type: InstructionBuilderTypes.StakingDeactivate; | ||
params: { | ||
fromAddress: string; | ||
stakingAddress: string; | ||
amount?: string; | ||
unstakingAddress?: string; | ||
isMarinade?: boolean; | ||
stakingTypeParams: StakingDeactivateStakingTypeParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving stakingType out as an enum is cleaner, e.g.
enum StakingType {
NATIVE = 'NATIVE',
MARINADE = 'MARINADE',
JITO = 'JITO'
}
export interface StakingDeactivate {
type: InstructionBuilderTypes.StakingDeactivate;
params: {
fromAddress: string;
stakingAddress: string;
amount?: string;
stakingType: StakingType;
extraParams?: JitoStakePoolData | MarinadeParams | NativeParams;
Similarly for Activate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -124,14 +154,34 @@ export interface StakingDelegate { | |||
params: { stakingAddress: string; fromAddress: string; validator: string }; | |||
} | |||
|
|||
export interface JitoStakingDeactivateParams { | |||
type: 'JITO'; | |||
stakePoolData: WithdrawStakeStakePoolData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just expand required field here?
poolMint: string
validatorList: string
managerFeeAccount: string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do we need to pass validatorList here? Isn't the caller(staking service) already selecting the validator (validatorAddress below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validatorList
is the address of the account that stores the validator list data. It's derived from parsing the stake pool account data. We need the public key as part of the withdraw stake instruction.
Edit: and yes, validatorAddress
is selected by the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just expand required field here?
We could, but I think that would make it less clear that those fields come from parsing the stake pool account data.
With the current setup, you can do something like this:
const account = await connection.getAccountInfo("J1to.....")
const stakePoolData = StakePoolLayout.decode(account.data)
const txBuilder = new StakingDeactivateBulder()
txBuilder.extraParams({
stakePoolData, // notice we can pass the entire parsed object instead of having to select fields
validatorAddress: await getValidatorFrom(connection, stakePoolData.validatorList)
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just expand required field here?
If SOL has schema changes of those unused fields, the fields defined here might not be consistent and others might find it confusing in the future. But I am also okay with keeping them. Your call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validatorList is the address of the account that stores the validator list data
If validatorList is an account address, the name looks confusing. Could we have a more accurate name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the validatorList name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If SOL has schema changes of those unused fields, the fields defined here might not be consistent and others might find it confusing in the future. But I am also okay with keeping them. Your call
If this happens then we'll have to update the instruction encoder/decoder in jitoStakePoolOperations.ts
, i.e. the file that exports WithdrawStakeStakePoolData
. I think it won't be confusing because it's linked together by the compiler, so I will keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed validatorList name
const isMarinade = | ||
unstakingInstruction.deactivate === undefined && unstakingInstruction.withdrawStake === undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be cleaner, you can find out the stakingType upfront and use switch later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -6,6 +6,7 @@ import { | |||
DecodedMintToInstruction, | |||
decodeMintToInstruction, | |||
TOKEN_2022_PROGRAM_ID, | |||
decodeApproveInstruction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your problem, but this file is 1100+ line and is a nightmare to maintain. Create a followup ticket and refactor when you have more bandwidth
🚨 Critical Problems:
1. Massive monolithic functions - parseSendInstructions() is 180+ lines
2. Repeated code patterns - Nonce parsing duplicated 8+ times
3. Deep nested switches - Multiple levels of switch statements
4. Mixed responsibilities - Parsing, validation, and construction all mixed
5. Hard to maintain - Adding new instruction types requires touching multiple places
6. No abstraction - Each instruction type reimplements similar logic
⏺ 🎯 Specific Refactoring Opportunities:
1. Extract instruction parsers - Each case in switches should be a separate class
2. Common nonce handling - Extract to shared utility
3. Validation separation - Move validation to dedicated validators
4. Factory pattern - Use registry of instruction parsers
5. Eliminate duplicated token logic - Extract token name resolution
⏺ 💡 Proposed Refactoring Strategy:
Step 1: Create Instruction Parser Interface
interface InstructionParser<T extends InstructionParams> {
canParse(instruction: TransactionInstruction): boolean;
parse(instruction: TransactionInstruction, context?: ParsingContext): T;
}
Step 2: Extract Individual Parsers
// instructionParsers/NonceParser.ts
class NonceParser implements InstructionParser<Nonce> {
canParse(instruction: TransactionInstruction): boolean {
return getInstructionType(instruction) === ValidInstructionTypesEnum.AdvanceNonceAccount;
}
parse(instruction: TransactionInstruction): Nonce {
const decoded = SystemInstruction.decodeNonceAdvance(instruction);
return {
type: InstructionBuilderTypes.NonceAdvance,
params: {
walletNonceAddress: decoded.noncePubkey.toString(),
authWalletAddress: decoded.authorizedPubkey.toString(),
},
};
}
}
Step 3: Registry Pattern
class InstructionParserRegistry {
private parsers: InstructionParser<any>[] = [
new NonceParser(),
new TransferParser(),
new TokenTransferParser(),
// ... etc
];
parse(instruction: TransactionInstruction, context?: ParsingContext): InstructionParams {
const parser = this.parsers.find(p => p.canParse(instruction));
if (!parser) throw new NotSupported(`Unsupported instruction: ${getInstructionType(instruction)}`);
return parser.parse(instruction, context);
}
}
Step 4: Transaction Type Handlers
// transactionHandlers/SendTransactionHandler.ts
class SendTransactionHandler {
constructor(private registry: InstructionParserRegistry) {}
handle(instructions: TransactionInstruction[], context: ParsingContext): InstructionParams[] {
return instructions.map(instruction => this.registry.parse(instruction, context));
}
}
Step 5: Simplified Factory
export function instructionParamsFactory(
type: TransactionType,
instructions: TransactionInstruction[],
coinName?: string,
instructionMetadata?: InstructionParams[],
_useTokenAddressTokenName?: boolean
): InstructionParams[] {
const context = { coinName, instructionMetadata, _useTokenAddressTokenName };
const handler = TransactionHandlerRegistry.getHandler(type);
return handler.handle(instructions, context);
}
Benefits:
- Single Responsibility - Each parser handles one instruction type
- Easy to Test - Small, focused classes
- Extensible - Add new parsers without touching existing code
- Maintainable - Clear separation of concerns
- Reusable - Common parsers shared across transaction types
This would reduce the file from 1100+ lines to ~50 lines of registry code plus small focused parser classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10d64f6
to
4616483
Compare
4616483
to
f14b718
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the new comments for cleaner code
} else if ( | ||
stakingInstructions.delegate === undefined && | ||
stakingInstructions.depositSol === undefined && | ||
stakingInstructions.create !== undefined && | ||
stakingInstructions.initialize !== undefined | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These mixes of type infers and validation are fragile and hard to debug. Try to decouple it into a separate function, e.g.
function determineStakingType(instructions: StakingInstructions): StakingType {
if (instructions.depositSol) return StakingType.JITO;
if (!instructions.delegate && instructions.create && instructions.initialize) return StakingType.MARINADE;
if (instructions.delegate && instructions.create) return StakingType.NATIVE;
throw new Error('Unknown staking type');
}
// Then use switch statements instead of complex boolean logic
const stakingType = determineStakingType(stakingInstructions);
switch (stakingType) {
case StakingType.JITO: // handle JITO
case StakingType.MARINADE: // handle MARINADE
case StakingType.NATIVE: // handle NATIVE
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
One of the reasons I was using the if
statements was to determine which fields were not null. So my new solution uses type guards (e.g. isJitoStakingInstructions
) as a replacement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also StakingInstructions.authorize?: AuthorizeStakeParams[];
wasn't being used anywhere, so I removed that field.
f14b718
to
5325cd6
Compare
assert(isMarinade !== undefined, 'Missing isMarinade param'); | ||
assert(isJito !== undefined, 'Missing isJito param'); | ||
assert([isMarinade, isJito].filter((x) => x).length <= 1, 'At most one of isMarinade and isJito can be true'); | ||
assert(stakingType, 'Missing stakingType param'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I can remove it entirely because stakingType
cannot be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the check on this line.
I added a compile time check on the default case of the switch statements:
default: {
const unreachable: never = stakingType;
throw new Error(`Unknown staking type ${unreachable}`);
}
faafa6c
to
e215eae
Compare
BREAKING CHANGE: the `isMarinade` setter in transaction builders has been removed. Existing calls to `txBuilder.isMarinade` will have to be replaced with `txBuilder.stakingType(StakingType.MARINADE)`. Ticket: SC-2620
e215eae
to
5cc2722
Compare
Description
This PR adds support for Jito withdrawal. It also includes a refactor from
isMarinade
andisJito
flags to astakingTypeParams.type
enum.Issue Number
Ticket: SC-2418
Ticket: SC-2620
Type of change
Please delete options that are not relevant.
txBuilder.isMarinade
will have to be replaced withtxBuilder.stakingTypeParams(StakingType.MARINADE)
How Has This Been Tested?
Unit tests. See files changed.