-
Notifications
You must be signed in to change notification settings - Fork 50
p-token: Add custom entrypoint #85
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: febo/simplify-math
Are you sure you want to change the base?
Conversation
3d74a84
to
fafdd3e
Compare
2edf20b
to
9e62a06
Compare
fafdd3e
to
8a3de38
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.
No show-stoppers from my side! I didn't calculate all of the consts myself to check, but the overall logic looks good to me.
&& (*input.add(ACCOUNT1_DATA_LEN).cast::<u64>() == 165) | ||
&& (*input.add(ACCOUNT2_HEADER_OFFSET) == 255) | ||
&& (*input.add(ACCOUNT2_DATA_LEN).cast::<u64>() == 82) | ||
&& (*input.add(IX12_ACCOUNT3_HEADER_OFFSET) == 255) | ||
&& (*input.add(IX12_ACCOUNT3_DATA_LEN).cast::<u64>() == 165) | ||
&& (*input.add(IX12_ACCOUNT4_HEADER_OFFSET) == 255) |
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.
nit: let's use the consts for token account size / mint size / non-dup marker here
|
||
/// Offset for the second account. |
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.
/// Offset for the second account. | |
/// Offset for the third account. |
/// Offset for the fourth account data length. | ||
/// | ||
/// This is expected to be an account with variable data | ||
/// length. | ||
const IX12_ACCOUNT4_DATA_LEN: usize = 0x7b20; |
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 is only ever used once, and still called with align
-- why not just put the aligned value as the const?
// Check that we have enough instruction data. | ||
// | ||
// Expected: instruction discriminator (u8) + amount (u64) + decimals (u8) | ||
if input.add(offset).cast::<usize>().read() >= 10 { |
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.
nit: maybe use u64
here since size_of::<u64>()
is added right after?
if input.add(offset).cast::<usize>().read() >= 10 { | |
if input.add(offset).cast::<u64>().read() >= 10 { |
let discriminator = input.add(offset + size_of::<u64>()).cast::<u8>().read(); | ||
|
||
// Check for transfer discriminator. | ||
if likely(discriminator == 12) { |
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.
nit: can you use const here too?
/// Offset for the third account data length. This is | ||
/// expected to be a mint account (82 bytes). | ||
const IX3_ACCOUNT3_DATA_LEN: usize = 0x5268; |
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 with this, just align it from the start?
/// Expected offset for the instruction data in the case all | ||
/// previous accounts have zero data. |
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.
Shouldn't this only assume that the authority has zero data?
Problem
Currently
p-token
uses the "generic" entrypoint. There is an opportunity to give priority to most used instruction to further save CUs. Looking at token instructions from a period of one month, the most used instructions are:transfer_checked
36.33%
transfer
13.22%
close_account
12.23%
initialize_account3
9.98%
initialize_immutable_owner
9.78%
sync_native
4.53%
initialize_account
2.58%
Other instructions account for less than
1%
each.Solution
This PR adds a custom entrypoint to
p-token
that includes a "fast-path" to transfer instructions. It also modifies the current processor to give priority tosync_native
andinitialize_immutable_owner
instructions. Both modifications lead to significant improvements in CU consumption when considering the usage distribution.➡️ Credits to @cavemanloverboy for the "fast-path" approach.