Skip to content

port and improve inline-spl from agave#70

Merged
2501babe merged 26 commits intosolana-program:mainfrom
2501babe:inline-spl
Apr 14, 2025
Merged

port and improve inline-spl from agave#70
2501babe merged 26 commits intosolana-program:mainfrom
2501babe:inline-spl

Conversation

@2501babe
Copy link
Member

@2501babe 2501babe commented Apr 8, 2025

due to the exigencies of history and the tumult of our modern world, we will need to parse token accounts and mints inside svm. the reasons for this are out of scope

agave has a small crate solana_inline_spl, which provides a thin bytemuck view over spl_token and spl_token_2022 accounts. i intended to use this mostly as-is, but it turns out we are splitting the agave and svm repos, so putting it in either of them would introduce a circular dependency. we dont want to put it in the token repos because it uses elements from all of tokenkeg, token22, and ata. we were going to put it in sdk, but everyone who approved of this approved tepidly at best. it seems silly to give it its own repo because it is so small. this feels like the perfect place, particularly since the workspace lets packages control their own dependencies seperately

changes from the existing solana_inline_spl:

  • the generic account trait now provides amount
  • we add a generic mint trait providing supply and decimals
  • we add concrete types generic_token::Account and generic_token::Mint which provide unpack(). this is because as-designed the generic trait expects callers to know what token programs they are working with and choose the correct trait implementation. instead, we accept the program id and outside callers do not need to know about specific token programs at all. we heavily restrict the fields we make available: if you need mint authority, account delegation, etc, you are doing something more complicated than this package is meant to cover
  • we port the functions spl_token_ids() and is_known_spl_token_id() from solana_account_decoder to consolidate in one place
  • some minor cleanups like taking account offsets private and using a macro for the safe wrappers

we also use the token22 implementation of verify_account_data() which fixes a bug found in the inline-spl version

to see only my changes to the original agave impl: git diff ca57aea..8e1aa5a

@2501babe 2501babe self-assigned this Apr 8, 2025
@2501babe 2501babe force-pushed the inline-spl branch 4 times, most recently from 862db78 to 3ea338c Compare April 8, 2025 19:16
@2501babe 2501babe marked this pull request as ready for review April 8, 2025 21:25
@2501babe 2501babe requested review from joncinque and removed request for joncinque April 8, 2025 21:38
@2501babe 2501babe marked this pull request as draft April 8, 2025 21:41
@2501babe 2501babe marked this pull request as ready for review April 10, 2025 12:13
@2501babe 2501babe requested a review from joncinque April 10, 2025 12:13
Comment on lines +63 to +71
if is_initialized && is_token_2022_account {
account_data.resize(SplAccount::LEN + 2, 0);
set_account_type::<SplAccount2022>(&mut account_data).unwrap();
Copy link
Member Author

@2501babe 2501babe Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest i couldnt figure out why set_account_type() requires there to be 167 bytes instead of 166

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's actually an off-by-one error in type_and_tlv_indices, and the <= should be <: https://github.com/solana-program/token-2022/blob/f2341f9d06abc0f0781cec5948232169d5e372a2/program/src/extension/mod.rs#L315

I put in solana-program/token-2022#369 to fix it

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall! Mostly small things

Comment on lines +4 to +6
pub mod program_v1_1_0 {
solana_pubkey::declare_id!("NatA1Zyo48dJ7yuwR7cGURwhskKA8ywUyxb9GvG7mTC");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove this, since it was just used for the program upgrade

@@ -0,0 +1,25 @@
# this package prevents a (future) circular dependency between spl_generic_token and spl_token
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense if we imagine that programs will implement the traits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, the programs already impl their own versions of the traits distinct from inline-spl so the code that makes up generic-token presently is copy-pasted in three places, my thinking was by having no dev dependency we can delete the code not just in inline-spl but in the token programs too and we have one and only one impl

] }

[lib]
crate-type = ["cdylib", "lib"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be an rlib or lib, no need for cdylib

Comment on lines +10 to +19
generic_token::Account {
mint: Pubkey,
owner: Pubkey,
amount: u64,
}

generic_token::Mint {
supply: u64,
decimals: u8,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe we should give the full path for users? On the flip-side, just seeing the path makes me wonder if we should change the filename from generic_token.rs to state.rs or something. What do you think?

Suggested change
generic_token::Account {
mint: Pubkey,
owner: Pubkey,
amount: u64,
}
generic_token::Mint {
supply: u64,
decimals: u8,
}
spl_generic_token::generic_token::Account {
mint: Pubkey,
owner: Pubkey,
amount: u64,
}
spl_generic_token::generic_token::Mint {
supply: u64,
decimals: u8,
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what i was imagining is a user doing use spl_generic_token::{generic_token, token::{self, GenericTokenAccount}, .. }. the existing pattern for token and token_2022 is that the structs are all named Mint and Account, so i kept the pattern so users can clearly distinguish their imported types without aliasing (generic_token::Account vs token::Account etc)

i think generic::Account and state::Account would both be confusing to users of the library if they import that way. however we do already have token::native as a module, so we could do token::generic::Account. i think a distinct toplevel (generic_token::Account) is preferable though because this overloads "token" to mean "the tokenkeg program" as well as "the concept of a token"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense to me!

Comment on lines +9 to +11
pub mod program_v3_4_0 {
solana_pubkey::declare_id!("NToK4t5AQzxPNpUA84DkxgfXaVDbDQQjpHKCqsbY46B");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this too

// Call after account length has already been verified
fn unpack_account_amount_unchecked(account_data: &[u8]) -> u64 {
let offset = SPL_TOKEN_ACCOUNT_AMOUNT_OFFSET;
*bytemuck::from_bytes(&account_data[offset..offset.wrapping_add(mem::size_of::<u64>())])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since account data is stored as little-endian, and this code could run on big-endian systems, I'm thinking we should use u64::from_le_bytes instead to be totally safe, similar to unpack_mint_supply_unchecked

);
define_checked_getter!(unpack_account_amount, unpack_account_amount_unchecked, u64);

// Call after account length has already been verified
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love these comments 🙏


pub struct Account;
impl Account {
pub fn get_packed_len() -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't see where this is used downstream, but wanna make this const?

Comment on lines +1 to +7
/// Minimum viable SPL Token parsers to avoid a dependency on the spl-token and spl-token-2022 crates.
/// Users may use the generic traits directly, but this requires them to select the correct implementation
/// based on the account's program id. `generic_token::Account` and `generic_token::Mint` abstract over
/// this and require no knowledge of the different token programs on the part of the caller at all.
///
/// We provide the minimum viable interface to determine balances and ownership. For more advanced use-cases,
/// it is recommended to use to full token program crates instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I believe these comments should have //! at the start of each line instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh this syntax is actually new to me!

Comment on lines +63 to +71
if is_initialized && is_token_2022_account {
account_data.resize(SplAccount::LEN + 2, 0);
set_account_type::<SplAccount2022>(&mut account_data).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's actually an off-by-one error in type_and_tlv_indices, and the <= should be <: https://github.com/solana-program/token-2022/blob/f2341f9d06abc0f0781cec5948232169d5e372a2/program/src/extension/mod.rs#L315

I put in solana-program/token-2022#369 to fix it

@2501babe
Copy link
Member Author

ok that should be everything! unless you have thoughts on changing the module name/structure

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@2501babe 2501babe merged commit c00f120 into solana-program:main Apr 14, 2025
29 checks passed
@2501babe 2501babe deleted the inline-spl branch April 14, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants