Adds unchecked calls to system program #130
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,131 @@ | ||
| #![no_std] | ||
| use core::usize; | ||
|
|
||
| use pinocchio::{ | ||
| account_info::AccountInfo, | ||
| cpi, | ||
| instruction::{AccountMeta, Instruction, Signer}, | ||
| pubkey::Pubkey, | ||
| }; | ||
|
|
||
| pub mod instructions; | ||
|
|
||
| pinocchio_pubkey::declare_id!("11111111111111111111111111111111"); | ||
|
|
||
| pub struct InvokeParts<'a, const ACCOUNTS: usize, Data> { | ||
| pub program_id: Pubkey, | ||
| pub accounts: [&'a AccountInfo; ACCOUNTS], | ||
| pub account_metas: [AccountMeta<'a>; ACCOUNTS], | ||
| pub instruction_data: Data, | ||
| } | ||
|
|
||
| pub struct FullInstructionData<const N: usize> { | ||
| data: [u8; N], | ||
| } | ||
|
|
||
| impl<const N: usize> FullInstructionData<N> { | ||
| pub fn new(data: [u8; N]) -> Self { | ||
| Self { data } | ||
| } | ||
| } | ||
|
|
||
| pub struct TruncatedInstructionData<const N: usize> { | ||
| data: [u8; N], | ||
| size: usize, | ||
| } | ||
|
|
||
| impl<const N: usize> TruncatedInstructionData<N> { | ||
| pub fn new(data: [u8; N], size: usize) -> Self { | ||
| Self { data, size } | ||
| } | ||
| } | ||
|
|
||
| pub struct SliceInstructionData<'a> { | ||
| data: &'a [u8], | ||
| } | ||
|
|
||
| impl<'a> SliceInstructionData<'a> { | ||
| pub fn new(data: &'a [u8]) -> Self { | ||
| Self { data } | ||
| } | ||
| } | ||
|
|
||
| pub trait InstructionData { | ||
| fn as_slice(&self) -> &[u8]; | ||
| fn len(&self) -> usize; | ||
| } | ||
|
|
||
| impl<const N: usize> InstructionData for FullInstructionData<N> { | ||
| fn as_slice(&self) -> &[u8] { | ||
| &self.data | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| N | ||
| } | ||
| } | ||
|
|
||
| impl<const N: usize> InstructionData for TruncatedInstructionData<N> { | ||
| fn as_slice(&self) -> &[u8] { | ||
| &self.data[..self.size] | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| self.size | ||
| } | ||
| } | ||
|
|
||
| impl<'a> InstructionData for SliceInstructionData<'a> { | ||
| fn as_slice(&self) -> &[u8] { | ||
| self.data | ||
| } | ||
|
|
||
| fn len(&self) -> usize { | ||
| self.data.len() | ||
| } | ||
| } | ||
|
|
||
| pub trait Invoke<'a, const ACCOUNTS: usize, Data>: Into<InvokeParts<'a, ACCOUNTS, Data>> | ||
| where | ||
| Data: InstructionData, | ||
| { | ||
| fn invoke(self) -> pinocchio::ProgramResult { | ||
| self.invoke_signed(&[]) | ||
| } | ||
|
|
||
| fn invoke_signed(self, signers: &[Signer]) -> pinocchio::ProgramResult { | ||
| invoke_invoker(self.into(), signers, |ix, acc, sigs| { | ||
| cpi::invoke_signed(&ix, acc, sigs) | ||
| }) | ||
| } | ||
|
|
||
| unsafe fn invoke_access_unchecked(self) -> pinocchio::ProgramResult { | ||
| self.invoke_singed_access_unchecked(&[]) | ||
| } | ||
|
|
||
| unsafe fn invoke_singed_access_unchecked(self, signers: &[Signer]) -> pinocchio::ProgramResult { | ||
| invoke_invoker(self.into(), signers, |ix, acc, sigs| unsafe { | ||
| cpi::invoke_signed_access_unchecked(&ix, acc, sigs) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl<'a, const ACCOUNTS: usize, T, Data> Invoke<'a, ACCOUNTS, Data> for T | ||
| where | ||
| T: Into<InvokeParts<'a, ACCOUNTS, Data>>, | ||
| Data: InstructionData, | ||
|
Comment on lines
+136
to
+139
Contributor
There was a problem hiding this comment. These kinds of blanket implementations are considered bad for extensibility, are they not? Similarly, the inheritance from Invoke<'a, const ACCOUNTS: usize, Data>: Into<InvokeParts<'a, ACCOUNTS, Data>>I am not sure that I fully understand the proposed change end to end, but looking at the usage site, it does not seem to be doing anything more than a simple trait implementation would not do: impl<'a> From<AdvanceNonceAccount<'a>>
for InvokeParts<'a, N_ACCOUNTS, FullInstructionData<DATA_LEN>>
{
fn from(value: AdvanceNonceAccount<'a>) -> Self {Whey is the above better than trait<'accounts> Invoke<'accounts, const N_ACCOUNTS: usize> {
type InstructionData: InstructionData;
fn accounts(&self) -> Self::InstructionData;
/* ... */
}
impl<'accounts> Invoke<'accounts, N_ACCOUNTS> for AdvanceNonceAccount<'a>
{
type InstructionData: FullInstructionData<DATA_LEN>;
fn accounts(&self) -> Self::InstructionData {There might be some minor changes required. Or am I missing something?
Author
There was a problem hiding this comment. So initially I thought of having the invoke functions implemented under the // probably worth creating a `IntoInvokeParts` trait instead of using `Into/From`
Transfer<'a> {....}.into_parts().invoke()
// vs
Transfer<'a> {....}.invoke()You raise a valid point about the blanket The Now why use
This isn't inheritance but a bound on the trait, it's saying that in order to implement
Contributor
There was a problem hiding this comment.
If you implement a trait, you can call this trait methods directly.
I think the opposite is better.
If there is a duplication, it can be extracted into a shared function that does not depend on the parameters that produce the unnecessary duplication. Also,
If it is not necessary, there is no need to include it, no?
Author
There was a problem hiding this comment. Let's take a look at the transfer instruction and the current invoke call: pub struct Transfer<'a> {
pub from: &'a AccountInfo,
pub to: &'a AccountInfo,
pub lamports: u64,
}
impl Transfer<'_> {
pub fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult {
// NOTE: the number of account is always static and doesn't change.
let account_metas: [AccountMeta; 2] = [
AccountMeta::writable_signer(self.from.key()),
AccountMeta::writable(self.to.key()),
];
// NOTE: the data is constructed as a byte array but due to `no_std`
// we cant use box/vec so we statically size the array
let mut instruction_data = [0; 12];
instruction_data[0] = 2;
instruction_data[4..12].copy_from_slice(&self.lamports.to_le_bytes());
// NOTE: the sdk already provides a partial `InvokeParts` but the data is borrowed
let instruction = Instruction {
program_id: &crate::ID,
accounts: &account_metas,
data: &instruction_data,
};
invoke_signed(&instruction, &[self.from, self.to], signers)
}
}Now let's construct the pub trait Invoke {
const ACCOUNTS: usize;
type Data:: InstructionData;
// NOTE: here you're just defining the `InvokeParts` struct but using functions
// NOTE: this isn't defining behavior but pseudo getters
fn program_id(&self) -> Pubkey;
fn accounts<'a>(&'a self) -> [&'a AccountInfo; ACCOUNTS];
fn account_metas(&self) -> [AccountMeta<'a>; Self::ACCOUNTS];
fn instruction_data(&self) -> Self::Data;
// NOTE: here you are calling function that take `&self` and in particular `x.accounts()` which
// returns referenced data meaning that the likelihood of rust being able optimize out copies of data
// is reduced potentially increasing CU count. By using the intermediate `InvokeParts` you move data
// which allows rust to optimize out copies and better structure the data layout
fn invoke_signed(self, signers: &[Signer]) -> pinocchio::ProgramResult {
let account_metas = self.account_metas();
let instruction_data = self.data();
let accounts = self.accounts();
let instruction = Instruction {
program_id: &self.program_id(),
accounts: &account_metas,
data: instruction_data.as_slice(),
};
invoke_signed(&instruction, &accounts, signers)
}
}Ultimately this comes down to preference. From my experience with rust providing function based views via traits tend to cause less flexibility and its usually better to create an intermediate struct, its also a common pattern used by many crates. If the use of generics is the issue. As stated before we could add a // NOTE: this ensures one impl per type
pub trait IntoInvokeParts<'a> {
const ACCOUNTS: usize;
type Data:: InstructionData;
fn into_parts(self) -> InvokeParts<'a, Self::ACCOUNTS, Self::Data>;
}
#[sealed]
pub trait Invoke { .... }
impl Invoke for T where T: IntoInvokeParts { .... }
Contributor
There was a problem hiding this comment.
Why is it less flexible? I think it is better to use a callback mechanism, if we want to get better optimization. You are already using a callback in the pub trait CanInvoke {
const ACCOUNTS_LEN: usize;
fn invoke_via<F>(&self, invoke: F) -> ProgramResult
where
invoke: for<'invoke> FnOnce(
/* program_id: */ &'invoke Pubkey,
/* accounts: */ &'invoke [&'invoke AccountInfo; Self::ACCOUNTS_LEN],
/* account_metas: */ &'invoke[AccountMeta<'invoke>; Self::ACCOUNTS_LEN],
/* data: */ &'invoke [u8],
)
;
}
pub struct Transfer<'a> {
pub from: &'a AccountInfo,
pub to: &'a AccountInfo,
pub lamports: u64,
}
impl CanInvoke for Transfer<'_> {
const ACCOUNTS_LEN: usize = 2;
pub fn invoke_via<F>(&self, invoke: F) -> ProgramResult
where
invoke: for<'invoke> FnOnce(
/* program_id: */ &'invoke Pubkey,
/* accounts: */ &'invoke [&'invoke AccountInfo; Self::ACCOUNTS_LEN],
/* metas: */ &'invoke[AccountMeta<'invoke>; Self::ACCOUNTS_LEN],
/* data: */ &'invoke [u8],
)
{
let metas: [AccountMeta; 2] = [
AccountMeta::writable_signer(self.from.key()),
AccountMeta::writable(self.to.key()),
];
let mut data = [0; 12];
data[0] = 2;
data[4..12].copy_from_slice(&self.lamports.to_le_bytes());
invoke(&crate::ID, &[self.from, self.to], &metas, &data)
}
}I do not see why the data needs to be generalized either. The
Author
There was a problem hiding this comment. The From my point of view
This also comes from a point of view of the type being public and part of the SDK or for a
You don't need a an explicit type to invoke, you can just construct a Though you are correct there is more code but in the end the rust compiler is better able to identify intermediate data structures(Zero-Cost Abstractions) and optimize them away creating smaller binary sizes.
Contributor
There was a problem hiding this comment.
It provides exactly the same values as what you put in
I can see why you might be seeing it this way. [...]
I do not understand this argument.
This does not make it less flexible.
I do not think it would be a good idea to connect these things.
This is, unfortunately, not the case, at least for the SBF target. I've talked to Lucas about this a bit more outside the PR discussion. But Pinocchio is designed as an SDK that is trying to avoid any overhead.
Author
There was a problem hiding this comment. I agree this is mostly subjective, I'm happy to open a new PR exploring the simplified I don't know how I feel about writing code that is tied to |
||
| { | ||
| } | ||
|
|
||
| fn invoke_invoker<'a, const ACCOUNTS: usize>( | ||
| invoke_parts: InvokeParts<'a, ACCOUNTS, impl InstructionData>, | ||
| signers: &[Signer], | ||
| invoker: impl FnOnce(Instruction, &[&AccountInfo; ACCOUNTS], &[Signer]) -> pinocchio::ProgramResult, | ||
| ) -> pinocchio::ProgramResult { | ||
| let instruction = Instruction { | ||
| program_id: &invoke_parts.program_id, | ||
| accounts: &invoke_parts.account_metas, | ||
| data: invoke_parts.instruction_data.as_slice(), | ||
| }; | ||
| invoker(instruction, &invoke_parts.accounts, signers) | ||
| } | ||
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 don't think this should live in the SDK -- perhaps a
pinocchio-client-helperscrate?But for now let's just copy the trait + types into the other clients and see if it generalizes 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'll need at least one more
InstructionDatafor slices