-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: Light SDK without Anchor #1349
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the pr! Couple comments.
@@ -11,13 +11,14 @@ crate-type = ["cdylib", "lib"] | |||
name = "light_sdk" | |||
|
|||
[features] | |||
anchor = [] | |||
no-entrypoint = [] | |||
no-idl = [] | |||
no-log-ix-name = [] | |||
cpi = ["no-entrypoint"] | |||
custom-heap = ["light-heap"] |
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.
what do we need light-heap
for in light-sdk
?
can we remove light-heap
and the custom heap feature from this cargo toml?
(one feedback from squads was that they use their own heap implementation and encountered conflicts.)
#[cfg(feature = "anchor")] | ||
use anchor_lang::{AnchorDeserialize, AnchorSerialize}; | ||
#[cfg(not(feature = "anchor"))] | ||
use borsh::{BorshDeserialize as AnchorDeserialize, BorshSerialize as AnchorSerialize}; |
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.
nice fix
pub fn new_address_params(&self) -> Option<PackedNewAddressParams> { | ||
unimplemented!() | ||
} |
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.
what's the purpose of this placeholder?
sdk/src/error.rs
Outdated
impl From<LightSdkError> for u32 { | ||
fn from(e: LightSdkError) -> Self { | ||
match e { | ||
LightSdkError::ConstraintViolation => 13001, |
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 already use 13000 + for errors of the light-verifier
crate, can we use 14000 instead?
}; | ||
use thiserror::Error; | ||
|
||
entrypoint!(process_instruction); |
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.
file would be easier to read if we put the implementation of process_instruction up here as well.
.data | ||
.ok_or(LightSdkError::ExpectedData)?; | ||
let mut data = data.borrow_mut(); | ||
let mut cpda = MyCompressedAccount::deserialize(&mut data.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.
Since we are using borsh here, can we do a test which uses LightAccount
directly (just duplicate this one but use LightAccount
without anchor?
(It definitely makes sense to have a test without LightAccount
as this one to make sure that the sdk works with any deserialization.)
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.
what do you think about renaming LightAccount
to LightBorshAccount
?
(This would fit nicely with future LightZeroCopyAccount
.)
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 like the idea.
The reason why I went for LightAccount
before is sticking with Anchor's naming. But I can see the appeal of implementing more wrappers (bytemuck, bincode etc.).
e914db7
to
5fa6ba1
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.
Nice accounts look a lot cleaner now! Comments are mainly about account checks and some naming here a summary:
- when initing system accounts, imo we should treat fee_payer and authority separately.
- For system account checks we need to check that fee_payer and authority are signers, invoking program is the correct program and system program is the correct program.
- instruction accounts and system accounts are the same we should unify naming see comment.
} | ||
|
||
pub fn fee_payer(&self) -> &'c AccountInfo<'info> { | ||
// PANICS: We are sure about the bounds of the 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.
does split_at
panic if requested len is out of range?
sdk/src/system_accounts.rs
Outdated
pub fn new(accounts: &'c [I]) -> Self { | ||
// `split_at` doesn't make any copies. | ||
|
||
// Take `CPI_ACCOUNTS_LEN` elements. | ||
let (accounts, _) = accounts.split_at(SYSTEM_ACCOUNTS_LEN); | ||
Self { | ||
accounts, | ||
_marker: PhantomData, | ||
} | ||
} | ||
|
||
pub fn new_with_start_index(accounts: &'c [I], start_index: usize) -> Self { |
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.
if used with anchor it doesn't feel intuitive to me to assume thatfee_payer
and authority
are passed with the other accounts in correct order with remaining accounts since you will want to do a signer check for authority
.
Can we pass fee_payer
and authority
as separate account infos and check that these are 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.
additionally we need to check that light_system_program
is the actual light system program and that it is executable. We need to do the same for invoking_program
that it's id is the program id. The program id should be passed as an argument or const generic. We do a check like this here.
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.
pub fn new(accounts: &'c [I]) -> Self { | |
// `split_at` doesn't make any copies. | |
// Take `CPI_ACCOUNTS_LEN` elements. | |
let (accounts, _) = accounts.split_at(SYSTEM_ACCOUNTS_LEN); | |
Self { | |
accounts, | |
_marker: PhantomData, | |
} | |
} | |
pub fn new_with_start_index(accounts: &'c [I], start_index: usize) -> Self { | |
pub fn new( fee_payer: AccountInfo<'info>, authority:AccountInfo<'info>, program_id: &Pubkey, accounts: &'c [I])) -> Self { | |
Self::new_with_start_index(accounts, 0, fee_payer, authority, program_id) | |
} | |
pub fn new_with_start_index(fee_payer: AccountInfo<'info>, authority:AccountInfo<'info>, program_id: &Pubkey, accounts: &'c [I], start_index: usize) -> Self { | |
// check invoking_program_id vs program id here |
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.
if we pass fee_payer and authority separately maybe it makes sense to store these references in separate fields to avoid to create a vector in case that's necessary.
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.
Yeah, the main reason I wanted to avoid that is that it screws up my nice trick with just assigning the slice to the struct. 😅
With your approach, I will have to create a vector. I can try to do that and make a vector of references, so we don't make any copies, but we will still do a wasteful allocation of 11 * 8 bytes for storing the pointers.
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.
if you add two fields fee_payer
and authority
you could use your trick for the remaining accounts without creating a vector right?
pub struct CreateRecord<'info> { | ||
#[account(mut)] | ||
#[fee_payer] | ||
pub signer: Signer<'info>, |
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.
feels intuitive to me to keep the signer here so that it can be used for anchor checks see comment for LightSystemAccounts
registered_program_pda: &Pubkey, | ||
account_compression_authority: &Pubkey, |
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.
these accounts are always the same, you can hardcode these.
I think you are missing the cpi signer of the invoking program which is also deterministic you can derive it inside of new from the program id.
program_id: &Pubkey, | ||
accounts: &[AccountInfo], | ||
instruction_data: &[u8], | ||
) -> ProgramResult { |
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.
signer checks are missing
use solana_program::{account_info::AccountInfo, instruction::AccountMeta}; | ||
|
||
#[repr(usize)] | ||
pub enum LightSystemAccountIndex { |
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.
You are probably aware just a reminder for final review, there is a duplicate enum in instruction_accounts
.
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, good point. I kept this one and removed the other one.
}; | ||
|
||
/// Collection of Light Protocol system accounts which are sent to the program. | ||
pub struct LightInstructionAccounts { |
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 could rename to LightInstructionAccountsBuilder
and LightSystemAccounts
to LightInstructionAccounts
or the other way around to keep naming consistent and intuitive for devs on and offchain.
wdyt @vadorovsky ?
cc @SwenSchaeferjohann .
05ed541
to
1be0fcb
Compare
sdk/src/system_accounts.rs
Outdated
pub struct LightCpiAccounts<'c, 'info, I> | ||
where | ||
// The reason behind using `AsRef` is to handle the inconsistency between | ||
// solana-program and Anchor: | ||
// | ||
// - solana-program passes all accounts as `&[&AccountInfo]` (slice of | ||
// references to `AccountInfo`). | ||
// - Anchor stores `remaining_accounts` as `&[AccountInfo]` (slices of | ||
// owned `AccountInfo` instances). | ||
// | ||
// Taking `AsRef<AccountInfo<'info>>` is the only way which makes this | ||
// wrapper work with both owned and referenced `AccountInfo`s. | ||
I: AsRef<AccountInfo<'info>>, | ||
{ | ||
accounts: &'c [I], | ||
_marker: PhantomData<&'info ()>, | ||
} |
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.
pub struct LightCpiAccounts<'c, 'info, I> | |
where | |
// The reason behind using `AsRef` is to handle the inconsistency between | |
// solana-program and Anchor: | |
// | |
// - solana-program passes all accounts as `&[&AccountInfo]` (slice of | |
// references to `AccountInfo`). | |
// - Anchor stores `remaining_accounts` as `&[AccountInfo]` (slices of | |
// owned `AccountInfo` instances). | |
// | |
// Taking `AsRef<AccountInfo<'info>>` is the only way which makes this | |
// wrapper work with both owned and referenced `AccountInfo`s. | |
I: AsRef<AccountInfo<'info>>, | |
{ | |
accounts: &'c [I], | |
_marker: PhantomData<&'info ()>, | |
} | |
pub struct LightCpiAccounts<'c, 'info, I> | |
where | |
// The reason behind using `AsRef` is to handle the inconsistency between | |
// solana-program and Anchor: | |
// | |
// - solana-program passes all accounts as `&[&AccountInfo]` (slice of | |
// references to `AccountInfo`). | |
// - Anchor stores `remaining_accounts` as `&[AccountInfo]` (slices of | |
// owned `AccountInfo` instances). | |
// | |
// Taking `AsRef<AccountInfo<'info>>` is the only way which makes this | |
// wrapper work with both owned and referenced `AccountInfo`s. | |
I: AsRef<AccountInfo<'info>>, | |
{ | |
fee_payer: &'c I, | |
authority: &'c I, | |
accounts: &'c [I], | |
_marker: PhantomData<&'info ()>, | |
} |
be5ca6c
to
3870020
Compare
Provide a possibility to use the Rust Light SDK for programs which are not using Anchor. Related changes: - Rename the former `sdk-test` program to `sdk-anchor-test`. - Add the new `sdk-test` program, which uses only solana-program. - Replace all re-imports from `anchor_lang` with direct imports from `solana_program`. - Introduce `anchor` feature flag in `light_sdk`. - Make the whole `light_sdk::verify` module independent from Anchor.
3870020
to
c5c882b
Compare
Unfortunately, there are still errors when running the tests for
The latest error is:
This means that there is something wrong going on with the Merkle tree incides. To give you some context here - there are 3 places in which we deal with account packing:
light-protocol/sdk/src/instruction_accounts.rs Lines 25 to 49 in c5c882b
Then the Merkle tree accounts are being added through the light-protocol/sdk/src/account_meta.rs Line 51 in c5c882b
light-protocol/sdk/src/merkle_context.rs Lines 57 to 58 in c5c882b
light-protocol/sdk/src/system_accounts.rs Lines 25 to 42 in c5c882b
light-protocol/examples/name-service/programs/name-service-without-macros/src/lib.rs Lines 133 to 137 in c5c882b
In Anchor-less programs, this wrapper can be used just with
Before making a CPI, we subtract the Merkle tree account indices by the number of "system accounts" passed in the wrapper: light-protocol/sdk/src/account_info.rs Lines 75 to 84 in c5c882b
Before doing so, the error was even worse, because system-program would crash with "out of bounds error". The reason why it's done is that the most of what we considered as "remaining accounts" in the example program, is being recognized by Anchor in light-system-program instructions. Merkle trees are being the only "remaining accounts". But still, even after adjusting the indices, we're hitting the error. I think the bug resides in this exact mechanism and subtraction. Maybe to avoid that entirely, we should construct the Summary - remaining work
|
Provide a possibility to use the Rust Light SDK for programs which are not using Anchor.
Related changes:
sdk-test
program tosdk-anchor-test
.sdk-test
program, which uses only solana-program.anchor_lang
with direct imports fromsolana_program
.anchor
feature flag inlight_sdk
.light_sdk::verify
module independent from Anchor.