-
Notifications
You must be signed in to change notification settings - Fork 20
p-token: Add withdraw_excess_lamports
instruction
#36
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
Conversation
c81e695
to
6ee62a4
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.
Looks great! Just tiny nits that don't even need to be addressed here
|
||
use super::validate_owner; | ||
|
||
#[inline(always)] |
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.
Totally separate from this PR, but what's the impact of these inlines? I wonder if there's a chance that we're actually making things worse by adding this annotation everywhere.
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.
In some cases it helps a lot – e.g., for transfer
, inlining brings the CUs from 227
to 143
.
But for others there is no difference in terms of CU, e.g., withdraw_excess_lamports
, although it increases the program binary (+200
bytes).
I can revise all the inlines in a separate PR and only keep the ones that make a difference on the CUs. This way we can optimize both CU and program size.
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.
Ah interesting, I'm glad to see that it does help in some cases!
let mut withdraw_ix = spl_token_2022::instruction::withdraw_excess_lamports( | ||
&spl_token_2022::ID, | ||
&account_pubkey, | ||
&destination, | ||
&owner.pubkey(), | ||
&[], | ||
) | ||
.unwrap(); | ||
// Switches the program id to the token program. | ||
withdraw_ix.program_id = token_program; |
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.
We should definitely relax the logic in spl-token-2022 to allow for spl-token::id as well
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.
We should do this when p-token
is live, right?
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 think we can do it whenever, since it adoption of new library versions takes so long. But there's no rush either way
539d06b
to
43e8fce
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.
Just one last little comment for the assert_matches!
usage
Problem
As part of the migration to
p-token
, it will be proposed to add thewithdraw_excess_lamports
instruction – this is the same instruction that already exists in SPL Token-2022.Solution
This PR updates the
TokenInstruction
enum to add aWithdrawExcessLamports
variant with the same value as SPL Token-2022 and adds the processor implementation for the instruction.