refactor: light token client #2197
Conversation
ananas-block
force-pushed
the
jorrit/refactor-light-token-client
branch
from
9478525 to
58b985a
Compare
📝 WalkthroughWalkthroughThis pull request refactors the token-client SDK from free-function instruction builders to a consistent struct-based action pattern. New action modules introduce structs (Approve, CreateAta, CreateMint, MintTo, Revoke, Transfer, TransferChecked, TransferInterface, Unwrap, Wrap) with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/create_mint.rs`:
- Around line 76-90: The code reads rpc_result.addresses[0] when constructing
CreateMintParams which will panic if addresses is empty; update the caller
(e.g., in the create_mint action before building CreateMintParams) to explicitly
check rpc_result.addresses.is_empty() and return an RpcError::CustomError with a
clear message (e.g., "No addresses returned from indexer for create_mint")
instead of indexing into [0]; then use the checked entry (e.g., let address_root
= rpc_result.addresses[0].root_index) when populating
address_merkle_tree_root_index in CreateMintParams.
In `@sdk-libs/token-client/src/actions/mint_to.rs`:
- Around line 32-38: The MintTo struct initialization is missing the optional
fee_payer field and causes a compile error; update the MintTo construction
(where MintTo { mint, destination, amount, authority: authority.pubkey(),
max_top_up: None, ... }) to include fee_payer: None so the authority account
pays for rent top-ups, matching the project's pattern.
In `@sdk-libs/token-client/src/actions/transfer_interface.rs`:
- Around line 61-76: If spl_token_program is None but either source_owner or
destination_owner is an SPL Token program (e.g. equals spl_token::id()), fail
fast with a clear error instead of continuing; add a guard before building
SplInterface that checks if spl_token_program.is_none() && (source_owner ==
spl_token::id() || destination_owner == spl_token::id()) and return a
descriptive Err (or custom error) indicating missing spl_token_program for
SPL-owned accounts (references: source_owner, destination_owner,
spl_token_program, SplInterface, find_spl_interface_pda_with_index, mint,
restricted).
| // Build params | ||
| let params = CreateMintParams { | ||
| decimals, | ||
| mint_authority_keypair.pubkey(), | ||
| address_merkle_tree_root_index: rpc_result.addresses[0].root_index, | ||
| mint_authority: mint_authority.pubkey(), | ||
| proof: rpc_result.proof.0.ok_or_else(|| { | ||
| RpcError::CustomError("Validity proof is required for create_mint".to_string()) | ||
| })?, | ||
| compression_address, | ||
| mint, | ||
| bump, | ||
| freeze_authority, | ||
| extensions: None, | ||
| rent_payment: 16, // ~24 hours rent | ||
| write_top_up: 766, // ~3 hours per write |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f create_mint.rsRepository: Lightprotocol/light-protocol
Length of output: 542
🏁 Script executed:
cat -n sdk-libs/token-client/src/actions/create_mint.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 3961
🏁 Script executed:
# Search for the function that calls create_mint to understand rpc_result
rg -B 10 "CreateMintParams" sdk-libs/token-client/src/actions/create_mint.rs -A 5Repository: Lightprotocol/light-protocol
Length of output: 1122
🏁 Script executed:
# Find the get_validity_proof function/method definition
rg "get_validity_proof" --type rs -A 20Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
# Look for the definition of addresses field type
rg "struct.*ValidityProof\|addresses:" --type rs -A 10Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
# Search for ValidityProof struct definition
rg "struct ValidityProof" --type-listRepository: Lightprotocol/light-protocol
Length of output: 461
🏁 Script executed:
# Search for ValidityProof struct definition
rg "struct ValidityProof" -A 15Repository: Lightprotocol/light-protocol
Length of output: 3712
🏁 Script executed:
# Find get_validity_proof implementation
rg "fn get_validity_proof" -A 30Repository: Lightprotocol/light-protocol
Length of output: 18893
🏁 Script executed:
# Search for addresses field in ValidityProof or similar structures
rg "addresses:" -B 5 -A 5Repository: Lightprotocol/light-protocol
Length of output: 50385
🏁 Script executed:
# Look at the AddressProofInputs structure and how addresses are returned
rg "struct AddressProofInputs" -A 10Repository: Lightprotocol/light-protocol
Length of output: 1671
🏁 Script executed:
# Check if there are similar patterns in the codebase that guard against empty addresses
rg "addresses\[0\]" --max-count=20Repository: Lightprotocol/light-protocol
Length of output: 3472
🏁 Script executed:
# Search for any defensive checks in similar scenarios
rg "addresses\.first\(\)" -B 3 -A 3 --max-count=10Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check the docstring or comments around get_validity_proof to understand the contract
rg "get_validity_proof" -B 5 -A 15 sdk-libs/client/src/indexer/indexer_trait.rsRepository: Lightprotocol/light-protocol
Length of output: 953
🏁 Script executed:
# Look at actual test cases to see if addresses can ever be empty
rg "get_validity_proof" -A 20 sdk-tests/sdk-light-token-test/tests/test_create_mint.rs | head -50Repository: Lightprotocol/light-protocol
Length of output: 1351
🏁 Script executed:
# Check if there are any error handling patterns in similar address-getting code
rg "addresses\.len\(\)|addresses\.is_empty\(\)" --max-count=15Repository: Lightprotocol/light-protocol
Length of output: 8539
🏁 Script executed:
# Check if there's any guarantee about the number of addresses returned
rg "new_addresses_with_trees" -B 2 -A 2 sdk-libs/token-client/src/actions/create_mint.rsRepository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Look at similar patterns to see if anyone does guard against empty
rg -B 3 "\.first\(\)" sdk-libs/token-client/ -A 3Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check test files to see if there are any tests that expect empty addresses
rg "addresses.*empty\|empty.*addresses" sdk-tests/ --max-count=10Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Look at the actual indexer implementations to see what could cause empty addresses
rg "addresses\s*=" sdk-libs/program-test/src/indexer/test_indexer.rs -B 3 -A 3 | head -100Repository: Lightprotocol/light-protocol
Length of output: 1746
🏁 Script executed:
# Check if there are any error cases documented for get_validity_proof
rg "IndexerError|RpcError" sdk-libs/client/src/indexer/indexer_trait.rs -B 5 -A 5Repository: Lightprotocol/light-protocol
Length of output: 7906
🏁 Script executed:
# See if insert_addresses guards against empty or validates the list
cat -n programs/account-compression/src/processor/insert_addresses.rs | head -50Repository: Lightprotocol/light-protocol
Length of output: 2271
🏁 Script executed:
# Final check: are there any other similar patterns in token-client that guard against empty
rg "\.first\(\)" sdk-libs/token-client/ -B 3 -A 3Repository: Lightprotocol/light-protocol
Length of output: 54
Guard against empty addresses to avoid a panic.
Line 79 indexes rpc_result.addresses[0]; if the indexer returns an empty list, this will panic. Prefer an explicit check and surface an RpcError instead.
✅ Suggested fix
- let params = CreateMintParams {
- decimals,
- address_merkle_tree_root_index: rpc_result.addresses[0].root_index,
+ let address_info = rpc_result
+ .addresses
+ .first()
+ .ok_or_else(|| RpcError::CustomError("Validity proof missing address root".to_string()))?;
+ let params = CreateMintParams {
+ decimals,
+ address_merkle_tree_root_index: address_info.root_index,🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/create_mint.rs` around lines 76 - 90, The
code reads rpc_result.addresses[0] when constructing CreateMintParams which will
panic if addresses is empty; update the caller (e.g., in the create_mint action
before building CreateMintParams) to explicitly check
rpc_result.addresses.is_empty() and return an RpcError::CustomError with a clear
message (e.g., "No addresses returned from indexer for create_mint") instead of
indexing into [0]; then use the checked entry (e.g., let address_root =
rpc_result.addresses[0].root_index) when populating
address_merkle_tree_root_index in CreateMintParams.
| let source_owner = source_account.owner; | ||
| let destination_owner = destination_account.owner; | ||
|
|
||
| // Build SplInterface if needed for cross-interface transfers | ||
| let spl_interface = if let Some(spl_program) = spl_token_program { | ||
| let (spl_interface_pda, spl_interface_pda_bump) = | ||
| find_spl_interface_pda_with_index(&mint, 0, restricted); | ||
| Some(SplInterface { | ||
| mint, | ||
| spl_token_program: spl_program, | ||
| spl_interface_pda, | ||
| spl_interface_pda_bump, | ||
| }) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a clearer guard for SPL owners without spl_token_program.
You already fetch owners, so you can fail fast with a precise error when SPL accounts are involved but spl_token_program is None, instead of letting instruction creation fail later.
💡 Optional guard
use light_token::{
instruction::{SplInterface, TransferInterface},
spl_interface::find_spl_interface_pda_with_index,
+ ID as LIGHT_TOKEN_PROGRAM_ID,
};
@@
let source_owner = source_account.owner;
let destination_owner = destination_account.owner;
+ if spl_token_program.is_none()
+ && (source_owner != LIGHT_TOKEN_PROGRAM_ID || destination_owner != LIGHT_TOKEN_PROGRAM_ID)
+ {
+ return Err(RpcError::CustomError(
+ "spl_token_program is required for SPL transfers".to_string(),
+ ));
+ }
+
// Build SplInterface if needed for cross-interface transfers🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/transfer_interface.rs` around lines 61 -
76, If spl_token_program is None but either source_owner or destination_owner is
an SPL Token program (e.g. equals spl_token::id()), fail fast with a clear error
instead of continuing; add a guard before building SplInterface that checks if
spl_token_program.is_none() && (source_owner == spl_token::id() ||
destination_owner == spl_token::id()) and return a descriptive Err (or custom
error) indicating missing spl_token_program for SPL-owned accounts (references:
source_owner, destination_owner, spl_token_program, SplInterface,
find_spl_interface_pda_with_index, mint, restricted).
| ForesterConfig, | ||
| }; | ||
| use light_token::compressed_token::create_compressed_mint; | ||
| use light_token_client::actions::{ |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/approve.rs`:
- Around line 12-16: The execute method in approve.rs currently uses self.owner
to set the owner but always signs with payer, which causes invalid signatures
when owner != payer; modify execute (and its doc comment) to enforce owner/payer
alignment by checking that self.owner is either None or equals the payer's
pubkey and return an error if a different owner is provided, and update the docs
to direct callers who need a separate owner to use execute_with_owner instead;
reference the execute and execute_with_owner functions and the owner field on
the approvals parameter to locate where to add the guard and doc change.
In `@sdk-libs/token-client/src/actions/mint_to.rs`:
- Around line 47-67: The execute() signature currently accepts (self, rpc,
payer, authority) but lacks an optional fee payer keypair; update execute<R:
Rpc>(self, rpc: &mut R, payer: &Keypair, authority: &Keypair, fee_payer_keypair:
Option<&Keypair>) so you can sign with a distinct fee payer when self.fee_payer
is set; after building the MintToInstruction via
MintToInstruction::instruction() keep constructing signers as Vec<&Keypair>
starting with payer, then push authority if authority.pubkey() !=
payer.pubkey(), and additionally push fee_payer_keypair when Some(fp) and
fp.pubkey() != payer.pubkey() && fp.pubkey() != authority.pubkey() so the
transaction is signed by the correct fee payer when self.fee_payer differs.
In `@sdk-libs/token-client/src/actions/revoke.rs`:
- Around line 12-16: The execute method currently derives owner from self.owner
but always signs with payer, which will fail when owner != payer; modify execute
(in revoke.rs) to check whether self.owner is Some and, if so, ensure the
derived owner_pubkey equals payer.pubkey() or return an Err instructing callers
to use execute_with_owner; update the doc comment for execute to state it only
supports cases where owner == payer and point callers to execute_with_owner for
different owner/payer combos. Ensure the guard compares the owner pubkey derived
from self.owner (or default) against payer.pubkey() before building/signing the
transaction.
In `@sdk-libs/token-client/src/actions/transfer_checked.rs`:
- Around line 56-78: The execute method builds a TransferCheckedInstruction but
fails to include the fee_payer as a signer when it differs from the transaction
payer, causing authorization failures; update the execute signature (e.g.,
transfer_checked::execute and similarly transfer::execute) to accept an optional
fee_payer Keypair (or a required Keypair if design dictates), and add logic
after building the instruction to push the fee_payer Keypair into the signers
vector when self.fee_payer.is_some() and fee_payer.pubkey() != payer.pubkey();
ensure the fee_payer Keypair is used for signing the transaction and mirror the
same signer-handling change in transfer.rs to keep behavior consistent.
In `@sdk-libs/token-client/src/actions/transfer.rs`:
- Around line 47-67: The execute method currently builds a TransferInstruction
but only signs with payer and authority; if self.fee_payer is a different pubkey
the transaction needs that account as a signer. Modify execute (or its
caller-facing API) to either accept an additional Keypair parameter (e.g.,
fee_payer_keypair) and push it into the signers vec when fee_payer !=
payer.pubkey() && fee_payer != authority.pubkey(), or add an upfront validation
that self.fee_payer equals payer.pubkey() or authority.pubkey() and return an
error otherwise; update the signers logic that currently constructs vec![payer]
and conditionally pushes authority to also conditionally push the
fee_payer_keypair when provided and required.
♻️ Duplicate comments (1)
sdk-libs/token-client/src/actions/create_mint.rs (1)
92-99: Guard against empty validity-proof addresses before indexing.Line 95 indexes
rpc_result.addresses[0]; if the indexer returns an empty list, this will panic. Preferfirst()and return a descriptiveRpcError.✅ Suggested fix
- let params = CreateMintInstructionParams { - decimals: self.decimals, - address_merkle_tree_root_index: rpc_result.addresses[0].root_index, + let address_info = rpc_result + .addresses + .first() + .ok_or_else(|| { + RpcError::CustomError("Validity proof missing address root".to_string()) + })?; + let params = CreateMintInstructionParams { + decimals: self.decimals, + address_merkle_tree_root_index: address_info.root_index,
| pub async fn execute<R: Rpc>( | ||
| self, | ||
| rpc: &mut R, | ||
| payer: &Keypair, | ||
| authority: &Keypair, | ||
| ) -> Result<Signature, RpcError> { | ||
| let ix = TransferCheckedInstruction { | ||
| source: self.source, | ||
| mint: self.mint, | ||
| destination: self.destination, | ||
| amount: self.amount, | ||
| decimals: self.decimals, | ||
| authority: authority.pubkey(), | ||
| max_top_up: self.max_top_up, | ||
| fee_payer: self.fee_payer, | ||
| } | ||
| .instruction() | ||
| .map_err(|e| RpcError::CustomError(format!("Failed to create instruction: {}", e)))?; | ||
|
|
||
| let mut signers = vec![payer]; | ||
| if authority.pubkey() != payer.pubkey() { | ||
| signers.push(authority); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Explore the token-client directory structure
find sdk-libs/token-client/src -type f -name "*.rs" | head -20Repository: Lightprotocol/light-protocol
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Read the full transfer_checked.rs file to see complete context
wc -l sdk-libs/token-client/src/actions/transfer_checked.rs
cat -n sdk-libs/token-client/src/actions/transfer_checked.rsRepository: Lightprotocol/light-protocol
Length of output: 3299
🏁 Script executed:
#!/bin/bash
# Read transfer.rs to find the referenced guard/fix
wc -l sdk-libs/token-client/src/actions/transfer.rs
cat -n sdk-libs/token-client/src/actions/transfer.rsRepository: Lightprotocol/light-protocol
Length of output: 2762
🏁 Script executed:
#!/bin/bash
# Find the light_token instruction definition
find . -type f -name "*.rs" -path "*/light_token/*" | grep -E "(instruction|transfer)" | head -20Repository: Lightprotocol/light-protocol
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Search for TransferChecked instruction definition
rg "struct TransferChecked|impl.*TransferChecked" -A 10 --type rs | head -50Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for any fee_payer signer requirements in codebase
rg "fee_payer.*signer|signer.*fee_payer" --type rs -B 2 -A 2Repository: Lightprotocol/light-protocol
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Check CLAUDE.md files for fee_payer patterns
find . -name "CLAUDE.md" -exec grep -l "fee_payer" {} \;Repository: Lightprotocol/light-protocol
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look for light_token program instruction documentation
find . -type f \( -name "CLAUDE.md" -o -name "README.md" \) | xargs grep -l "TransferChecked\|fee_payer" 2>/dev/null | head -10Repository: Lightprotocol/light-protocol
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Read the transfer_checked test to see if fee_payer is used as signer
cat -n ./program-tests/compressed-token-test/tests/light_token/transfer_checked.rsRepository: Lightprotocol/light-protocol
Length of output: 8101
🏁 Script executed:
#!/bin/bash
# Read the transfer test for comparison
cat -n ./program-tests/compressed-token-test/tests/light_token/transfer.rsRepository: Lightprotocol/light-protocol
Length of output: 38830
🏁 Script executed:
#!/bin/bash
# Check compressed-token CLAUDE.md
cat -n ./programs/compressed-token/program/CLAUDE.mdRepository: Lightprotocol/light-protocol
Length of output: 16622
🏁 Script executed:
#!/bin/bash
# Search for fee_payer requirement without rg type flag
grep -r "fee_payer" --include="*.rs" -B 2 -A 2 | head -100Repository: Lightprotocol/light-protocol
Length of output: 7958
🏁 Script executed:
#!/bin/bash
# Find the light_token instruction definitions
find . -type f -name "*.rs" -path "*/light_token/*" | grep -i "instruction\|transfer" | head -20Repository: Lightprotocol/light-protocol
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Search for TransferChecked struct definition in light_token
rg "pub struct TransferChecked|pub struct Transfer" -A 15 --type-list | head -100Repository: Lightprotocol/light-protocol
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Check if fee_payer is ever set to Some(...) in actual usage code
rg "fee_payer.*Some\|Some.*fee_payer" --type-list -B 2 -A 2 | head -50Repository: Lightprotocol/light-protocol
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Look for TRANSFER_CHECKED instruction documentation
find . -type f -name "*.md" | xargs grep -l "TransferChecked\|fee_payer.*signer" 2>/dev/null | head -10Repository: Lightprotocol/light-protocol
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Read the TRANSFER_CHECKED documentation
cat -n ./programs/compressed-token/program/docs/ctoken/TRANSFER_CHECKED.mdRepository: Lightprotocol/light-protocol
Length of output: 12371
🏁 Script executed:
#!/bin/bash
# Also read TRANSFER documentation for comparison
cat -n ./programs/compressed-token/program/docs/ctoken/TRANSFER.mdRepository: Lightprotocol/light-protocol
Length of output: 8133
🏁 Script executed:
#!/bin/bash
# Search for fee_payer usage as Some in actual implementation code (not tests)
rg "fee_payer.*Some|Some.*fee_payer" -B 3 -A 3 --type rust | grep -v "test\|program-tests" | head -80Repository: Lightprotocol/light-protocol
Length of output: 6196
🏁 Script executed:
#!/bin/bash
# Find the light_token instruction structs in the interface/definition crate
find . -path "*/light_token*" -type f -name "*.rs" | xargs grep -l "struct Transfer\|struct TransferChecked" 2>/dev/null | grep -v test | head -10Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check if there's any conditional signer logic for fee_payer in either file
rg "fee_payer" sdk-libs/token-client/src/actions/transfer.rs -B 5 -A 5Repository: Lightprotocol/light-protocol
Length of output: 789
🏁 Script executed:
#!/bin/bash
# Verify the instruction builder method to understand the intent
cat -n sdk-libs/token-client/src/actions/transfer.rs | head -83Repository: Lightprotocol/light-protocol
Length of output: 2713
Add fee_payer to signers when it differs from the transaction payer.
When self.fee_payer is set to a distinct account, the program spec requires it as a (signer, writable) account to authorize top-ups. The current code only signs with payer and authority, causing instruction failure if fee_payer is provided. Add logic to include fee_payer in the signers list:
let mut signers = vec![payer];
if authority.pubkey() != payer.pubkey() {
signers.push(authority);
}
if let Some(fee_payer_pubkey) = self.fee_payer {
if fee_payer_pubkey != payer.pubkey() {
// Need keypair for fee_payer, not just pubkey
// Adjust execute signature to accept fee_payer keypair
}
}Note: Both transfer.rs and transfer_checked.rs have identical missing signer handling—fix both files consistently. The SDK layer correctly marks fee_payer as a signer; the client must do the same.
🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/transfer_checked.rs` around lines 56 - 78,
The execute method builds a TransferCheckedInstruction but fails to include the
fee_payer as a signer when it differs from the transaction payer, causing
authorization failures; update the execute signature (e.g.,
transfer_checked::execute and similarly transfer::execute) to accept an optional
fee_payer Keypair (or a required Keypair if design dictates), and add logic
after building the instruction to push the fee_payer Keypair into the signers
vector when self.fee_payer.is_some() and fee_payer.pubkey() != payer.pubkey();
ensure the fee_payer Keypair is used for signing the transaction and mirror the
same signer-handling change in transfer.rs to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/approve.rs`:
- Around line 57-74: The execute method builds an ApproveInstruction using
self.owner but never signs with an owner keypair, so if owner != payer the
transaction will fail; add a guard at the start of execute (in function execute)
that returns an error when self.owner.is_some() and self.owner.unwrap() !=
payer.pubkey(), or alternatively require callers to use an existing
execute_with_owner that accepts and uses the owner Keypair for signing; update
the error to a clear RpcError::CustomError message indicating the owner/payer
mismatch.
In `@sdk-libs/token-client/src/actions/revoke.rs`:
- Around line 49-65: The execute method currently allows self.owner to differ
from payer but only signs with payer, causing unsigned owner-required
transactions; update execute in revoke.rs (function execute, using self.owner,
payer, RevokeInstruction) to check if self.owner.is_some() and the resolved
owner != payer.pubkey() and immediately return a clear RpcError::CustomError
(e.g., "owner must be payer; use execute_with_owner") instead of
building/sending the tx, or alternatively require/accept an owner signer
parameter and forward it to rpc.create_and_send_transaction; pick one approach
and implement the corresponding early-return or signature-accepting change so
the transaction is always signed by the owner.
♻️ Duplicate comments (2)
sdk-libs/token-client/src/actions/create_mint.rs (1)
92-100: Guard against emptyaddressesbefore indexing.
Line 95 will panic if the indexer returns an empty address list; surface a clearRpcErrorinstead.✅ Suggested fix
- let params = CreateMintInstructionParams { - decimals: self.decimals, - address_merkle_tree_root_index: rpc_result.addresses[0].root_index, + let address_info = rpc_result + .addresses + .first() + .ok_or_else(|| RpcError::CustomError("Validity proof missing address root".to_string()))?; + let params = CreateMintInstructionParams { + decimals: self.decimals, + address_merkle_tree_root_index: address_info.root_index,sdk-libs/token-client/src/actions/transfer_interface.rs (1)
82-97: Add a guard when SPL-owned accounts are used withoutspl_token_program.Line 82-97: if either account is SPL-owned and
spl_token_programisNone, fail fast with a clear error. This avoids late instruction-build errors and clarifies required inputs.💡 Optional guard (same idea as prior review)
use light_token::{ instruction::{SplInterface, TransferInterface as TransferInterfaceInstruction}, spl_interface::find_spl_interface_pda_with_index, + ID as LIGHT_TOKEN_PROGRAM_ID, }; @@ let source_owner = source_account.owner; let destination_owner = destination_account.owner; + if self.spl_token_program.is_none() + && (source_owner != LIGHT_TOKEN_PROGRAM_ID || destination_owner != LIGHT_TOKEN_PROGRAM_ID) + { + return Err(RpcError::CustomError( + "spl_token_program is required for SPL-owned accounts".to_string(), + )); + } + // Build SplInterface if needed for cross-interface transfers
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/create_ata.rs`:
- Around line 85-98: The doc comments should explicitly state that the public
API takes parameters as (mint, owner) but intentionally calls the underlying
helpers with (owner, mint); update the comments for get_ata_address and
derive_ata_address to add a short note like "Note: parameters are (mint, owner)
for this API but are passed to
get_associated_token_address/derive_associated_token_account as (owner, mint)
intentionally" so readers won't be confused by the reordered arguments.
In `@sdk-libs/token-client/src/actions/create_mint.rs`:
- Around line 152-154: The magic numbers assigned to rent_payment (16) and
write_top_up (766) should be extracted and documented or made configurable:
replace the inline literals in the CreateMint initialization with well-named
constants (e.g., DEFAULT_RENT_PAYMENT, DEFAULT_WRITE_TOP_UP) and add doc
comments explaining how each value was derived (hours covered, assumptions about
rent rate/mainnet vs devnet), or add optional fields to the CreateMint struct to
allow callers to override rent_payment and write_top_up; update any code
constructing CreateMint to use the new constants as defaults and add a brief
unit/integration test or comment showing the derivation formula.
♻️ Duplicate comments (1)
sdk-libs/token-client/src/actions/create_mint.rs (1)
140-146: Guard against emptyaddressesto avoid a panic.Line 142 directly indexes
rpc_result.addresses[0]. If the indexer returns an empty list, this will panic at runtime. Use.first()with proper error handling instead.🐛 Proposed fix
+ let address_info = rpc_result + .addresses + .first() + .ok_or_else(|| { + RpcError::CustomError("Validity proof missing address info".to_string()) + })?; + // Build params let params = CreateMintInstructionParams { decimals: self.decimals, - address_merkle_tree_root_index: rpc_result.addresses[0].root_index, + address_merkle_tree_root_index: address_info.root_index, mint_authority: mint_authority.pubkey(),
| pub fn get_ata_address(mint: &Pubkey, owner: &Pubkey) -> Pubkey { | ||
| get_associated_token_address(owner, mint) | ||
| } | ||
|
|
||
| /// Derive the associated token address with bump seed. | ||
| /// | ||
| /// # Arguments | ||
| /// * `mint` - The mint public key | ||
| /// * `owner` - The owner public key | ||
| /// | ||
| /// # Returns | ||
| /// `(Pubkey, u8)` - The ATA address and bump seed | ||
| pub fn derive_ata_address(mint: &Pubkey, owner: &Pubkey) -> (Pubkey, u8) { | ||
| derive_associated_token_account(owner, mint) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the intentional parameter order swap.
Both get_ata_address and derive_ata_address accept (mint, owner) but internally call functions with (owner, mint). This is a valid design choice for API consistency, but it's easy to get confused when reading the code or comparing against SPL documentation.
Consider adding a brief note in the doc comments clarifying this intentional reordering:
📝 Suggested doc enhancement
/// Get the associated token address for a given owner and mint.
///
/// This is a pure function that computes the ATA address without any RPC calls.
+/// Note: Parameter order is (mint, owner) for consistency with other Light Token APIs,
+/// which differs from SPL's (owner, mint) convention.
///
/// # Arguments
/// * `mint` - The mint public key
/// * `owner` - The owner public key🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/create_ata.rs` around lines 85 - 98, The
doc comments should explicitly state that the public API takes parameters as
(mint, owner) but intentionally calls the underlying helpers with (owner, mint);
update the comments for get_ata_address and derive_ata_address to add a short
note like "Note: parameters are (mint, owner) for this API but are passed to
get_associated_token_address/derive_associated_token_account as (owner, mint)
intentionally" so readers won't be confused by the reordered arguments.
| rent_payment: 16, // ~24 hours rent | ||
| write_top_up: 766, // ~3 hours per write | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Explain or extract magic numbers for rent and write top-up.
The values 16 and 766 are undocumented. While the inline comments hint at their purpose ("~24 hours rent", "~3 hours per write"), it's unclear how these values were derived or whether they're environment-dependent (e.g., mainnet vs devnet rent rates).
Consider:
- Extracting these as named constants with detailed documentation
- Or making them configurable via the
CreateMintstruct for advanced users
♻️ Example extraction
/// Default rent payment covering ~24 hours of account rent.
const DEFAULT_RENT_PAYMENT: u64 = 16;
/// Default write top-up covering ~3 hours per write operation.
const DEFAULT_WRITE_TOP_UP: u64 = 766;🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/create_mint.rs` around lines 152 - 154, The
magic numbers assigned to rent_payment (16) and write_top_up (766) should be
extracted and documented or made configurable: replace the inline literals in
the CreateMint initialization with well-named constants (e.g.,
DEFAULT_RENT_PAYMENT, DEFAULT_WRITE_TOP_UP) and add doc comments explaining how
each value was derived (hours covered, assumptions about rent rate/mainnet vs
devnet), or add optional fields to the CreateMint struct to allow callers to
override rent_payment and write_top_up; update any code constructing CreateMint
to use the new constants as defaults and add a brief unit/integration test or
comment showing the derivation formula.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/create_mint.rs`:
- Around line 56-67: Update the CreateMint::seed field documentation to
explicitly state that when seed is None the implementation generates a new
random Keypair internally (as done where seed is created) and that this
generated seed is not returned to the caller, so callers who need to sign future
transactions must supply their own seed Keypair; reference the CreateMint struct
and the seed field in the comment so users understand the recoverability and
signing implications.
- Around line 152-154: The numeric literals rent_payment: 16 and write_top_up:
766 should be replaced with well-named constants and documented: create
constants like RENT_PAYMENT_UNITS = 16 and WRITE_TOP_UP_UNITS = 766 (or more
descriptive names) and use those constants in the struct/assignment where
rent_payment and write_top_up are set (referencing the rent_payment and
write_top_up fields in create_mint.rs); add short doc comments above each
constant explaining what the unit represents (e.g., "approx. 24h rent units" and
"approx. 3h per-write top-up units") so future maintainers can understand the
rationale without relying solely on inline comments.
♻️ Duplicate comments (1)
sdk-libs/token-client/src/actions/create_mint.rs (1)
140-146: Guard against emptyaddressesto avoid a panic.Line 142 indexes
rpc_result.addresses[0]directly. If the indexer returns an empty list, this will panic at runtime. Prefer an explicit check and surface anRpcErrorinstead.✅ Suggested fix
- let params = CreateMintInstructionParams { - decimals: self.decimals, - address_merkle_tree_root_index: rpc_result.addresses[0].root_index, + let address_info = rpc_result + .addresses + .first() + .ok_or_else(|| RpcError::CustomError("Validity proof missing address info".to_string()))?; + + let params = CreateMintInstructionParams { + decimals: self.decimals, + address_merkle_tree_root_index: address_info.root_index,
| #[derive(Default, Debug)] | ||
| pub struct CreateMint { | ||
| /// Number of decimals for the token. | ||
| pub decimals: u8, | ||
| /// Optional authority that can freeze token accounts. | ||
| pub freeze_authority: Option<Pubkey>, | ||
| /// Optional token metadata (name, symbol, uri). | ||
| pub token_metadata: Option<TokenMetadata>, | ||
| /// Optional seed keypair for deterministic mint address. | ||
| /// If None, a new keypair is generated. | ||
| pub seed: Option<Keypair>, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the behavior when seed is None.
When seed is None, a new random Keypair is generated internally (line 85). This means the caller cannot recover the mint seed keypair after the call completes. If users need to sign future transactions with this keypair, they must provide it explicitly. The docstring mentions "auto-generate" but could be clearer about this implication.
📝 Suggested documentation improvement
/// Optional seed keypair for deterministic mint address.
- /// If None, a new keypair is generated.
+ /// If None, a new random keypair is generated internally and cannot be recovered.
+ /// Provide a keypair if you need to reference the mint seed in future transactions.
pub seed: Option<Keypair>,🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/create_mint.rs` around lines 56 - 67,
Update the CreateMint::seed field documentation to explicitly state that when
seed is None the implementation generates a new random Keypair internally (as
done where seed is created) and that this generated seed is not returned to the
caller, so callers who need to sign future transactions must supply their own
seed Keypair; reference the CreateMint struct and the seed field in the comment
so users understand the recoverability and signing implications.
| rent_payment: 16, // ~24 hours rent | ||
| write_top_up: 766, // ~3 hours per write | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document or extract magic numbers for rent/write costs.
The values rent_payment: 16 and write_top_up: 766 appear without explanation. Adding brief comments or extracting these to named constants would help future maintainers understand the rationale (the inline comments are helpful but could be more prominent).
📝 Suggested improvement
+ // Rent payment units: ~24 hours of rent coverage
+ const RENT_PAYMENT_UNITS: u64 = 16;
+ // Write top-up units: ~3 hours per write operation
+ const WRITE_TOP_UP_UNITS: u64 = 766;
+
let params = CreateMintInstructionParams {
// ... other fields ...
- rent_payment: 16, // ~24 hours rent
- write_top_up: 766, // ~3 hours per write
+ rent_payment: RENT_PAYMENT_UNITS,
+ write_top_up: WRITE_TOP_UP_UNITS,
};🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/create_mint.rs` around lines 152 - 154, The
numeric literals rent_payment: 16 and write_top_up: 766 should be replaced with
well-named constants and documented: create constants like RENT_PAYMENT_UNITS =
16 and WRITE_TOP_UP_UNITS = 766 (or more descriptive names) and use those
constants in the struct/assignment where rent_payment and write_top_up are set
(referencing the rent_payment and write_top_up fields in create_mint.rs); add
short doc comments above each constant explaining what the unit represents
(e.g., "approx. 24h rent units" and "approx. 3h per-write top-up units") so
future maintainers can understand the rationale without relying solely on inline
comments.
…ctions to test utils
ananas-block
force-pushed
the
jorrit/refactor-light-token-client
branch
from
2e32f8e to
da718b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@forester/Cargo.toml`:
- Line 34: Production code imports legacy test instruction builders
(mint_action) from light-test-utils; remove that production dependency and move
the builders into a non-test crate. Remove light-test-utils from [dependencies]
in Cargo.toml, create or use an existing production crate (e.g.,
light-token-client or a new instructions crate) to host the legacy builders,
relocate functions/modules (mint_action and its parents under
light_test_utils::actions::legacy::instructions) into that crate, update
compressor.rs to import mint_action from the new crate, and ensure the new crate
is added to [dependencies] (not dev-dependencies).
In `@forester/src/compressible/mint/compressor.rs`:
- Around line 9-11: The production module compressor.rs currently imports
create_mint_action_instruction, MintActionParams, and MintActionType from the
test-only crate light_test_utils::actions::legacy::instructions::mint_action;
remove that test utility dependency and instead import those symbols from the
appropriate production SDK crate (e.g., light-token-client or
light-compressed-token-sdk) by adding the SDK to Cargo.toml and replacing the
use path in compressor.rs to the SDK's module that exports
create_mint_action_instruction, MintActionParams, and MintActionType; ensure any
API differences are adapted (rename types or conversion wrappers) so
compressor.rs uses only production-exported APIs and no test utility crates.
In `@sdk-libs/token-client/src/actions/approve.rs`:
- Around line 100-118: The method execute_with_owner currently ignores
self.owner if set; add a guard near the start of execute_with_owner to check if
self.owner.is_some() and if so compare it to owner.pubkey(), returning an
Err(RpcError::CustomError(...)) when they differ; place this check before
building the ApproveInstruction and before assembling signers (reference
self.owner, execute_with_owner, and owner.pubkey()) so callers supplying a
conflicting owner get a clear error instead of silent misuse.
In `@sdk-libs/token-client/src/actions/unwrap.rs`:
- Around line 53-83: The unwrap.execute currently hardcodes SPL_TOKEN_PROGRAM_ID
into the TransferToSpl instruction, which prevents Token-2022 support; add an
optional spl_token_program field to the unwrap action (same shape as used by
TransferInterface/wrap action), use that field (falling back to
SPL_TOKEN_PROGRAM_ID when None) when constructing TransferToSpl (set
spl_token_program: self.spl_token_program.unwrap_or(SPL_TOKEN_PROGRAM_ID)), and
mirror the same change in the sibling wrap.rs and any constructors/deserializers
for the action so the optional program id is plumbed through.
In `@sdk-libs/token-client/src/actions/wrap.rs`:
- Around line 54-84: The execute function in wrap.rs currently hardcodes
SPL_TOKEN_PROGRAM_ID for TransferFromSpl.spl_token_program; update it to mirror
the Token-2022 change you made in Unwrap by using the configurable token program
id instead of the constant (e.g., use self.token_program or the same
token_program_id getter/parameter introduced for Unwrap). Modify the
TransferFromSpl construction to set spl_token_program from that field/value and
propagate any struct/constructor or execute signature changes so callers supply
the chosen token program id.
♻️ Duplicate comments (5)
sdk-libs/token-client/src/actions/transfer_checked.rs (1)
28-40: Consider exposingmax_top_upfor advanced use cases.The struct currently hardcodes
max_top_up: Nonein the instruction builder (line 72). For users who need to control rent top-up limits during transfers, consider adding an optional field:pub struct TransferChecked { /// Source token account. pub source: Pubkey, /// The mint public key. pub mint: Pubkey, /// Destination token account. pub destination: Pubkey, /// Amount of tokens to transfer. pub amount: u64, /// Expected decimals for the token. pub decimals: u8, + /// Maximum lamports to top up for rent (None = use default). + pub max_top_up: Option<u64>, }This maintains backward compatibility via
Defaultwhile enabling advanced scenarios.sdk-libs/token-client/src/actions/create_ata.rs (1)
85-99: Document the parameter order convention.Both
get_ata_addressandderive_ata_addressaccept(mint, owner)but internally call functions with(owner, mint). This is intentional for API consistency but differs from SPL's convention.📝 Suggested doc enhancement
/// Get the associated token address for a given owner and mint. /// /// This is a pure function that computes the ATA address without any RPC calls. +/// +/// Note: Parameter order is (mint, owner) for consistency with Light Token APIs, +/// which differs from SPL's (owner, mint) convention. /// /// # Arguments /// * `mint` - The mint public key /// * `owner` - The owner public keysdk-libs/token-client/src/actions/transfer_interface.rs (1)
64-97: Consider adding an early guard when SPL accounts are detected butspl_token_programisNone.Currently, if
spl_token_programisNonebut either account is owned by an SPL token program, the instruction will fail during on-chain validation with a less descriptive error. You could fail fast with a clear message.💡 Optional defensive guard
+use light_token::ID as LIGHT_TOKEN_PROGRAM_ID; + let source_owner = source_account.owner; let destination_owner = destination_account.owner; + // Fail fast if SPL accounts are involved but spl_token_program is missing + if self.spl_token_program.is_none() + && (source_owner != LIGHT_TOKEN_PROGRAM_ID || destination_owner != LIGHT_TOKEN_PROGRAM_ID) + { + return Err(RpcError::CustomError( + "spl_token_program is required when transferring to/from SPL accounts".to_string(), + )); + } + // Build SplInterface if needed for cross-interface transferssdk-libs/token-client/src/actions/create_mint.rs (2)
139-143: Guard against empty validity-proof addresses to avoid a panic.Line 142 indexes
rpc_result.addresses[0]; if the indexer returns an empty list this will panic. Please validate and return a clearRpcErrorinstead.🔧 Proposed fix
+ let address_info = rpc_result.addresses.first().ok_or_else(|| { + RpcError::CustomError("No addresses returned from indexer for create_mint".to_string()) + })?; // Build params let params = CreateMintInstructionParams { decimals: self.decimals, - address_merkle_tree_root_index: rpc_result.addresses[0].root_index, + address_merkle_tree_root_index: address_info.root_index, mint_authority: mint_authority.pubkey(),
152-153: Extract rent/write top-up magic numbers.Line 152–153 embeds numeric literals; consider named constants or configurable fields for clarity.
♻️ Example extraction
+ const DEFAULT_RENT_PAYMENT: u64 = 16; // ~24 hours rent + const DEFAULT_WRITE_TOP_UP: u64 = 766; // ~3 hours per write let params = CreateMintInstructionParams { // ... - rent_payment: 16, // ~24 hours rent - write_top_up: 766, // ~3 hours per write + rent_payment: DEFAULT_RENT_PAYMENT, + write_top_up: DEFAULT_WRITE_TOP_UP, };
forester/Cargo.toml
Outdated
| light-token-interface = { workspace = true } | ||
| light-token-client = { workspace = true } | ||
| light-token = { workspace = true } | ||
| light-test-utils = { workspace = true } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check forester/Cargo.toml for light-test-utils occurrences
echo "=== Checking forester/Cargo.toml for light-test-utils ==="
grep -n "light-test-utils" forester/Cargo.toml || echo "No matches found"
echo ""
echo "=== Full relevant sections of forester/Cargo.toml ==="
cat -n forester/Cargo.toml | grep -A 5 -B 5 "dependencies\|light-test"Repository: Lightprotocol/light-protocol
Length of output: 1887
🏁 Script executed:
#!/bin/bash
# Find all production (non-test) Rust files in forester that import light_test_utils
echo "=== Production files importing light_test_utils ==="
find forester/src -type f -name "*.rs" | xargs grep -l "light_test_utils" 2>/dev/null || echo "No files found"
echo ""
echo "=== Test files importing light_test_utils ==="
find forester/tests -type f -name "*.rs" 2>/dev/null | xargs grep -l "light_test_utils" 2>/dev/null || echo "No test files found or directory doesn't exist"Repository: Lightprotocol/light-protocol
Length of output: 704
🏁 Script executed:
#!/bin/bash
# Show exact imports of light_test_utils in forester source
echo "=== All imports of light_test_utils in forester ==="
find forester/src -type f -name "*.rs" -exec grep -H "use light_test_utils\|use.*light_test_utils" {} \; 2>/dev/null || echo "No imports found"Repository: Lightprotocol/light-protocol
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Check where mint_action is actually defined
echo "=== Searching for mint_action definition ==="
rg "mint_action" --type rust -l
echo ""
echo "=== Files containing mint_action definition ==="
rg "fn mint_action|pub.*mint_action" --type rust -A 2 || echo "No matches"Repository: Lightprotocol/light-protocol
Length of output: 13175
Move mint_action instruction builders to a production crate.
Forester's production code (compressor.rs) imports from light_test_utils::actions::legacy::instructions::mint_action, which violates the principle that production code shouldn't depend on test utilities. These legacy instruction builders should live in a non-test crate—either light-token-client or a dedicated instructions utility crate—so forester can depend on them without pulling in test infrastructure.
Additionally, light-test-utils in line 34 should be removed from [dependencies] since it's already in [dev-dependencies] (line 69). Remove the production dependency.
🤖 Prompt for AI Agents
In `@forester/Cargo.toml` at line 34, Production code imports legacy test
instruction builders (mint_action) from light-test-utils; remove that production
dependency and move the builders into a non-test crate. Remove light-test-utils
from [dependencies] in Cargo.toml, create or use an existing production crate
(e.g., light-token-client or a new instructions crate) to host the legacy
builders, relocate functions/modules (mint_action and its parents under
light_test_utils::actions::legacy::instructions) into that crate, update
compressor.rs to import mint_action from the new crate, and ensure the new crate
is added to [dependencies] (not dev-dependencies).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sdk-libs/token-client/src/actions/unwrap.rs`:
- Around line 60-69: The code assigns destination_account_info.owner to
spl_token_program without verifying it’s a supported SPL token program; update
unwrap.rs to check that destination_account_info.owner equals the known SPL
Token program IDs (e.g., the standard TOKEN_PROGRAM_ID and Token-2022 ID
constants) before proceeding, and return a clear RpcError::CustomError
(referencing self.destination_spl_ata and destination_account_info) if it does
not match so we fail fast with an explanatory message rather than later CPI
failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@forester/src/compressible/mint/compressor.rs`:
- Around line 73-87: The error path wrapping the await on
create_compress_and_close_mint_instruction should include mint context (e.g.,
mint_pda, mint_seed, compressed_address) in the error message; update the
map_err closures for create_compress_and_close_mint_instruction (and the similar
call around lines 218-230) to format and include those variables so the anyhows
show seed and compressed address along with the original error.
In `@sdk-libs/token-client/src/actions/wrap.rs`:
- Around line 5-10: Add the spl-token crate to Cargo.toml and, inside wrap.rs
(and mirror in unwrap.rs), read and deserialize the source SPL token account
before constructing the TransferFromSpl instruction: use
spl_token::state::Account to parse the account data (via the account's pubkey
fetched with find_spl_interface_pda or the provided source account), verify its
mint equals self.mint (and optionally validate decimals against self.decimals),
and return an explicit error if they differ; only build and send TransferFromSpl
once the on-chain account's mint matches the expected mint to prevent opaque
on-chain failures.
| let ix = create_compress_and_close_mint_instruction( | ||
| &mut *rpc, | ||
| payer, | ||
| actions: vec![MintActionType::CompressAndCloseMint { idempotent: true }], | ||
| new_mint: None, | ||
| }; | ||
|
|
||
| let ix = create_mint_action_instruction(&mut *rpc, params) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to build CompressAndCloseMint instruction for {}: {:?}", | ||
| mint_pda, | ||
| e | ||
| ) | ||
| })?; | ||
| compressed_address, | ||
| mint_seed, | ||
| true, // idempotent | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to build CompressAndCloseMint instruction for {}: {:?}", | ||
| mint_pda, | ||
| e | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add mint context to instruction-build errors.
Including seed/compressed address in both error paths will make triage much faster when a build fails.
♻️ Proposed tweak
- .map_err(|e| {
- anyhow::anyhow!(
- "Failed to build CompressAndCloseMint instruction for {}: {:?}",
- mint_pda,
- e
- )
- })?;
+ .map_err(|e| {
+ anyhow::anyhow!(
+ "Failed to build CompressAndCloseMint instruction for {} (seed: {:?}, compressed_address: {:?}): {:?}",
+ mint_pda,
+ mint_seed,
+ compressed_address,
+ e
+ )
+ })?;- .map_err(|e| {
- anyhow::anyhow!("Failed to build CompressAndCloseMint instruction: {:?}", e)
- })?;
+ .map_err(|e| {
+ anyhow::anyhow!(
+ "Failed to build CompressAndCloseMint instruction for {} (seed: {:?}, compressed_address: {:?}): {:?}",
+ mint_pda,
+ mint_seed,
+ compressed_address,
+ e
+ )
+ })?;Also applies to: 218-230
🤖 Prompt for AI Agents
In `@forester/src/compressible/mint/compressor.rs` around lines 73 - 87, The error
path wrapping the await on create_compress_and_close_mint_instruction should
include mint context (e.g., mint_pda, mint_seed, compressed_address) in the
error message; update the map_err closures for
create_compress_and_close_mint_instruction (and the similar call around lines
218-230) to format and include those variables so the anyhows show seed and
compressed address along with the original error.
| use light_client::rpc::{Rpc, RpcError}; | ||
| use light_token::{ | ||
| constants::{SPL_TOKEN_2022_PROGRAM_ID, SPL_TOKEN_PROGRAM_ID}, | ||
| instruction::TransferFromSpl, | ||
| spl_interface::{find_spl_interface_pda, has_restricted_extensions}, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the repository structure and locate the file
fd "wrap.rs" --type fRepository: Lightprotocol/light-protocol
Length of output: 211
🏁 Script executed:
# Check the actual wrap.rs file
cat -n sdk-libs/token-client/src/actions/wrap.rs | head -120Repository: Lightprotocol/light-protocol
Length of output: 4821
🏁 Script executed:
# Check dependencies in the token-client Cargo.toml
cat sdk-libs/token-client/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 1078
🏁 Script executed:
# Search for existing token account parsing patterns in the codebase
rg "SplAccount|StateWithExtensions" --type rust -A 2 -B 2Repository: Lightprotocol/light-protocol
Length of output: 18310
🏁 Script executed:
# Check if spl_token is used anywhere in the codebase
rg "use spl_token::" --type rust | head -20Repository: Lightprotocol/light-protocol
Length of output: 633
🏁 Script executed:
# Check what Account types are being imported for token account parsing
rg "use.*state::Account" --type rust -B 2 -A 2 | head -40Repository: Lightprotocol/light-protocol
Length of output: 3705
🏁 Script executed:
# Check root Cargo.toml to see available versions
grep -A 5 "spl-token" Cargo.toml 2>/dev/null || cat Cargo.toml | grep -A 10 "\[workspace.dependencies\]"Repository: Lightprotocol/light-protocol
Length of output: 343
🏁 Script executed:
# Look at the test file to see how it actually validates source account mint
cat -n sdk-tests/token-client-test/tests/test_wrap_unwrap.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 3819
🏁 Script executed:
# Check what token-client actually imports/exports
cat sdk-libs/token-client/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 1078
🏁 Script executed:
# Verify the actual parse pattern for token accounts in the test
rg "SplTokenAccount::unpack|StateWithExtensions.*unpack" sdk-tests/token-client-test/tests/test_wrap_unwrap.rs -B 3 -A 3Repository: Lightprotocol/light-protocol
Length of output: 627
🏁 Script executed:
# Check if any sdk-libs crate explicitly depends on spl-token
rg "spl-token[^-]" sdk-libs/*/Cargo.toml --type tomlRepository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check how light_token crate (which is imported) handles token account parsing
cat light-libs/light-token/Cargo.toml | grep -A 10 "\[dependencies\]"Repository: Lightprotocol/light-protocol
Length of output: 137
🏁 Script executed:
# See if there's a utility function for parsing token accounts in light-token
rg "fn.*unpack|parse.*account" light-libs/light-token/src --type rust | head -20Repository: Lightprotocol/light-protocol
Length of output: 138
🏁 Script executed:
# Find where light-token is located
fd "light-token" -t d | grep -v node_modules | grep -v targetRepository: Lightprotocol/light-protocol
Length of output: 103
🏁 Script executed:
# Check the light-token package location and its dependencies
find . -name "light-token" -type d 2>/dev/null | head -5Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Search for light-token in workspace
rg "name = \"light-token\"" --type tomlRepository: Lightprotocol/light-protocol
Length of output: 154
🏁 Script executed:
# Check light-token (token-sdk) dependencies
cat sdk-libs/token-sdk/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 2923
🏁 Script executed:
# Search for token account parsing patterns in token-sdk
rg "unpack|parse.*Account" sdk-libs/token-sdk/src --type rust -B 2 -A 2 | head -40Repository: Lightprotocol/light-protocol
Length of output: 2411
🏁 Script executed:
# Check if spl_interface module in light-token has any account parsing utilities
fd "spl_interface" sdk-libs/ --type fRepository: Lightprotocol/light-protocol
Length of output: 162
🏁 Script executed:
# Check the spl_interface module in token-sdk
cat -n sdk-libs/token-sdk/src/spl_interface.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 786
🏁 Script executed:
# Check if there's a find_spl_interface_pda and has_restricted_extensions functions
rg "find_spl_interface_pda|has_restricted_extensions" sdk-libs/token-sdk/src --type rust -B 2 -A 5Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Look for how mint accounts are parsed in the codebase for Token-2022
rg "StateWithExtensions.*Account|PodStateWithExtensions.*Account" --type rust | grep -v test | grep -v "\.rs-" | head -10Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Find the actual implementation in compressed-token-sdk
find sdk-libs/compressed-token-sdk -name "spl_interface.rs" -type fRepository: Lightprotocol/light-protocol
Length of output: 122
🏁 Script executed:
# Check the compressed-token-sdk spl_interface module
cat -n sdk-libs/compressed-token-sdk/src/spl_interface.rsRepository: Lightprotocol/light-protocol
Length of output: 6038
🏁 Script executed:
# Search more broadly for how source account mint is validated anywhere in token operations
rg "source.*mint|validate.*mint" --type rust -B 2 -A 2 | grep -A 5 -B 5 "wrap\|Wrap"Repository: Lightprotocol/light-protocol
Length of output: 11133
🏁 Script executed:
# Check what TransferFromSpl instruction expects and how it validates
rg "struct TransferFromSpl|impl.*TransferFromSpl" sdk-libs/ --type rust -A 15 | head -50Repository: Lightprotocol/light-protocol
Length of output: 4841
🏁 Script executed:
# Check light-token-interface to see if it has token account parsing utilities
cat -n sdk-libs/token-interface/src/lib.rs | head -50Repository: Lightprotocol/light-protocol
Length of output: 139
🏁 Script executed:
# Look for any account validation in the TransferFromSpl instruction or on-chain validation
rg "TransferFromSpl" programs/ --type rust -B 2 -A 10 | head -60Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check if spl-token-2022 provides a way to parse Account states
rg "pub fn unpack|pub struct Account" $(find sdk-libs -name "*.rs" -path "*/spl-token-2022/*" 2>/dev/null | head -5) 2>/dev/null || echo "Not in local source"Repository: Lightprotocol/light-protocol
Length of output: 2541
🏁 Script executed:
# Look at the actual on-chain TransferFromSpl instruction implementation to see if it validates
find programs/ -name "transfer_from_spl.rs" -o -name "transfer_from_spl" -type dRepository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check token-interface to understand the validation model
fd "lib.rs|mod.rs" program-libs/token-interface/srcRepository: Lightprotocol/light-protocol
Length of output: 625
🏁 Script executed:
# Check token-interface lib.rs to understand the module structure and design
cat -n program-libs/token-interface/src/lib.rs | head -80Repository: Lightprotocol/light-protocol
Length of output: 668
🏁 Script executed:
# Look for any source account validation in the on-chain TransferFromSpl processor
rg "TransferFromSpl|transfer_from_spl" programs/compressed-token/program/src --type rust -B 5 -A 10 | head -80Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check if spl-token is reasonable to add as an external dependency (already in workspace)
grep -E "spl-token[^-]|external" Cargo.toml | head -20Repository: Lightprotocol/light-protocol
Length of output: 91
🏁 Script executed:
# Look at a similar SDK operation to see how it handles token account data
cat -n sdk-libs/token-client/src/actions/unwrap.rs | head -100Repository: Lightprotocol/light-protocol
Length of output: 4116
🏁 Script executed:
# Final check: see if there's any validation pattern in the compressed-token-sdk
rg "source.*account|validate.*source" sdk-libs/compressed-token-sdk/src --type rust | head -20Repository: Lightprotocol/light-protocol
Length of output: 1008
Add spl-token to dependencies and validate source SPL account mint matches the expected mint.
Right now self.mint and self.decimals are used without verifying the source account actually holds tokens for that mint. A mismatch will cause opaque on-chain failures when the instruction tries to transfer from the wrong mint. Add spl-token to Cargo.toml (it's already in the workspace), then parse the source account to confirm its mint before building the instruction.
🧩 Suggested defensive check
use light_client::rpc::{Rpc, RpcError};
use light_token::{
constants::{SPL_TOKEN_2022_PROGRAM_ID, SPL_TOKEN_PROGRAM_ID},
instruction::TransferFromSpl,
spl_interface::{find_spl_interface_pda, has_restricted_extensions},
};
+use spl_token::state::Account as SplTokenAccount;
+use spl_token_2022::extension::StateWithExtensions;
+use spl_token_2022::state::Account as Spl2022Account;
use solana_keypair::Keypair;
use solana_pubkey::Pubkey;
use solana_signature::Signature;
use solana_signer::Signer;
@@
let spl_token_program = source_account_info.owner;
// Validate that the source account is owned by a supported SPL token program
if spl_token_program != SPL_TOKEN_PROGRAM_ID
&& spl_token_program != SPL_TOKEN_2022_PROGRAM_ID
{
return Err(RpcError::CustomError(format!(
"Source SPL token account {} is owned by an unsupported program {}. \
Expected SPL Token ({}) or Token-2022 ({}).",
self.source_spl_ata,
source_account_info.owner,
SPL_TOKEN_PROGRAM_ID,
SPL_TOKEN_2022_PROGRAM_ID
)));
}
+
+ // Verify the source account mint matches the requested mint
+ let source_mint = if spl_token_program == SPL_TOKEN_2022_PROGRAM_ID {
+ let acc = StateWithExtensions::<Spl2022Account>::unpack(&source_account_info.data)
+ .map_err(|e| RpcError::CustomError(format!("Failed to parse Token-2022 account: {e}")))?;
+ acc.base.mint
+ } else {
+ let acc = SplTokenAccount::unpack(&source_account_info.data)
+ .map_err(|e| RpcError::CustomError(format!("Failed to parse SPL token account: {e}")))?;
+ acc.mint
+ };
+ if source_mint != self.mint {
+ return Err(RpcError::CustomError(format!(
+ "Source SPL token account mint {} does not match requested mint {}.",
+ source_mint, self.mint
+ )));
+ }Also applies to: unwrap.rs (validate destination account mint similarly)
🤖 Prompt for AI Agents
In `@sdk-libs/token-client/src/actions/wrap.rs` around lines 5 - 10, Add the
spl-token crate to Cargo.toml and, inside wrap.rs (and mirror in unwrap.rs),
read and deserialize the source SPL token account before constructing the
TransferFromSpl instruction: use spl_token::state::Account to parse the account
data (via the account's pubkey fetched with find_spl_interface_pda or the
provided source account), verify its mint equals self.mint (and optionally
validate decimals against self.decimals), and return an explicit error if they
differ; only build and send TransferFromSpl once the on-chain account's mint
matches the expected mint to prevent opaque on-chain failures.
| pub fn get_ata_address(mint: &Pubkey, owner: &Pubkey) -> Pubkey { | ||
| get_associated_token_address(owner, mint) | ||
| } |
There was a problem hiding this comment.
let's just use get_associated_token_address then
| pub fn derive_ata_address(mint: &Pubkey, owner: &Pubkey) -> (Pubkey, u8) { | ||
| derive_associated_token_account(owner, mint) |
There was a problem hiding this comment.
mhm, what comes to mind is doing it analog to spl_associated_token_account_interface::address crate
get_associated_token_address and
get_associated_token_address_and_bump_seed respectively
https://github.com/solana-program/associated-token-account/blob/main/interface/src/address.rs#L7-L70
There was a problem hiding this comment.
those do require explicit program id and ata program id. we could wrap them with the light_token::id() as defaults or simply re-export
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.