-
Notifications
You must be signed in to change notification settings - Fork 146
Adds CanInvoke
to sys program
#212
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: main
Are you sure you want to change the base?
Adds CanInvoke
to sys program
#212
Conversation
programs/system/src/lib.rs
Outdated
pub mod instructions; | ||
|
||
pinocchio_pubkey::declare_id!("11111111111111111111111111111111"); | ||
|
||
pub trait CanInvoke<const ACCOUNTS_LEN: usize>: Sized { |
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.
ACCOUNTS_LEN
should be an associated type, not a type parameter.
I do not think it is meaningful to define multiple instances of CanInvoke
for the same type.
In other words, it does not make sense, for example, for the WithdrawNonceAccount
to have an instance of CanInvoke<2>
and CanInvoke<3>
.
Each specific type defines a single way of how it represents a program instruction that can be invoked.
And the number of accounts involved is a property of a particular instruction.
Moving ACCOUNTS_LEN
to be an associated type would reduce parametrization in the code that uses CanInvoke
as well.
The downside is that it would make invoke_via
non dyn-safe, but I think, for this method it should not be a problem?
Solana does not require instructions to always accept the same number of accounts.
Though, I think, it is not very common, it is possible.
So, for an instruction that accepts more than one number of accounts, it is possible to define several structs, say WithdrawNonceAccount3
and WithdrawNonceAccount5
, if WithdrawNonceAccount
had accepted 3 and 5 accounts respectively.
In terms of flexibility, it is the same as the generic parameter.
But this still leaves a gap in case an instruction accepts a variable number of accounts, based on some runtime properties.
This would be the cpi::slice_invoke()
function and friends.
I think a separate trait could cover this case.
Maybe named CanInvokeSlice
, to match the API function naming scheme.
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.
unfortunately that's not possible due to a current limitations of const generics
and would require unstable branch to enable the feature.
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.
a possible solution
pub trait CanInvoke: Sized {
type Accounts;
fn invoke_via(
self,
invoke: impl FnOnce(
/* program_id: */ &Pubkey,
/* accounts: */ &Self::Accounts,
/* account_metas: */ &[AccountMeta],
/* data: */ &[u8],
) -> ProgramResult,
) -> ProgramResult;
}
mod sealed {
pub trait Sealed {}
impl<T> Sealed for T where T: crate::CanInvoke {}
}
pub trait Invoke: sealed::Sealed {
fn invoke(self) -> ProgramResult;
fn invoke_signed(self, signers: &[Signer]) -> ProgramResult;
}
impl<'a, const ACCOUNTS_LEN: usize, T> Invoke for T
where
T: CanInvoke<Accounts = [&'a AccountInfo; ACCOUNTS_LEN]>,
{
fn invoke(self) -> ProgramResult {
self.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::invoke(&instruction, accounts)
})
}
fn invoke_signed(self, signers: &[Signer]) -> ProgramResult {
self.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::invoke_signed(&instruction, 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.
unfortunately that's not possible due to a current limitations of
const generics
and would require unstable branch to enable the feature.
This is unfortunate.
I was suggesting we use typenum
instead of const generics in another PR.
It seems const generics are still pretty limited on stable.
a possible solution
[...]
Interesting. Not sure if I would have figured it out %)
I assume the compiler can figure out a value for ACCOUNTS_LEN
and it does not need to be specified?
I've tried using your trick with a typenum
based solution.
But it does not seem to generalize for a more complex case.
I actually wonder why are we putting some of the instruction information into a struct, to only then deconstruct it into components and call cpi::invoke*()
?
If we just drop the intermediate struct, things become much simpler.
Here is what the transfer instruction looks like:
use pinocchio::{
account_info::AccountInfo,
cpi,
instruction::{AccountMeta, Instruction, Signer},
ProgramResult,
};
pub fn invoke(from: &AccountInfo, to: &AccountInfo, lamports: u64) -> ProgramResult {
invoke_signed(from, to, lamports, &[])
}
pub fn invoke_signed(
from: &AccountInfo,
to: &AccountInfo,
lamports: u64,
signers: &[Signer],
) -> ProgramResult {
// instruction data
// - [0..4 ]: instruction discriminator
// - [4..12]: lamports amount
let mut data = [0; 12];
data[0] = 2;
data[4..12].copy_from_slice(&lamports.to_le_bytes());
let instruction = Instruction {
program_id: &crate::ID,
accounts: &[
AccountMeta::writable_signer(from.key()),
AccountMeta::writable(to.key()),
],
data: &data,
};
let accounts = &[from, to];
cpi::invoke_signed(&instruction, accounts, signers)
}
And, actually, we can do even better, as we know that there is supposed to be exactly one signer. So there seems to be no reason to provide a case for no signers, or a case for multiple signers:
use pinocchio::{
account_info::AccountInfo,
cpi,
instruction::{AccountMeta, Instruction, Signer},
ProgramResult,
};
pub fn invoke(
from: &AccountInfo,
from_signer: Option<Signer>,
to: &AccountInfo,
lamports: u64,
) -> ProgramResult {
// instruction data
// - [0..4 ]: instruction discriminator
// - [4..12]: lamports amount
let mut data = [0; 12];
data[0] = 2;
data[4..12].copy_from_slice(&lamports.to_le_bytes());
let instruction = Instruction {
program_id: &crate::ID,
accounts: &[
AccountMeta::writable_signer(from.key()),
AccountMeta::writable(to.key()),
],
data: &data,
};
let accounts = &[from, to];
cpi::invoke_signed(&instruction, accounts, from_signer.as_slice())
}
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 would be the
cpi::slice_invoke()
function and friends. I think a separate trait could cover this case. Maybe namedCanInvokeSlice
, to match the API function naming scheme.
This was actually a bad idea.
As it would require the caller to use different invoke methods depending on if the instruction has a number of accounts known at compile time or not.
If we are going with the const generics approach, a better solution would hide this distinction within the struct implementation of CanInvoke
.
As the account structure is known within the CanInvoke::invoke_via()
implementation, it can be provided with two callbacks.
Allowing it to choose the one that fits.
Something like this:
pub trait CanInvoke {
type SizedAccounts;
fn invoke_via(
&self,
invoke: impl FnOnce(
/* program_id: */ &Pubkey,
/* accounts: */ &Self::SizedAccounts,
/* account_metas: */ &[AccountMeta],
/* data: */ &[u8],
) -> ProgramResult,
invoke_slice: impl FnOnce(
/* program_id: */ &Pubkey,
/* accounts: */ &[&AccountInfo],
/* account_metas: */ &[AccountMeta],
/* data: */ &[u8],
) -> ProgramResult,
) -> ProgramResult;
}
mod sealed {
pub trait Sealed {}
impl<T> Sealed for T where T: crate::CanInvoke {}
}
pub trait Invoke: sealed::Sealed {
fn invoke(&self) -> ProgramResult;
fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult;
}
impl<'a, const ACCOUNTS_LEN: usize, T> Invoke for T
where
T: CanInvoke<SizedAccounts = [&'a AccountInfo; ACCOUNTS_LEN]>,
{
fn invoke(&self) -> ProgramResult {
self.invoke_via(
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
cpi::invoke(&instruction, accounts)
},
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
cpi::invoke_slice(&instruction, accounts)
},
)
}
fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult {
self.invoke_via(
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
cpi::invoke_signed(&instruction, accounts, signers)
},
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
cpi::invoke_slice_signed(&instruction, accounts, signers)
},
)
}
}
But it still seems that just using function everything is much simpler.
And the invoke
function for a specific program can use the right cpi::invoke*()
function.
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 unfortunate.
I was suggesting we use typenum instead of const generics in another PR.
It seems const generics are still pretty limited on stable.
Yup doesn't seem to be priority, granted it's also a hard problem.
I assume the compiler can figure out a value for ACCOUNTS_LEN and it does not need to be specified?
Yeah it's being bounded by T: CanInvoke<Accounts = [&'a AccountInfo; ACCOUNTS_LEN]>
I've tried using your trick with a typenum based solution.
But it does not seem to generalize for a more complex case.
Luckily I don't see there being any other cases other than a slice
or afixed slice
.
Using enums
is a good solution but adds branching, when there's a low amount of variants and static code rust can optimize away branching but that's not guaranteed. Blow I have added a solution using the type system to avoid branching.
I actually wonder why are we putting some of the instruction information into a struct, to only then deconstruct it > into components and call cpi::invoke*()?
No idea it was like that when I got here hehe, but it's part of the public API and would require a breaking change. My best guess it was used to prevent a function with many args. Rusts zero cost abstractions remove intermediate structs, so it's really just a way to organize code without any overhead.
If we just drop the intermediate struct, things become much simpler.
Here is what the transfer instruction looks like:
....
But it still seems that just using function everything is much simpler.
And the invoke function for a specific program can use the right cpi::invoke*() function.
I agree if the only cases were signed and unsigned we wouldn't need the trait or some intermediate struct or even a PR, the current implementation works just fine. The issue arises from #124, so the question then becomes do we have duplicate code for the mapping between an instruction definition and the instruction call or can we do better.
This is one of the reasons behind InvokeParts
in the previous PR(#130) to generalize for the use between safe and unsafe invocations.
Now lets update the snippet from above to allow for both slices and fixed slices.
Note the below cant be done with traits due to limitations of the type system and blanket trait impls.
Also I don't think we need to support slice calls for well defined instructions right? I can't think of a case where a slice is needed. None the less here's the code
// Just some type aliases for account types
pub type ConstAccounts<'a, const ACCOUNTS_LEN: usize> = [&'a AccountInfo; ACCOUNTS_LEN];
pub type SliceAccounts<'a> = [&'a AccountInfo];
mod sealed {
pub trait Sealed {}
impl<'a, const ACCOUNTS_LEN: usize> Sealed for crate::ConstAccounts<'a, ACCOUNTS_LEN> {}
impl<'a> Sealed for crate::SliceAccounts<'a> {}
}
// a sealed trait to restrict accounts type generics
pub trait AccountType: sealed::Sealed {}
impl<'a, const ACCOUNTS_LEN: usize> AccountType for ConstAccounts<'a, ACCOUNTS_LEN> {}
impl<'a> AccountType for SliceAccounts<'a> {}
pub trait CanInvoke {
type Accounts: AccountType;
fn invoke_via(
&self,
invoke: impl FnOnce(
/* program_id: */ &Pubkey,
/* accounts: */ &Self::Accounts,
/* account_metas: */ &[AccountMeta],
/* data: */ &[u8],
) -> ProgramResult,
) -> ProgramResult;
// NOTE: to invoke you will need to call this first e.g. `transfer.as_invoker().invoke()`
// NOTE: this will be optimized away as well as the intermediate struct
#[inline]
fn as_invoker<'a>(&'a self) -> Invoker<'a, Self, &'a Self::Accounts>
where
Self: Sized,
{
Invoker {
inner: self,
account_ty: PhantomData,
}
}
}
// An intermediate struct to help direct the compiler to the desired implementation
// This will be optimized away, it basically acts like an extension to T
pub struct Invoker<'a, T, Account> {
inner: &'a T,
account_ty: PhantomData<Account>,
}
// implementation for fixed slice accounts
impl<'a, const ACCOUNTS_LEN: usize, T> Invoker<'a, T, &ConstAccounts<'a, ACCOUNTS_LEN>>
where
T: CanInvoke<Accounts = ConstAccounts<'a, ACCOUNTS_LEN>>,
{
#[inline]
pub fn invoke(self) -> ProgramResult {
self.inner
.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::invoke(&instruction, accounts)
})
}
#[inline]
pub fn invoke_signed(self, signers: &[Signer]) -> ProgramResult {
self.inner
.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::invoke_signed(&instruction, accounts, signers)
})
}
}
// implementation for a slice accounts
impl<'a, T> Invoker<'a, T, &SliceAccounts<'a>>
where
T: CanInvoke<Accounts = SliceAccounts<'a>>,
{
#[inline]
pub fn invoke(self) -> ProgramResult {
self.inner
.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::slice_invoke(&instruction, accounts)
})
}
#[inline]
pub fn invoke_signed(self, signers: &[Signer]) -> ProgramResult {
self.inner
.invoke_via(|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id: program_id,
accounts: &account_metas,
data: data,
};
pinocchio::cpi::slice_invoke_signed(&instruction, 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 assume the compiler can figure out a value for ACCOUNTS_LEN and it does not need to be specified?
Yeah it's being bounded by
T: CanInvoke<Accounts = [&'a AccountInfo; ACCOUNTS_LEN]>
I mean, when you invoke it in something like
Transfer {
from: &from_account,
to: &to_account,
lamports: 100_000,
}
.invoke();
Here, will the compiler correctly infer that ACCOUNTS_LEN
is 2?
And it would not require .invoke::<2>()
, right?
I've tried using your trick with a typenum based solution.
But it does not seem to generalize for a more complex case.Luckily I don't see there being any other cases other than a
slice
or afixed slice
.Using
enums
is a good solution but adds branching, when there's a low amount of variants and static code rust can optimize away branching but that's not guaranteed. Blow I have added a solution using the type system to avoid branching.
Not sure if I understand what enums you are talking about.
But the solution you have is pretty interesting.
I wonder if it may cause ambiguities, as nothing prevents both ConstAccounts
and SliceAccounts
to be defined for the same specific value of Accounts
.
Also, why is AccountType
necessary?
It does not seem like it is used for anything.
I can probably just pull your branch and check myself, but I'm a bit lazy right now :P
I actually wonder why are we putting some of the instruction information into a struct, to only then deconstruct it > into components and call cpi::invoke*()?
No idea it was like that when I got here hehe, but it's part of the public API and would require a breaking change. My best guess it was used to prevent a function with many args. Rusts zero cost abstractions remove intermediate structs, so it's really just a way to organize code without any overhead.
We do not necessary need to have a breaking change.
One way would be to add functions similar to the one I've suggested in the other thread.
And then use it in the implementation of Transfer::invoke()
.
Plus, mark Transfer::invoke()
as deprecated.
You can then add an unchecked version as a function.
Any common code will be shared.
I disagree that we have zero costs here.
Using these structs clearly complicates further development and higher level abstractions.
Partially because Rust const generics are not where they need to be.
But the cost generics implementation level is a secondary detail.
If a simpler solution is possible, it seems reasonable to use it.
I'm not really part of the SDK team. So, I'm not sure if I can push a change like this myself.
Maybe we can get @febo and/or @joncinque here to see what they think.
It is probably a bit tedious to reconstruct the exact details from all the conversations - I can chat with them and provide a summary.
If we just drop the intermediate struct, things become much simpler.
Here is what the transfer instruction looks like:
....
But it still seems that just using function everything is much simpler.
And the invoke function for a specific program can use the right cpi::invoke*() function.I agree if the only cases were signed and unsigned we wouldn't need the trait or some intermediate struct or even a PR, the current implementation works just fine. The issue arises from #124, so the question then becomes do we have duplicate code for the mapping between an instruction definition and the instruction call or can we do better.
This is one of the reasons behind
InvokeParts
in the previous PR(#130) to generalize for the use between safe and unsafe invocations.
Thank you for providing the context - I was actually wondering about it.
And sorry if I'm blocking your progress towards the problem you are trying to solve.
Still, if we add this code to the public API of the SDK it will be pretty hard to change later.
So, I think, it would be great to think enough about this problem to make sure we end up with a well-designed solution.
As of now, I still think that simple functions will both be easier to use, and will provide a better API.
Using a single function call rather than constructing a struct and calling a method on it seems more straightforward.
Plus, in the transfer::invoke()
function I was able to both specify that a maximum of 1 signer can be provided, and indicate what this signer is for.
Now lets update the snippet from above to allow for both slices and fixed slices.
We can probably work on the trait based solution in parallel.
In case there would be any issues switching to using functions directly.
Note the below cant be done with traits due to limitations of the type system and blanket trait impls.
Also I don't think we need to support slice calls for well defined instructions right? I can't think of a case where a slice is needed. None the less here's the code
I think it depends on the instruction.
For example, the multisig program will use slices, as the number of signers is only known at runtime.
It is the case that most programs have instructions with rather small scope, and it is very common to have a fixed number of accounts.
But it is not always the case.
// Just some type aliases for account types pub type ConstAccounts<'a, const ACCOUNTS_LEN: usize> = [&'a AccountInfo; ACCOUNTS_LEN]; pub type SliceAccounts<'a> = [&'a AccountInfo]; mod sealed { pub trait Sealed {} impl<'a, const ACCOUNTS_LEN: usize> Sealed for crate::ConstAccounts<'a, ACCOUNTS_LEN> {} impl<'a> Sealed for crate::SliceAccounts<'a> {} } // a sealed trait to restrict accounts type generics pub trait AccountType: sealed::Sealed {} impl<'a, const ACCOUNTS_LEN: usize> AccountType for ConstAccounts<'a, ACCOUNTS_LEN> {} impl<'a> AccountType for SliceAccounts<'a> {} pub trait CanInvoke { type Accounts: AccountType; fn invoke_via( &self, invoke: impl FnOnce( /* program_id: */ &Pubkey, /* accounts: */ &Self::Accounts, /* account_metas: */ &[AccountMeta], /* data: */ &[u8], ) -> ProgramResult, ) -> ProgramResult; // NOTE: to invoke you will need to call this first e.g. `transfer.as_invoker().invoke()` // NOTE: this will be optimized away as well as the intermediate struct #[inline] fn as_invoker<'a>(&'a self) -> Invoker<'a, Self, &'a Self::Accounts> where Self: Sized, { Invoker { inner: self, account_ty: PhantomData, } } } // An intermediate struct to help direct the compiler to the desired implementation // This will be optimized away, it basically acts like an extension to T pub struct Invoker<'a, T, Account> { inner: &'a T, account_ty: PhantomData<Account>, } // implementation for fixed slice accounts impl<'a, const ACCOUNTS_LEN: usize, T> Invoker<'a, T, &ConstAccounts<'a, ACCOUNTS_LEN>> where T: CanInvoke<Accounts = ConstAccounts<'a, ACCOUNTS_LEN>>, { #[inline] pub fn invoke(self) -> ProgramResult { self.inner .invoke_via(|program_id, accounts, account_metas, data| { let instruction = Instruction { program_id: program_id, accounts: &account_metas, data: data, }; pinocchio::cpi::invoke(&instruction, accounts) }) } #[inline] pub fn invoke_signed(self, signers: &[Signer]) -> ProgramResult { self.inner .invoke_via(|program_id, accounts, account_metas, data| { let instruction = Instruction { program_id: program_id, accounts: &account_metas, data: data, }; pinocchio::cpi::invoke_signed(&instruction, accounts, signers) }) } } // implementation for a slice accounts impl<'a, T> Invoker<'a, T, &SliceAccounts<'a>> where T: CanInvoke<Accounts = SliceAccounts<'a>>, { #[inline] pub fn invoke(self) -> ProgramResult { self.inner .invoke_via(|program_id, accounts, account_metas, data| { let instruction = Instruction { program_id: program_id, accounts: &account_metas, data: data, }; pinocchio::cpi::slice_invoke(&instruction, accounts) }) } #[inline] pub fn invoke_signed(self, signers: &[Signer]) -> ProgramResult { self.inner .invoke_via(|program_id, accounts, account_metas, data| { let instruction = Instruction { program_id: program_id, accounts: &account_metas, data: data, }; pinocchio::cpi::slice_invoke_signed(&instruction, accounts, signers) }) } }
A minor nitpick is that ConstAccounts
can be interpreted as if the accounts themselves are constant.
I was struggling with a good name for this, when I was writing my previous example.
Even while I do not like it too much, I think FixedAccounts
is better than ConstAccounts
.
But maybe there is a better name yet.
A complete name would be something like AccountsCompileTimeKnownLength
, but it is just too long.
With the other one being called AccountsRuntimeLength
.
Maybe AccountsCTLen
and AccountRTLen
?
Not great, and also the Len
suffix makes it look as if it is a length of something, while it is actually an array.
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 tried compiling the version with ConstAccounts
and SliceAccounts
, and there were issues with lifetimes.
In particular, when I tried implementing it for Transfer
.
And after I've added lifetimes, now, for some reason, the compiler was refusing to treat ACCOUNTS_LEN
as constrained in the following impl:
impl<'a, const ACCOUNTS_LEN: usize, T> Invoker<'a, T, &ConstAccounts<'a, ACCOUNTS_LEN>>
where
T: CanInvoke<'a, Accounts = ConstAccounts<'a, ACCOUNTS_LEN>>,
(Notice that in this version, CanInvoke
as an extra lifetime parameter).
I had a similar problem when I was using typenum
.
Seems like there is some hardcoded special treatment for certain use of const generics.
And it fails when the case gets too complicated.
I was actually surprised it worked here.
In any case, it seems that accepting two invoke
callbacks is just simpler still.
Below is a version that worked for me.
Notice that CanInvoke::Accounts
is now a GAT, as it needs to accept a lifetime.
This lifetime is related to the fact that the caller of invoke_via
needs to provide callbacks that need to be able to handle references with arbitrary lifetimes.
This removes any lifetimes from Invoke
itself, which seems correct to me.
It is up to the CanInvoke::invoke_via
implementation to decide how the arguments are going to be allocated. They can be stack allocated, live inside self
, or live for the program lifetime.
Maybe there is a way to be more precise here, that would remove this generalization?
But I am not sure...
lib.rs
:
#![no_std]
use pinocchio::{
account_info::AccountInfo,
cpi,
instruction::{AccountMeta, Instruction, Signer},
pubkey::Pubkey,
ProgramResult,
};
pub mod instructions;
pinocchio_pubkey::declare_id!("11111111111111111111111111111111");
mod sealed {
pub trait Sealed {}
impl<T> Sealed for T where T: crate::CanInvoke {}
}
pub trait CanInvoke {
type Accounts<'a>;
fn invoke_via(
&self,
invoke: impl for<'a> FnOnce(
/* program_id: */ &'a Pubkey,
/* accounts: */ &'a Self::Accounts<'a>,
/* account_metas: */ &'a [AccountMeta],
/* data: */ &'a [u8],
) -> ProgramResult,
slice_invoke: impl for<'a> FnOnce(
/* program_id: */ &'a Pubkey,
/* accounts: */ &'a [&'a AccountInfo],
/* account_metas: */ &'a [AccountMeta],
/* data: */ &'a [u8],
) -> ProgramResult,
) -> ProgramResult;
}
pub trait Invoke: sealed::Sealed {
fn invoke(&self) -> ProgramResult;
fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult;
}
impl<const ACCOUNTS_LEN: usize, T> Invoke for T
where
T: for<'a> CanInvoke<Accounts<'a> = [&'a AccountInfo; ACCOUNTS_LEN]>,
{
fn invoke(&self) -> ProgramResult {
self.invoke_via(
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id,
accounts: &account_metas,
data,
};
cpi::invoke(&instruction, accounts)
},
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id,
accounts: &account_metas,
data,
};
cpi::slice_invoke(&instruction, accounts)
},
)
}
fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult {
self.invoke_via(
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id,
accounts: &account_metas,
data,
};
cpi::invoke_signed(&instruction, accounts, signers)
},
|program_id, accounts, account_metas, data| {
let instruction = Instruction {
program_id,
accounts: &account_metas,
data,
};
cpi::slice_invoke_signed(&instruction, accounts, signers)
},
)
}
}
And here is an implementation for Transfer
:
(I've added a few pub
to AccountInfo
and Account
in the SDK to be able to compile the test)
use pinocchio::{
account_info::AccountInfo, instruction::AccountMeta, pubkey::Pubkey, ProgramResult,
};
use crate::CanInvoke;
/// Transfer lamports.
///
/// ### Accounts:
/// 0. `[WRITE, SIGNER]` Funding account
/// 1. `[WRITE]` Recipient account
pub struct Transfer<'a> {
/// Funding account.
pub from: &'a AccountInfo,
/// Recipient account.
pub to: &'a AccountInfo,
/// Amount of lamports to transfer.
pub lamports: u64,
}
const ACCOUNTS_LEN: usize = 2;
impl CanInvoke for Transfer<'_> {
type Accounts<'a> = [&'a AccountInfo; ACCOUNTS_LEN];
fn invoke_via(
&self,
invoke: impl for<'a> FnOnce(
/* program_id: */ &'a Pubkey,
/* accounts: */ &'a [&'a AccountInfo; ACCOUNTS_LEN],
/* account_metas: */ &'a [AccountMeta],
/* data: */ &'a [u8],
) -> ProgramResult,
_slice_invoke: impl for<'a> FnOnce(
/* program_id: */ &'a Pubkey,
/* accounts: */ &'a [&'a AccountInfo],
/* account_metas: */ &'a [AccountMeta],
/* data: */ &'a [u8],
) -> ProgramResult,
) -> ProgramResult {
let mut instruction_data = [0; 12];
instruction_data[0] = 2;
instruction_data[4..12].copy_from_slice(&self.lamports.to_le_bytes());
invoke(
&crate::ID,
&[self.from, self.to],
&[
AccountMeta::writable_signer(self.from.key()),
AccountMeta::writable(self.to.key()),
],
&instruction_data,
)
}
}
#[cfg(test)]
mod tests {
use pinocchio::{
account_info::{Account, AccountInfo},
ProgramResult,
};
use crate::Invoke as _;
use super::Transfer;
const NOT_BORROWED: u8 = u8::MAX;
#[test]
fn simple_transfer() {
// 8-bytes aligned account data.
let mut from_data = {
let mut data = [0u64; size_of::<Account>() / size_of::<u64>()];
data[0] = NOT_BORROWED as u64;
data
};
let from = AccountInfo {
raw: from_data.as_mut_ptr() as *mut Account,
};
let mut to_data = {
let mut data = [0u64; size_of::<Account>() / size_of::<u64>()];
data[0] = NOT_BORROWED as u64;
data
};
let to = AccountInfo {
raw: to_data.as_mut_ptr() as *mut Account,
};
let res = Transfer {
from: &from,
to: &to,
lamports: 42,
}
.invoke();
assert_eq!(res, ProgramResult::Ok(()));
}
}
While I see how this approach captures the notion of "this struct represents arguments for a CPI call, and so you can call it in a few different ways", the solutions still seem somewhat complex.
P.S. This line was also somewhat surprising for me:
T: for<'a> CanInvoke<Accounts<'a> = [&'a AccountInfo; ACCOUNTS_LEN]>,
I would not expect a need for 'a
to be qualified in any way.
This is supposed to be an equality test, not an instantiation.
And so the for<'a>
part makes no sense, I think.
But the compiler required me to do it.
I would expect the constraint to look like this:
T: CanInvoke<Accounts<'a> = [&'a AccountInfo; ACCOUNTS_LEN]>,
'a
is introduced by the Accounts<'a>
. It should not be a free variable.
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.
Alright I've made a push the with all three solutions, double callbacks, callback + Invoker, and InvokeParts.
After seeing all three laid out, I still think the InvokeParts wins in readability and extensibility(remember we want to add unsafe calls in the future), and separation of concern.
With only callbacks the trait will need a breaking change every time a new variation is crated.
The Invoke + callback, feels a bit hacky/ a workaround for the IntoParts version with callbacks. You get the downsides of both callbacks and IntoParts as well as making the code a bit hard to follow and organize.
Given that the client code will most likely be generated I really do think providing a simple mapping between a Instruction definition and an InvokeParts will make it less breaking and adaptable.
At the bottom of each solution you will find the impl for the Transfer
ix. I'll try and see if I can get the byte code generated to see if there's any difference between the three once compiled.
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.
Alright looks like the generated bytecode is identical for both the callback and invoke parts implementations. So it really is just about the code organization. I'm happy to go with either or.
I tried to get all the context from the discussion but I must admit that I might have missed something. I will start with a bit of background on the motivation for these CPI helpers. The main idea is to provide a "nice" UX to perform CPI calls – nice in the sense that the caller does not need to think about allocations, With this in mind, would a trait similar to: pub trait CanInvoke {
fn invoke(&self) -> ProgramResult {
self.invoke_signed(self, &[])
}
fn invoke_signed(&self, signers: &[Signer]) -> ProgramResult;
} be sufficient? These CPI helpers soon won't need to be written by hand (see here). Separate from that, it would be great to check the impact on CUs when refactoring these helpers. That can help guide which solution provides a good balance between UX and performance. |
3d66f5d
to
7ca83e8
Compare
7ca83e8
to
23d48a3
Compare
Problem
The predefined solana programs(
system
,token
, etc.) only exposeinvoke
andinvoke_signed
calls. It would be nice to have an abstraction over the behavior.Solution
CanInvoke
trait that contains default implementations for 'invoke' and 'invoke_signed'