Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@program-libs/token-interface/src/state/mint/anchor_wrapper.rs`:
- Around line 49-55: Add the LightZeroCopy trait bound to the generic
AccountLoader<'info, T> so T: LightZeroCopy (and update every impl block for
AccountLoader<'info, T>) — this ensures only types marked by the LightZeroCopy
marker (e.g., Mint) can be used with AccountLoader and keeps the API
self-documenting; update the struct/generic declaration and all impl blocks
referencing AccountLoader<'info, T> to include the T: LightZeroCopy bound.
- Around line 288-469: Add two unit tests: one that verifies
AccountLoader::load_mut successfully returns a mutable Mint when account owner
and data indicate a valid, initialized Mint (set IS_INITIALIZED_OFFSET = 1 and
populate any required fields) and that modifications through the returned &mut
Mint persist in the wrapped AccountInfo data; and another that calls
AccountLoader::load() twice on the same AccountLoader instance (or two fresh
AccountLoader::new(info) views if necessary) to assert both calls succeed and
return equivalent Mint values, ensuring repeated reads are supported. Locate
tests near the existing test module and reference AccountLoader::load_mut and
AccountLoader::load (and IS_INITIALIZED_OFFSET, Mint) when creating the mock
AccountInfo.
In `@sdk-libs/macros/src/light_pdas/accounts/builder.rs`:
- Around line 404-427: The IDL generator (__anchor_private_gen_idl_accounts)
currently hardcodes writable: false and signer: false for every field; update
the code that builds the account list to collect per-field metadata (e.g., use
the ParsedField values: ParsedField.is_mut and ParsedField.is_signer alongside
field_names) and emit each IdlInstructionAccount with writable set to is_mut and
signer set to is_signer; locate the vector construction that iterates over
`#field_names` and change it to iterate over a collected (name, is_mut, is_signer)
tuple so each IdlInstructionAccount reflects the real mutability and signer
flags.
In `@sdk-libs/macros/src/light_pdas/accounts/parse.rs`:
- Around line 204-220: The current has_mut_attribute uses
token_str.contains("mut") which can produce false positives; instead parse the
attribute's Meta::List and iterate its list.nested items, and treat the
attribute as mut only when you find a NestedMeta that is exactly the identifier
"mut" (e.g., match NestedMeta::Meta(syn::Meta::Path(path)) and
path.is_ident("mut")). Update has_mut_attribute to ignore other nested meta
forms (like key = value or function calls) and only return true for a standalone
mut NestedMeta so you don't match substrings inside other tokens.
Comment on lines
+49
to
+55
| /// Marker trait for types that can be loaded via AccountLoader. | ||
| /// | ||
| /// This trait marks types that have zero-copy serialization support | ||
| /// and can be accessed through the AccountLoader pattern. | ||
| pub trait LightZeroCopy {} | ||
|
|
||
| impl LightZeroCopy for Mint {} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding LightZeroCopy trait bound to AccountLoader<T>.
The LightZeroCopy marker trait exists to mark types compatible with AccountLoader, but the generic AccountLoader<'info, T> struct doesn't require T: LightZeroCopy. Currently, someone could construct AccountLoader<'info, SomeOtherType> even though only Mint has the specialized load() methods.
This isn't a bug (there's no generic load() they could call), but adding the bound would make the API more self-documenting.
🔧 Optional: Add trait bound
-pub struct AccountLoader<'info, T> {
+pub struct AccountLoader<'info, T: LightZeroCopy> {
info: AccountInfo<'info>,
_phantom: PhantomData<T>,
}Note: This would require adding the bound to all impl blocks as well.
Also applies to: 82-85
🤖 Prompt for AI Agents
In `@program-libs/token-interface/src/state/mint/anchor_wrapper.rs` around lines
49 - 55, Add the LightZeroCopy trait bound to the generic AccountLoader<'info,
T> so T: LightZeroCopy (and update every impl block for AccountLoader<'info, T>)
— this ensures only types marked by the LightZeroCopy marker (e.g., Mint) can be
used with AccountLoader and keeps the API self-documenting; update the
struct/generic declaration and all impl blocks referencing AccountLoader<'info,
T> to include the T: LightZeroCopy bound.
Comment on lines
+288
to
+469
| #[cfg(test)] | ||
| mod tests { | ||
| use std::{cell::RefCell as StdRefCell, rc::Rc}; | ||
|
|
||
| use solana_pubkey::Pubkey as SolanaPubkey; | ||
|
|
||
| use super::*; | ||
|
|
||
| /// Helper to create a mock AccountInfo for testing | ||
| fn create_mock_account_info<'a>( | ||
| key: &'a SolanaPubkey, | ||
| owner: &'a SolanaPubkey, | ||
| lamports: &'a mut u64, | ||
| data: &'a mut [u8], | ||
| is_writable: bool, | ||
| is_signer: bool, | ||
| ) -> AccountInfo<'a> { | ||
| AccountInfo { | ||
| key, | ||
| lamports: Rc::new(StdRefCell::new(lamports)), | ||
| data: Rc::new(StdRefCell::new(data)), | ||
| owner, | ||
| rent_epoch: 0, | ||
| is_signer, | ||
| is_writable, | ||
| executable: false, | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_account_loader_new() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
| assert_eq!(*loader.key(), key); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deref_provides_account_info_access() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
|
|
||
| // Deref should provide access to AccountInfo fields | ||
| assert!(loader.is_writable); | ||
| assert!(!loader.is_signer); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_load_fails_for_wrong_owner() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let wrong_owner = SolanaPubkey::new_unique(); // Not Light Token Program | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = | ||
| create_mock_account_info(&key, &wrong_owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
|
|
||
| let result = loader.load(); | ||
| assert!(matches!(result, Err(TokenError::InvalidMintOwner))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_load_fails_for_uninitialized() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| // Create data with is_initialized = 0 (uninitialized) | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
|
|
||
| let result = loader.load(); | ||
| // Will fail during validation | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_account_metas_writable() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
|
|
||
| let metas = loader.to_account_metas(None); | ||
| assert_eq!(metas.len(), 1); | ||
| assert_eq!(metas[0].pubkey, key); | ||
| assert!(metas[0].is_writable); | ||
| assert!(!metas[0].is_signer); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_account_metas_readonly() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, false, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
|
|
||
| let metas = loader.to_account_metas(None); | ||
| assert_eq!(metas.len(), 1); | ||
| assert!(!metas[0].is_writable); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_key_trait() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
| assert_eq!(anchor_lang::Key::key(&loader), key); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_initialized_false_for_wrong_owner() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let wrong_owner = SolanaPubkey::new_unique(); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
| // Set is_initialized byte to 1 | ||
| data[IS_INITIALIZED_OFFSET] = 1; | ||
|
|
||
| let info = | ||
| create_mock_account_info(&key, &wrong_owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
| assert!(!loader.is_initialized()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_initialized_false_for_zero_byte() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
| // is_initialized byte is 0 | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
| assert!(!loader.is_initialized()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_initialized_true() { | ||
| let key = SolanaPubkey::new_unique(); | ||
| let owner = SolanaPubkey::new_from_array(LIGHT_TOKEN_PROGRAM_ID); | ||
| let mut lamports = 1_000_000u64; | ||
| let mut data = vec![0u8; 256]; | ||
| // Set is_initialized byte to 1 | ||
| data[IS_INITIALIZED_OFFSET] = 1; | ||
|
|
||
| let info = create_mock_account_info(&key, &owner, &mut lamports, &mut data, true, false); | ||
|
|
||
| let loader: AccountLoader<'_, Mint> = AccountLoader::new(info); | ||
| assert!(loader.is_initialized()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good test coverage!
The tests cover construction, trait implementations, error cases, and the is_initialized quick check. Consider adding a test for load_mut() success path and a test verifying that calling load() twice works (since each creates a fresh view).
🤖 Prompt for AI Agents
In `@program-libs/token-interface/src/state/mint/anchor_wrapper.rs` around lines
288 - 469, Add two unit tests: one that verifies AccountLoader::load_mut
successfully returns a mutable Mint when account owner and data indicate a
valid, initialized Mint (set IS_INITIALIZED_OFFSET = 1 and populate any required
fields) and that modifications through the returned &mut Mint persist in the
wrapped AccountInfo data; and another that calls AccountLoader::load() twice on
the same AccountLoader instance (or two fresh AccountLoader::new(info) views if
necessary) to assert both calls succeed and return equivalent Mint values,
ensuring repeated reads are supported. Locate tests near the existing test
module and reference AccountLoader::load_mut and AccountLoader::load (and
IS_INITIALIZED_OFFSET, Mint) when creating the mock AccountInfo.
Comment on lines
+404
to
+427
| /// IDL generation method required by Anchor's #[program] macro. | ||
| impl #impl_generics #struct_name #ty_generics #where_clause { | ||
| pub fn __anchor_private_gen_idl_accounts( | ||
| _accounts: &mut std::collections::BTreeMap<String, anchor_lang::idl::types::IdlAccount>, | ||
| _types: &mut std::collections::BTreeMap<String, anchor_lang::idl::types::IdlTypeDef>, | ||
| ) -> Vec<anchor_lang::idl::types::IdlInstructionAccountItem> { | ||
| vec![ | ||
| #( | ||
| anchor_lang::idl::types::IdlInstructionAccountItem::Single( | ||
| anchor_lang::idl::types::IdlInstructionAccount { | ||
| name: stringify!(#field_names).into(), | ||
| docs: vec![], | ||
| writable: false, | ||
| signer: false, | ||
| optional: false, | ||
| address: None, | ||
| pda: None, | ||
| relations: vec![], | ||
| } | ||
| ) | ||
| ),* | ||
| ] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
IDL metadata loses field mutability and signer information.
The generated IdlInstructionAccount hardcodes writable: false and signer: false for all fields, but you have this metadata available in ParsedField.is_mut and ParsedField.is_signer. Clients relying on IDL to construct transactions may incorrectly mark accounts as read-only.
🔧 Proposed fix to use actual field metadata
- anchor_lang::idl::types::IdlInstructionAccountItem::Single(
- anchor_lang::idl::types::IdlInstructionAccount {
- name: stringify!(`#field_names`).into(),
- docs: vec![],
- writable: false,
- signer: false,
- optional: false,
- address: None,
- pda: None,
- relations: vec![],
- }
- )
+ anchor_lang::idl::types::IdlInstructionAccountItem::Single(
+ anchor_lang::idl::types::IdlInstructionAccount {
+ name: stringify!(`#field_names`).into(),
+ docs: vec![],
+ writable: `#field_is_mut`,
+ signer: `#field_is_signer`,
+ optional: false,
+ address: None,
+ pda: None,
+ relations: vec![],
+ }
+ )You'll need to collect the is_mut and is_signer values alongside field_names:
let field_metadata: Vec<_> = fields.iter().map(|f| {
let name = &f.ident;
let is_mut = f.is_mut;
let is_signer = f.is_signer;
(name, is_mut, is_signer)
}).collect();🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/accounts/builder.rs` around lines 404 - 427,
The IDL generator (__anchor_private_gen_idl_accounts) currently hardcodes
writable: false and signer: false for every field; update the code that builds
the account list to collect per-field metadata (e.g., use the ParsedField
values: ParsedField.is_mut and ParsedField.is_signer alongside field_names) and
emit each IdlInstructionAccount with writable set to is_mut and signer set to
is_signer; locate the vector construction that iterates over `#field_names` and
change it to iterate over a collected (name, is_mut, is_signer) tuple so each
IdlInstructionAccount reflects the real mutability and signer flags.
Comment on lines
+204
to
+220
| /// Check if field has `#[account(mut)]` attribute. | ||
| fn has_mut_attribute(attrs: &[syn::Attribute]) -> bool { | ||
| for attr in attrs { | ||
| if attr.path().is_ident("account") { | ||
| // Parse the attribute content to look for `mut` | ||
| let tokens = match &attr.meta { | ||
| syn::Meta::List(list) => list.tokens.clone(), | ||
| _ => continue, | ||
| }; | ||
| let token_str = tokens.to_string(); | ||
| if token_str.contains("mut") { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
String-based mut detection may produce false positives.
The token_str.contains("mut") check at line 214 will match any occurrence of "mut" in the attribute content, including:
#[account(seeds = [b"immutable"])]#[account(constraint = compute_mutable(...))]
Consider checking for mut as a standalone token or at word boundaries.
🔧 Proposed fix using word boundary matching
fn has_mut_attribute(attrs: &[syn::Attribute]) -> bool {
for attr in attrs {
if attr.path().is_ident("account") {
// Parse the attribute content to look for `mut`
let tokens = match &attr.meta {
syn::Meta::List(list) => list.tokens.clone(),
_ => continue,
};
let token_str = tokens.to_string();
- if token_str.contains("mut") {
- return true;
- }
+ // Check for `mut` as a standalone token (not part of another word)
+ // Handles: mut, mut), (mut, etc.
+ for token in token_str.split(|c: char| !c.is_alphanumeric() && c != '_') {
+ if token == "mut" {
+ return true;
+ }
+ }
}
}
false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Check if field has `#[account(mut)]` attribute. | |
| fn has_mut_attribute(attrs: &[syn::Attribute]) -> bool { | |
| for attr in attrs { | |
| if attr.path().is_ident("account") { | |
| // Parse the attribute content to look for `mut` | |
| let tokens = match &attr.meta { | |
| syn::Meta::List(list) => list.tokens.clone(), | |
| _ => continue, | |
| }; | |
| let token_str = tokens.to_string(); | |
| if token_str.contains("mut") { | |
| return true; | |
| } | |
| } | |
| } | |
| false | |
| } | |
| /// Check if field has `#[account(mut)]` attribute. | |
| fn has_mut_attribute(attrs: &[syn::Attribute]) -> bool { | |
| for attr in attrs { | |
| if attr.path().is_ident("account") { | |
| // Parse the attribute content to look for `mut` | |
| let tokens = match &attr.meta { | |
| syn::Meta::List(list) => list.tokens.clone(), | |
| _ => continue, | |
| }; | |
| let token_str = tokens.to_string(); | |
| // Check for `mut` as a standalone token (not part of another word) | |
| // Handles: mut, mut), (mut, etc. | |
| for token in token_str.split(|c: char| !c.is_alphanumeric() && c != '_') { | |
| if token == "mut" { | |
| return true; | |
| } | |
| } | |
| } | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/accounts/parse.rs` around lines 204 - 220, The
current has_mut_attribute uses token_str.contains("mut") which can produce false
positives; instead parse the attribute's Meta::List and iterate its list.nested
items, and treat the attribute as mut only when you find a NestedMeta that is
exactly the identifier "mut" (e.g., match
NestedMeta::Meta(syn::Meta::Path(path)) and path.is_ident("mut")). Update
has_mut_attribute to ignore other nested meta forms (like key = value or
function calls) and only return true for a standalone mut NestedMeta so you
don't match substrings inside other tokens.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notes:
UncheckedAccountorAccountInfoorAccountLoader.LightMintor a customAccountLoader<Mint>only by replacing anchorsAccountsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.