-
Notifications
You must be signed in to change notification settings - Fork 20
p-token: Add batch
instruction
#35
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
e058e93
to
c12b54f
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 really good! I didn't think it would be so simple. Mostly small comments
let (discriminator, instruction_data) = instruction_data | ||
.split_first() | ||
.ok_or(ProgramError::InvalidInstructionData)?; | ||
let instruction = TokenInstruction::try_from(*discriminator)?; |
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.
Was the usage of TokenInstruction
supposed to be reverted?
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.
Yes, using the TokenInstruction
enum increases 20
CUs at least once I added the Batch
variant. I could not find a way to make it efficient.
/// - `0`: `InitializeMint` | ||
/// - `1`: `InitializeAccount` | ||
/// - `3`: `Transfer` | ||
/// - `7`: `MintTo` | ||
/// - `9`: `CloseAccount` | ||
/// - `16`: `InitializeAccount2` |
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 to make sure, were these updated for better performance as a result of this PR?
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.
Yes, the idea was to keep all the Initialize*
, MintTo
, CloseAccount
and Transfer
on the "faster" path. We can tweak the list if you thing there are other instructions that are more critical.
accounts: &[AccountInfo], | ||
instruction_data: &[u8], | ||
instruction: TokenInstruction, |
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.
Same here: was this supposed to be reverted?
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.
Yes, it is more efficient to use the u8
value directly.
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! Too bad about the enum
This PR adds the
batch
instruction forp-token
– this was originally proposed here.The purpose is to allow the execution of multiple token instructions in a single program instruction – e.g., it is possible to execute multiple instructions in a single CPI call, which saves at least
1000
CUs (CPI base fee) for each additional instruction.Note
SInce tests now run without
test-sbf
, this PR also updates thep-token
tests to remove thetest-sbf
feature.