refactor: compressed token sdk transfer interface builder#2064
Conversation
WalkthroughSystematic refactor of the compressed token SDK's transfer instruction APIs, replacing function-based constructors with struct-driven patterns. Updates transfer_ctoken and transfer_interface modules to use new types like TransferCtoken, TransferSplToCtoken, TransferCtokenToSpl, and CtokenToSplTransferAndClose with corresponding AccountInfos structs and method-based invocation patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransferStruct as Transfer Struct
participant AccountInfos as AccountInfos Struct
participant Instruction as Instruction Builder
rect rgb(220, 245, 220)
Note over Client,Instruction: New Struct-Based Flow
Client->>AccountInfos: new(accounts...)
Client->>TransferStruct: From(&AccountInfos)
TransferStruct->>Instruction: instruction()
Instruction-->>Client: Instruction
Client->>AccountInfos: invoke(signer_seeds?)
AccountInfos->>TransferStruct: From(self)
TransferStruct->>Instruction: instruction()
Instruction->>Client: Result<(), ProgramError>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs (2)
187-199: Same critical account mismatch issue.This
invoke()method has the same problem asTransferSplToCtokenAccountInfos::invoke(). Thecompressed_token_program_authorityis in theaccount_infosarray but likely not in the instruction's accounts.
329-341: Same critical account mismatch issue (transfer and close variant).This
invoke()method also includescompressed_token_program_authorityinaccount_infoswithout corresponding inclusion in the instruction accounts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
program-tests/compressed-token-test/tests/transfer2/spl_ctoken.rsis excluded by none and included by none
📒 Files selected for processing (5)
sdk-libs/compressed-token-sdk/src/instructions/mod.rs(1 hunks)sdk-libs/compressed-token-sdk/src/instructions/transfer_ctoken.rs(1 hunks)sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs(2 hunks)sdk-libs/token-client/src/actions/transfer2/ctoken_to_spl.rs(2 hunks)sdk-libs/token-client/src/actions/transfer2/spl_to_ctoken.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
sdk-libs/compressed-token-sdk/src/instructions/transfer_ctoken.rs (1)
sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs (17)
instruction(48-50)instruction(100-154)instruction(183-185)instruction(236-296)instruction(325-327)instruction(378-441)from(84-96)from(219-231)from(361-373)invoke(52-65)invoke(187-200)invoke(329-342)invoke(522-586)invoke_signed(67-80)invoke_signed(202-215)invoke_signed(344-357)invoke_signed(592-656)
sdk-libs/token-client/src/actions/transfer2/spl_to_ctoken.rs (2)
sdk-libs/compressed-token-sdk/src/token_pool.rs (1)
find_token_pool_pda_with_index(20-28)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/compressed-token-sdk/src/instructions/mod.rs (1)
sdk-libs/token-client/src/actions/ctoken_transfer.rs (1)
transfer_ctoken(20-38)
sdk-libs/token-client/src/actions/transfer2/ctoken_to_spl.rs (2)
sdk-libs/compressed-token-sdk/src/token_pool.rs (1)
find_token_pool_pda_with_index(20-28)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs (5)
sdk-libs/token-client/src/actions/ctoken_transfer.rs (1)
transfer_ctoken(20-38)sdk-libs/compressed-token-sdk/src/instructions/transfer_ctoken.rs (5)
instruction(23-25)instruction(52-67)from(41-48)invoke(27-31)invoke_signed(33-37)sdk-libs/compressed-token-sdk/src/account2.rs (5)
mint(362-364)compress_spl(193-220)decompress_ctoken(224-247)compress_ctoken(169-190)decompress_spl(250-279)sdk-libs/compressed-token-sdk/src/instructions/transfer2/instruction.rs (1)
create_transfer2_instruction(71-140)sdk-libs/compressed-token-sdk/src/utils.rs (1)
is_ctoken_account(22-37)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: stateless-js-v2
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
🔇 Additional comments (8)
sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs (3)
387-387: Correct mutability for close operation.Good catch making the authority writable in the close variant. This is correct since the account is being closed and rent is refunded to the authority (as indicated by the comment on line 405: "destination is authority"). This differs appropriately from the non-close variant at line 245 which uses
new_readonly.
538-540: Good error handling for missing SPL bridge config.The error handling correctly catches the case where SPL bridge config is required but not provided. The
IncompleteSplBridgeConfigerror variant appropriately describes the issue.
119-127: Review contains inaccuracy regarding TODO comment locations.The review states that hardcoded
pool_index: 0appears at lines 125, 275, and 420 "with TODO comments," but this is partially incorrect:
- Line 125 (compress_spl): Hardcoded
0, no TODO comment- Line 275 (decompress_spl): Hardcoded
0, has "(TODO: make dynamic)" comment ✓- Line 420 (decompress_spl): Hardcoded
0, has "(TODO: make dynamic)" comment ✓The parameter mapping for
compress_splis correct. However, the characterization that all three locations have TODO comments is inaccurate—only the decompress operations flagged this concern. Additionally, intransfer2.rs,compress_splis called with dynamicpool_indexvalues from input data, suggesting the single-pool design intransfer_interface.rsmay be intentional for this code path.Likely an incorrect or invalid review comment.
sdk-libs/compressed-token-sdk/src/instructions/transfer_ctoken.rs (2)
27-30: Correct account ordering in invoke().Unlike the transfer_interface variants, this implementation correctly matches the accounts passed to
invoke()with the accounts in the instruction (lines 55-59). The three accounts (source, destination, authority) are consistent between both.
60-65: Verify C-token instruction data format (double discriminator).The instruction is serialized as [3u8, 3u8, <u64_le>] (two 3u8 bytes then amount). Other in-repo usages use a single-byte discriminator (e.g. sdk-libs/token-client/src/actions/ctoken_transfer.rs: vec![3u8] + amount; transfer2/withdraw/claim also use single-byte discriminators). This mismatch suggests the extra data.push(3u8) is likely incorrect — remove it or confirm it is required by the on‑chain C_TOKEN program ABI (or light_ctoken_types) before merging.
Locations: sdk-libs/compressed-token-sdk/src/instructions/transfer_ctoken.rs (60–65) — compare sdk-libs/token-client/src/actions/ctoken_transfer.rs (68–71) and sdk-libs/compressed-token-sdk/src/instructions/transfer2/instruction.rs (128–131).
sdk-libs/token-client/src/actions/transfer2/spl_to_ctoken.rs (1)
37-49: Good adoption of the new struct-based API.The transition from the old function-based API to the struct-based
TransferSplToCtokenbuilder pattern is clean and consistent. The use of.pubkey()to extract public keys from keypairs is appropriate, and the error handling correctly convertsProgramErrortoRpcError.The TODO comment on line 46 about making the SPL token program dynamic is appropriate for tracking future work to support Token-2022.
sdk-libs/token-client/src/actions/transfer2/ctoken_to_spl.rs (1)
26-38: Consistent with the spl_to_ctoken implementation.This follows the same clean pattern as the SPL-to-ctoken transfer. The struct construction, error handling, and TODO comment are all consistent with the other direction of transfer.
sdk-libs/compressed-token-sdk/src/instructions/mod.rs (1)
50-55: Public API exports correctly updated.The module exports have been properly updated to expose the new struct-based API:
TransferCtokenandTransferCtokenAccountInfosfrom transfer_ctoken- All the new transfer interface types including
TransferSplToCtoken,TransferCtokenToSpl,CtokenToSplTransferAndClose, and their correspondingAccountInfosvariants- The new
SplBridgeConfigandTransferInterfaceorchestrator typesThis aligns with the implementation changes in the other files.
| pub fn invoke(self) -> Result<(), ProgramError> { | ||
| let instruction = TransferSplToCtoken::from(&self).instruction()?; | ||
| let account_infos = [ | ||
| self.payer, | ||
| self.compressed_token_program_authority, | ||
| self.mint, // Index 0: Mint | ||
| self.destination_ctoken_account, // Index 1: Destination owner | ||
| self.authority, // Index 2: Authority (signer) | ||
| self.source_spl_token_account, // Index 3: Source SPL token account | ||
| self.token_pool_pda, // Index 4: Token pool PDA | ||
| self.spl_token_program, // Index 5: SPL Token program | ||
| ]; | ||
| invoke(&instruction, &account_infos) |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Account mismatch between invoke() and instruction().
The account_infos array passed to invoke() includes payer and compressed_token_program_authority at indices 0 and 1, but the instruction created by TransferSplToCtoken::instruction() uses Transfer2AccountsMetaConfig::new_decompressed_accounts_only(self.payer, packed_accounts) which only receives the payer as a separate parameter. The compressed_token_program_authority account doesn't appear to be added to the instruction's accounts by the create_transfer2_instruction path.
This mismatch will cause the instruction to fail at runtime because Solana requires the accounts passed to invoke() to match the accounts in the instruction.
Verify the account ordering by running:
🏁 Script executed:
#!/bin/bash
# Search for Transfer2AccountsMetaConfig implementation to understand account ordering
rg -nP "impl.*Transfer2AccountsMetaConfig" -A 30
# Check how new_decompressed_accounts_only adds accounts to the instruction
rg -nP "fn new_decompressed_accounts_only" -A 20Length of output: 5713
🏁 Script executed:
#!/bin/bash
# Get full get_transfer2_instruction_account_metas implementation
rg -nP "pub fn get_transfer2_instruction_account_metas" -A 50
# Find TransferSplToCtoken and its instruction() method
rg -nP "impl.*TransferSplToCtoken" -A 5
rg -nP "fn instruction\(" -B 5 -A 15 --type=rs | head -100Length of output: 8520
🏁 Script executed:
#!/bin/bash
# Get full TransferSplToCtoken::instruction() implementation including packed_accounts
rg -nP "impl TransferSplToCtoken" -A 100 sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs | head -150
# Find create_transfer2_instruction to see how instruction is created
rg -nP "create_transfer2_instruction" -B 3 -A 10 sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rsLength of output: 6240
🏁 Script executed:
#!/bin/bash
# Get the complete get_transfer2_instruction_account_metas function to see what accounts are added
rg -nP "pub fn get_transfer2_instruction_account_metas" -A 100 sdk-libs/compressed-token-sdk/src/instructions/transfer2/account_metas.rs | tail -60Length of output: 2582
Critical: Account mismatch between invoke() and instruction() will cause runtime failure.
The instruction created by TransferSplToCtoken::instruction() constructs accounts as [CPI_AUTHORITY_PDA, payer, ...packed_accounts] (via get_transfer2_instruction_account_metas lines 110-122), but TransferSplToCtokenAccountInfos::invoke() passes [payer, compressed_token_program_authority, ...packed_accounts]. The ordering is inverted for the first two accounts, and compressed_token_program_authority is included in the invoke but never added to the instruction, while CPI_AUTHORITY_PDA is in the instruction but not in the invoke array. This mismatch will cause Solana to reject the transaction at runtime.
The same issue exists in TransferCtokenToSplAccountInfos::invoke() (lines 187-199).
🤖 Prompt for AI Agents
In sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs around
lines 52 to 64, the account order passed to invoke() doesn't match the account
metas produced by TransferSplToCtoken::instruction(): the instruction expects
CPI_AUTHORITY_PDA as the first account and payer second, but invoke() supplies
payer then compressed_token_program_authority (and omits CPI_AUTHORITY_PDA). Fix
by reordering and aligning the account_infos array to exactly match the
instruction's account metas: replace the first two entries with
CPI_AUTHORITY_PDA then payer (remove compressed_token_program_authority if it is
not part of the instruction), and ensure the rest of the packed_accounts keep
the same indices as the instruction. Apply the same correction in
TransferCtokenToSplAccountInfos::invoke() (around lines 187 to 199) so both
invoke calls match their respective instruction() account order and contents.
| /// # Arguments | ||
| /// * `source_account` - Source token account (can be ctoken or SPL) | ||
| /// * `destination_account` - Destination token account (can be ctoken or SPL) | ||
| /// * `authority` - Authority for the transfer (must be signer) | ||
| /// * `amount` - Amount to transfer | ||
| /// * `payer` - Payer for the transaction | ||
| /// * `compressed_token_program_authority` - Compressed token program authority | ||
| /// * `mint` - Optional mint account (required for SPL<->ctoken transfers) | ||
| /// * `spl_token_program` - Optional SPL token program (required for SPL<->ctoken transfers) | ||
| /// * `compressed_token_pool_pda` - Optional token pool PDA (required for SPL<->ctoken transfers) | ||
| /// * `compressed_token_pool_pda_bump` - Optional bump seed for token pool PDA | ||
| pub fn new( | ||
| source_account: AccountInfo<'info>, | ||
| destination_account: AccountInfo<'info>, | ||
| authority: AccountInfo<'info>, | ||
| amount: u64, | ||
| payer: AccountInfo<'info>, | ||
| compressed_token_program_authority: AccountInfo<'info>, | ||
| ) -> Self { | ||
| Self { | ||
| source_account, | ||
| destination_account, | ||
| authority, | ||
| amount, | ||
| signer_seeds, | ||
| ), | ||
|
|
||
| // ctoken -> spl: Requires bridge accounts | ||
| (true, false) => { | ||
| // Validate all required accounts are provided | ||
| let (mint_acct, spl_program, pool_pda, bump) = match ( | ||
| mint, | ||
| spl_token_program, | ||
| compressed_token_pool_pda, | ||
| compressed_token_pool_pda_bump, | ||
| ) { | ||
| (Some(m), Some(p), Some(pd), Some(b)) => (m, p, pd, b), | ||
| _ => { | ||
| return Err(ProgramError::Custom( | ||
| TokenSdkError::IncompleteSplBridgeConfig.into(), | ||
| )) | ||
| } | ||
| }; | ||
|
|
||
| transfer_ctoken_to_spl_signed( | ||
| payer.clone(), | ||
| authority.clone(), | ||
| source_account.clone(), | ||
| destination_account.clone(), | ||
| mint_acct.clone(), | ||
| spl_program.clone(), | ||
| pool_pda.clone(), | ||
| bump, | ||
| compressed_token_program_authority.clone(), | ||
| amount, | ||
| signer_seeds, | ||
| ) | ||
| payer, | ||
| compressed_token_program_authority, | ||
| spl_bridge_config: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for the TransferInterface API.
The TransferInterface struct and its constructor lack rustdoc comments. This is a public API that dynamically dispatches based on account types, so clear documentation would help users understand:
- When to use this vs the specific transfer types
- What happens when
spl_bridge_configis None vs Some - The runtime account type detection behavior
Add rustdoc comments like:
/// High-level transfer interface that automatically routes between ctoken and SPL transfers.
///
/// This type dynamically determines the transfer path based on the source and destination
/// account types (ctoken vs SPL). For transfers involving SPL accounts, you must provide
/// an `SplBridgeConfig` via `with_spl_bridge()` or `with_spl_bridge_config()`.
///
/// # Example
/// ```ignore
/// let transfer = TransferInterface::new(source, dest, authority, amount, payer, program_auth)
/// .with_spl_bridge(mint, spl_program, pool_pda, bump);
/// transfer.invoke()?;
/// ```
pub struct TransferInterface<'info> {| pub fn invoke(self) -> Result<(), ProgramError> { | ||
| let source_is_ctoken = is_ctoken_account(&self.source_account) | ||
| .map_err(|_| ProgramError::InvalidAccountData)?; | ||
| let dest_is_ctoken = is_ctoken_account(&self.destination_account) | ||
| .map_err(|_| ProgramError::InvalidAccountData)?; | ||
|
|
||
| match (source_is_ctoken, dest_is_ctoken) { | ||
| (true, true) => TransferCtokenAccountInfos { | ||
| source: self.source_account.clone(), | ||
| destination: self.destination_account.clone(), | ||
| amount: self.amount, | ||
| authority: self.authority.clone(), | ||
| } | ||
| .invoke(), | ||
|
|
||
| (true, false) => { | ||
| let config = self.spl_bridge_config.ok_or_else(|| { | ||
| ProgramError::Custom(TokenSdkError::IncompleteSplBridgeConfig.into()) | ||
| })?; | ||
|
|
||
| TransferCtokenToSplAccountInfos { | ||
| source_ctoken_account: self.source_account.clone(), | ||
| destination_spl_token_account: self.destination_account.clone(), | ||
| amount: self.amount, | ||
| authority: self.authority.clone(), | ||
| mint: config.mint.clone(), | ||
| payer: self.payer.clone(), | ||
| token_pool_pda: config.token_pool_pda.clone(), | ||
| token_pool_pda_bump: config.token_pool_pda_bump, | ||
| spl_token_program: config.spl_token_program.clone(), | ||
| compressed_token_program_authority: self | ||
| .compressed_token_program_authority | ||
| .clone(), | ||
| } | ||
| }; | ||
|
|
||
| transfer_spl_to_ctoken_signed( | ||
| payer.clone(), | ||
| authority.clone(), | ||
| source_account.clone(), | ||
| destination_account.clone(), | ||
| mint_acct.clone(), | ||
| spl_program.clone(), | ||
| pool_pda.clone(), | ||
| bump, | ||
| compressed_token_program_authority.clone(), | ||
| amount, | ||
| signer_seeds, | ||
| ) | ||
| .invoke() | ||
| } | ||
|
|
||
| (false, true) => { | ||
| let config = self.spl_bridge_config.ok_or_else(|| { | ||
| ProgramError::Custom(TokenSdkError::IncompleteSplBridgeConfig.into()) | ||
| })?; | ||
|
|
||
| TransferSplToCtokenAccountInfos { | ||
| source_spl_token_account: self.source_account.clone(), | ||
| to: self.destination_account.clone(), | ||
| amount: self.amount, | ||
| authority: self.authority.clone(), | ||
| mint: config.mint.clone(), | ||
| payer: self.payer.clone(), | ||
| token_pool_pda: config.token_pool_pda.clone(), | ||
| token_pool_pda_bump: config.token_pool_pda_bump, | ||
| spl_token_program: config.spl_token_program.clone(), | ||
| compressed_token_program_authority: self | ||
| .compressed_token_program_authority | ||
| .clone(), | ||
| destination_ctoken_account: self.destination_account.clone(), | ||
| } | ||
| .invoke() | ||
| } | ||
|
|
||
| (false, false) => Err(ProgramError::Custom( | ||
| TokenSdkError::UseRegularSplTransfer.into(), | ||
| )), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing AccountInfo clones.
The pattern of cloning AccountInfo instances throughout the match arms is safe but slightly redundant. While AccountInfo cloning is cheap (it's mostly references), you could construct the account info structs by moving/consuming the fields from self instead of cloning, since invoke() consumes self anyway.
For example:
(true, true) => TransferCtokenAccountInfos {
source: self.source_account, // move instead of clone
destination: self.destination_account,
amount: self.amount,
authority: self.authority,
}
.invoke(),This would apply to all match arms in both invoke() and invoke_signed().
🤖 Prompt for AI Agents
In sdk-libs/compressed-token-sdk/src/instructions/transfer_interface.rs around
lines 522-586, the code clones AccountInfo fields from self for each
Transfer*Infos construction; since invoke() consumes self, change these to move
the fields instead of cloning (e.g., use self.source_account,
self.destination_account, self.authority, self.amount, and take ownership of
self.spl_bridge_config when needed) so you stop performing redundant clones;
ensure you reorder/consume fields only once per match arm and apply the same
moving approach to invoke_signed() as well.
| pub fn new( | ||
| source_account: AccountInfo<'info>, | ||
| destination_account: AccountInfo<'info>, | ||
| authority: AccountInfo<'info>, | ||
| amount: u64, | ||
| payer: AccountInfo<'info>, | ||
| compressed_token_program_authority: AccountInfo<'info>, | ||
| ) -> Self { |
There was a problem hiding this comment.
@ananas-block
can we have one that lets me pass these as options:
• mint: Option<>
• c_token_pool_pda: Option<>
• c_token_pool_pda_bump: Option<>
this makes it easiest for AMMs because then they just need to forward the options from the instruction accounts struct.
the Default/most common usage of TransferInterface is that the caller programs don't know at compile time which individual bridge param will be Some or None.
we can still keep the params explicit, eg with something like this:
.new(..., mint_option) // i think adding mint_option to new makes sense
.token_pool(c_token_pool_pda_option, c_token_pool_pda_bump_option) // bump none = 0
.source_token_program(source_token_program)
.dest_token_program(destination_token_program)
.invoke(...)or
.new(..., mint_option) // i think adding mint_option to new makes sense
.token_pool(c_token_pool_pda_option, c_token_pool_pda_bump_option) // bump none = 0
.token_programs(source_token_program, dest_token_program)
.invoke(...)(note that later we will enable SPL<>SPL so we must always pass source and dest token programs)
There was a problem hiding this comment.
at first glance sounds good to me.
I am adding tests in #2066 it makes more sense to merge this one and add it there so that I can test it right away.
SwenSchaeferjohann
deleted the
jorrit/refactor-compressed-token-sdk-transfer-interface-builder
branch
2 participants
Changes:
CTokenTransferandTransferInterfacePattern:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.