refactor: light account creation to generic function#2287
Open
ananas-block wants to merge 4 commits intomainfrom
Open
refactor: light account creation to generic function#2287ananas-block wants to merge 4 commits intomainfrom
ananas-block wants to merge 4 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR centralizes token/mint/ATA/PDA creation into a new SDK API Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Macro as Macro (codegen)
participant SDK as SDK::create_accounts
participant CPI as CPI Context / Programs
participant Storage as Accounts (PDAs, Mints, ATAs)
rect rgba(100,149,237,0.5)
Macro->>SDK: build PdaInitParam / CreateMintsInput / TokenInitParam / AtaInitParam
end
rect rgba(34,139,34,0.5)
SDK->>CPI: invoke create_pdas / create_mints / create_token_vaults / create_atas
CPI->>Storage: create or initialize accounts (system_program, token_program, metadata, rent)
CPI-->>SDK: return creation results / errors
end
SDK-->>Macro: return success flag or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/account/src/lib.rs`:
- Around line 226-230: The re-export of
light_sdk_types::interface::accounts::create_accounts is currently gated only by
feature "token" but the symbol requires both "token" and "cpi-context"; update
the cfg on the pub use (the create_accounts, AtaInitParam, CreateMintsInput,
PdaInitParam, SharedAccounts, TokenInitParam re-export) to require both features
(e.g., cfg(all(feature = "token", feature = "cpi-context"))) so consumers
enabling only "token" won't hit a missing-symbol compile error; alternatively,
if you prefer feature wiring, add a Cargo feature dependency ensuring "token"
enables "cpi-context" in this crate's Cargo.toml.
In `@sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs`:
- Around line 141-309: The PDAS to u8 casts can overflow silently: add an
explicit guard that fails when PDAS or the runtime pdas.len() would exceed 255
before any cast; for example, in the main function before calling
create_mints_inner::<AI, MINTS> and before the PDA preparation loop check that
PDAS <= 255 and pdas.len() <= 255 (or that i < 256) and return
Err(LightSdkTypesError::InvalidInstructionData) if not; ensure you remove or
keep casts only after the checks so the uses of PDAS as u8 (the
create_mints_inner::<AI, MINTS>(..., PDAS as u8) call) and i as u8 passed into
prepare_compressed_account_on_init are safe.
Comment on lines
+226
to
+230
| // create_accounts SDK function and parameter types | ||
| #[cfg(feature = "token")] | ||
| pub use light_sdk_types::interface::accounts::create_accounts::{ | ||
| create_accounts, AtaInitParam, CreateMintsInput, PdaInitParam, SharedAccounts, TokenInitParam, | ||
| }; |
Contributor
There was a problem hiding this comment.
Align create_accounts re-export gating with sdk-types.
light_sdk_types::interface::accounts::create_accounts only exists when both cpi-context and token features are enabled, but this re-export is gated on token alone. If a consumer enables token without cpi-context, this will fail to compile. Gate this re-export with both features or make token depend on cpi-context in Cargo features.
Run this to confirm the feature dependency chain:
#!/bin/bash
set -euo pipefail
# Scan all Cargo.toml files for feature wiring
fd -a 'Cargo.toml' | while read -r f; do
if rg -n "cpi-context|token" "$f" >/dev/null; then
echo "---- $f ----"
rg -n "cpi-context|token" "$f"
fi
done🤖 Prompt for AI Agents
In `@sdk-libs/account/src/lib.rs` around lines 226 - 230, The re-export of
light_sdk_types::interface::accounts::create_accounts is currently gated only by
feature "token" but the symbol requires both "token" and "cpi-context"; update
the cfg on the pub use (the create_accounts, AtaInitParam, CreateMintsInput,
PdaInitParam, SharedAccounts, TokenInitParam re-export) to require both features
(e.g., cfg(all(feature = "token", feature = "cpi-context"))) so consumers
enabling only "token" won't hit a missing-symbol compile error; alternatively,
if you prefer feature wiring, add a Cargo feature dependency ensuring "token"
enables "cpi-context" in this crate's Cargo.toml.
Replace manual CPI orchestration in pda, account_loader, and two_mints test modules with the unified create_accounts() generic function, reducing boilerplate across both pinocchio and anchor manual tests.
…ccounts() Replace manual CPI orchestration with the unified create_accounts() function across all 5 migratable processors (pda, account_loader, mint, two_mints, all).
Fail fast if PDAS or MINTS exceed u8::MAX, preventing silent wrapping in downstream `as u8` casts for cpi_context_offset and account indices.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs`:
- Around line 168-185: The code constructs a CpiAccounts (via
CpiAccounts::new_with_config and CpiAccountsConfig::new/_with_cpi_context) even
when PDAS == 0 and MINTS == 0 and the resulting cpi_accounts is never used by
create_token_vaults or create_atas; avoid the unnecessary validation/slicing by
guarding construction: check the conditions (e.g., if shared.proof.PDAS > 0 ||
shared.proof.MINTS > 0) before creating CpiAccounts and only call
CpiAccounts::new_with_config when needed, otherwise skip building cpi_accounts
(or make it an Option) and propagate that to callers so unused
allocation/validation is not performed; refer to symbols CpiAccounts,
CpiAccountsConfig, create_token_vaults, create_atas, remaining_accounts, and
shared.proof.system_accounts_offset when applying the change.
- Around line 105-140: In create_accounts, the pda_setup closure parameter is
silently ignored when the const generic PDAS == 0; add a short runtime/debug
assertion and/or an inline comment just before the PDAS > 0 check to document
and assert this behavior so callers aren’t surprised (reference the
create_accounts function, the pda_setup parameter, and the PDAS const generic);
specifically, add a brief debug_assert or explicit comment indicating “pda_setup
is intentionally not called when PDAS == 0” and consume/drop the closure in that
branch if needed to avoid unused-value warnings.
- Around line 396-417: Lift the duplicate CreateTokenAtaCpi construction out of
the conditional: inside the atas loop build a single CreateTokenAtaCpi instance
using shared.fee_payer, ata.owner, ata.mint, ata.ata, then if ata.idempotent
call .idempotent() on that instance else keep the base instance, and finally
call .rent_free(compressible_config, rent_sponsor, system_program).invoke()?;
this preserves the type-state flow of CreateTokenAtaCpi ->
CreateTokenAtaCpiIdempotent while removing duplicated struct construction and
keeping .rent_free()/.invoke() applied to the resulting value.
Comment on lines
+105
to
+140
| /// Create compressed PDAs, mints, token vaults, and ATAs in a single instruction. | ||
| /// | ||
| /// Returns `true` if CPI context was used (PDAS > 0 && MINTS > 0), `false` otherwise. | ||
| /// | ||
| /// # Const Generics | ||
| /// | ||
| /// - `PDAS`: Number of compressed PDAs to register. | ||
| /// - `MINTS`: Number of compressed mints to create via `CreateMints`. | ||
| /// - `TOKENS`: Number of PDA token vaults to create via `CreateTokenAccountCpi`. | ||
| /// - `ATAS`: Number of ATAs to create via `CreateTokenAtaCpi`. | ||
| /// | ||
| /// # Type Parameters | ||
| /// | ||
| /// - `AI`: Account info type (`AccountInfoTrait`). | ||
| /// - `F`: Closure called after all PDAs are prepared, before CPI context write. | ||
| /// Signature: `FnOnce(&LightConfig, u64) -> Result<(), LightSdkTypesError>`. | ||
| /// The closure receives the loaded `LightConfig` and current slot. | ||
| /// When `PDAS = 0`, pass `|_, _| Ok(())`. | ||
| #[inline(never)] | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn create_accounts< | ||
| AI: AccountInfoTrait + Clone, | ||
| const PDAS: usize, | ||
| const MINTS: usize, | ||
| const TOKENS: usize, | ||
| const ATAS: usize, | ||
| F: FnOnce(&LightConfig, u64) -> Result<(), LightSdkTypesError>, | ||
| >( | ||
| pdas: [PdaInitParam<'_, AI>; PDAS], | ||
| pda_setup: F, | ||
| mints: Option<CreateMintsInput<'_, AI, MINTS>>, | ||
| tokens: [TokenInitParam<'_, AI>; TOKENS], | ||
| atas: [AtaInitParam<'_, AI>; ATAS], | ||
| shared: &SharedAccounts<'_, AI>, | ||
| remaining_accounts: &[AI], | ||
| ) -> Result<bool, LightSdkTypesError> { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
API ergonomics: pda_setup closure is silently ignored when PDAS == 0.
The doc comment on line 122 says to pass |_, _| Ok(()) when PDAS = 0, which works. But this is a subtle API contract — a caller could pass a meaningful closure expecting it to run, and it would silently be dropped. Consider adding a brief debug_assert! or a compile-time note. Not a bug, just a teaching moment for future consumers.
// At line 190, before the PDAS > 0 check:
// Note: pda_setup is intentionally not called when PDAS == 0.
// The closure is consumed by value, so the compiler will drop it.This is purely documentation — the current behavior is correct.
🤖 Prompt for AI Agents
In `@sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs` around lines
105 - 140, In create_accounts, the pda_setup closure parameter is silently
ignored when the const generic PDAS == 0; add a short runtime/debug assertion
and/or an inline comment just before the PDAS > 0 check to document and assert
this behavior so callers aren’t surprised (reference the create_accounts
function, the pda_setup parameter, and the PDAS const generic); specifically,
add a brief debug_assert or explicit comment indicating “pda_setup is
intentionally not called when PDAS == 0” and consume/drop the closure in that
branch if needed to avoid unused-value warnings.
Comment on lines
+168
to
+185
| // ==================================================================== | ||
| // 2. Build CPI accounts | ||
| // ==================================================================== | ||
| let system_accounts_offset = shared.proof.system_accounts_offset as usize; | ||
| if remaining_accounts.len() < system_accounts_offset { | ||
| return Err(LightSdkTypesError::FewerAccountsThanSystemAccounts); | ||
| } | ||
|
|
||
| let config = if with_cpi_context { | ||
| CpiAccountsConfig::new_with_cpi_context(shared.cpi_signer) | ||
| } else { | ||
| CpiAccountsConfig::new(shared.cpi_signer) | ||
| }; | ||
| let cpi_accounts = CpiAccounts::new_with_config( | ||
| shared.fee_payer, | ||
| &remaining_accounts[system_accounts_offset..], | ||
| config, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CpiAccounts is constructed even when only TOKENS/ATAS are needed.
When PDAS == 0 and MINTS == 0, the cpi_accounts object is built (lines 181–185) but never consumed — create_token_vaults and create_atas don't use it. This isn't a bug, but it does perform unnecessary validation and slicing of remaining_accounts.
If you want to tighten things up, you could gate the construction:
💡 Optional: gate CpiAccounts construction
- let config = if with_cpi_context {
- CpiAccountsConfig::new_with_cpi_context(shared.cpi_signer)
- } else {
- CpiAccountsConfig::new(shared.cpi_signer)
- };
- let cpi_accounts = CpiAccounts::new_with_config(
- shared.fee_payer,
- &remaining_accounts[system_accounts_offset..],
- config,
- );
+ // CpiAccounts is only needed for PDAs and mints
+ let cpi_accounts = if PDAS > 0 || MINTS > 0 {
+ let config = if with_cpi_context {
+ CpiAccountsConfig::new_with_cpi_context(shared.cpi_signer)
+ } else {
+ CpiAccountsConfig::new(shared.cpi_signer)
+ };
+ Some(CpiAccounts::new_with_config(
+ shared.fee_payer,
+ &remaining_accounts[system_accounts_offset..],
+ config,
+ ))
+ } else {
+ None
+ };This would require adjusting callers to unwrap, so it's a tradeoff of precision vs. simplicity. Totally optional.
🤖 Prompt for AI Agents
In `@sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs` around lines
168 - 185, The code constructs a CpiAccounts (via CpiAccounts::new_with_config
and CpiAccountsConfig::new/_with_cpi_context) even when PDAS == 0 and MINTS == 0
and the resulting cpi_accounts is never used by create_token_vaults or
create_atas; avoid the unnecessary validation/slicing by guarding construction:
check the conditions (e.g., if shared.proof.PDAS > 0 || shared.proof.MINTS > 0)
before creating CpiAccounts and only call CpiAccounts::new_with_config when
needed, otherwise skip building cpi_accounts (or make it an Option) and
propagate that to callers so unused allocation/validation is not performed;
refer to symbols CpiAccounts, CpiAccountsConfig, create_token_vaults,
create_atas, remaining_accounts, and shared.proof.system_accounts_offset when
applying the change.
Comment on lines
+396
to
+417
| for ata in atas { | ||
| if ata.idempotent { | ||
| CreateTokenAtaCpi { | ||
| payer: shared.fee_payer, | ||
| owner: ata.owner, | ||
| mint: ata.mint, | ||
| ata: ata.ata, | ||
| } | ||
| .idempotent() | ||
| .rent_free(compressible_config, rent_sponsor, system_program) | ||
| .invoke()?; | ||
| } else { | ||
| CreateTokenAtaCpi { | ||
| payer: shared.fee_payer, | ||
| owner: ata.owner, | ||
| mint: ata.mint, | ||
| ata: ata.ata, | ||
| } | ||
| .rent_free(compressible_config, rent_sponsor, system_program) | ||
| .invoke()?; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, find where CreateTokenAtaCpi is defined
find . -type f -name "*.rs" | head -20Repository: Lightprotocol/light-protocol
Length of output: 1013
🏁 Script executed:
# Search for CreateTokenAtaCpi definition
rg "struct CreateTokenAtaCpi" -A 10Repository: Lightprotocol/light-protocol
Length of output: 4062
🏁 Script executed:
# Search for the idempotent method implementation
rg "fn idempotent\(" -B 2 -A 5 --type rsRepository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
# Check the actual file at the specified lines
cat -n sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs | sed -n '396,420p'Repository: Lightprotocol/light-protocol
Length of output: 931
🏁 Script executed:
# Check if CreateTokenAtaCpi base struct has rent_free method
rg "impl.*CreateTokenAtaCpi" -A 30 sdk-libs/sdk-types/src/interface/cpi/create_token_accounts.rsRepository: Lightprotocol/light-protocol
Length of output: 1866
🏁 Script executed:
# Get more context from the create_token_accounts.rs file to see all implementations
cat -n sdk-libs/sdk-types/src/interface/cpi/create_token_accounts.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 4180
🏁 Script executed:
# Check for CLAUDE.md files in sdk-libs and programs directories
find . -name "CLAUDE.md" -type f | grep -E "(sdk-libs|programs)"Repository: Lightprotocol/light-protocol
Length of output: 307
🏁 Script executed:
# Check CLAUDE.md files for guidance on CPI patterns
cat programs/compressed-token/program/CLAUDE.md | grep -A 5 -B 5 -i "cpi\|builder\|pattern"Repository: Lightprotocol/light-protocol
Length of output: 1317
🏁 Script executed:
# Check if there are any other similar patterns in the codebase for guidance
rg "CreateTokenAtaCpi" -B 2 -A 10 sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs | head -60Repository: Lightprotocol/light-protocol
Length of output: 1897
Simplify duplicated ATA initialization by moving struct construction outside the conditional.
The idempotent and non-idempotent branches construct CreateTokenAtaCpi identically, differing only in whether .idempotent() is called. Both paths ultimately converge to CreateTokenAtaRentFreeCpi (via .rent_free() on either the base CreateTokenAtaCpi or the CreateTokenAtaCpiIdempotent wrapper), so the struct construction can be lifted outside:
♻️ Suggested refactor
for ata in atas {
- if ata.idempotent {
- CreateTokenAtaCpi {
- payer: shared.fee_payer,
- owner: ata.owner,
- mint: ata.mint,
- ata: ata.ata,
- }
- .idempotent()
- .rent_free(compressible_config, rent_sponsor, system_program)
- .invoke()?;
- } else {
- CreateTokenAtaCpi {
- payer: shared.fee_payer,
- owner: ata.owner,
- mint: ata.mint,
- ata: ata.ata,
- }
- .rent_free(compressible_config, rent_sponsor, system_program)
- .invoke()?;
- }
+ let cpi = CreateTokenAtaCpi {
+ payer: shared.fee_payer,
+ owner: ata.owner,
+ mint: ata.mint,
+ ata: ata.ata,
+ };
+ if ata.idempotent {
+ cpi.idempotent()
+ .rent_free(compressible_config, rent_sponsor, system_program)
+ .invoke()?;
+ } else {
+ cpi.rent_free(compressible_config, rent_sponsor, system_program)
+ .invoke()?;
+ }
}The type-state pattern is preserved: .idempotent() returns a wrapper type that carries the idempotent flag into rent_free(), ensuring compile-time safety while keeping the code DRY.
📝 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
| for ata in atas { | |
| if ata.idempotent { | |
| CreateTokenAtaCpi { | |
| payer: shared.fee_payer, | |
| owner: ata.owner, | |
| mint: ata.mint, | |
| ata: ata.ata, | |
| } | |
| .idempotent() | |
| .rent_free(compressible_config, rent_sponsor, system_program) | |
| .invoke()?; | |
| } else { | |
| CreateTokenAtaCpi { | |
| payer: shared.fee_payer, | |
| owner: ata.owner, | |
| mint: ata.mint, | |
| ata: ata.ata, | |
| } | |
| .rent_free(compressible_config, rent_sponsor, system_program) | |
| .invoke()?; | |
| } | |
| } | |
| for ata in atas { | |
| let cpi = CreateTokenAtaCpi { | |
| payer: shared.fee_payer, | |
| owner: ata.owner, | |
| mint: ata.mint, | |
| ata: ata.ata, | |
| }; | |
| if ata.idempotent { | |
| cpi.idempotent() | |
| .rent_free(compressible_config, rent_sponsor, system_program) | |
| .invoke()?; | |
| } else { | |
| cpi.rent_free(compressible_config, rent_sponsor, system_program) | |
| .invoke()?; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/sdk-types/src/interface/accounts/create_accounts.rs` around lines
396 - 417, Lift the duplicate CreateTokenAtaCpi construction out of the
conditional: inside the atas loop build a single CreateTokenAtaCpi instance
using shared.fee_payer, ata.owner, ata.mint, ata.ata, then if ata.idempotent
call .idempotent() on that instance else keep the base instance, and finally
call .rent_free(compressible_config, rent_sponsor, system_program).invoke()?;
this preserves the type-state flow of CreateTokenAtaCpi ->
CreateTokenAtaCpiIdempotent while removing duplicated struct construction and
keeping .rent_free()/.invoke() applied to the resulting value.
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.
Summary
Refactors all account creation (PDAs, mints, tokens, ATAs) into a single generic
create_accounts()SDK function, replacing multiple separate code generation paths in the macro with one unified call.Changes
New
create_accounts()generic SDK function - Added unified SDK function insdk-libs/sdk-types/src/interface/accounts/create_accounts.rs(417 lines) for creating PDAs, mints, tokens, and ATAs in a single call with type-safe generic parameters.Export create_accounts in public SDKs - Added re-exports of
create_accounts,PdaInitParam,CreateMintsInput,TokenInitParam,AtaInitParam, andSharedAccountstosdk-libs/account/src/lib.rsandsdk-libs/account-pinocchio/src/lib.rsunder#[cfg(feature = "token")].Macro refactor: Single unified pre_init path - Replaced multiple separate code generation paths (
generate_pre_init_pdas_only(),generate_pre_init_pdas_and_mints(), etc.) with a singlegenerate_pre_init_all()that delegates all account creation to the genericcreate_accounts()function.PDA setup closure now includes serialization - Modified
generate_pda_setup_closure_body()to serialize account data afterset_decompressed()for both boxed and regular Anchor accounts (matching old PDA code pattern).Removed dead token generation code - Deleted
sdk-libs/macros/src/light_pdas/accounts/token.rs(279 lines) as token creation is now handled by genericcreate_accounts().Cleaned up mint field parsing - Removed unused
address_tree_infofield fromLightMintFieldstruct, removeddirect_proof_argparameter frombuild_mint_field(), and simplified mint parsing.Indexed bump slice identifiers - Fixed
__mint_bump_sliceand__auth_bump_sliceto be per-mint indexed (__mint_bump_slice_{idx},__auth_bump_slice_{idx}) to avoid shadowing across multiple mints.Updated manual tests - Simplified test implementations in
anchor-manual-testandpinocchio-manual-testto use newcreate_accounts()function instead of separate mint/PDA/token code paths.Fixed clippy warnings - Collapsed nested
ifstatement increate_accounts.rsand removed unnecessary.clone()calls onAccountInfo(Copy type) in manual tests.Test plan
cargo test -p light-sdk-macros(310 unit tests)cargo test-sbf -p csdk-anchor-full-derived-test(77 tests)cargo test-sbf -p single-pda-testcargo test-sbf -p single-mint-test./scripts/format.sh(lint clean)Summary by CodeRabbit