fix(sdk): align token-sdk authority mutability and wire format with pinocchio#2301
fix(sdk): align token-sdk authority mutability and wire format with pinocchio#2301
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved optional Changes
Sequence Diagram(s)(omitted) 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdk-libs/token-sdk/src/instruction/approve.rs (2)
29-29: 🧹 Nitpick | 🔵 Trivial
"payer for top-up"in the doc comment is misleading whenmax_top_upisNone.The owner is only writable (and therefore can pay for a top-up) when
max_top_upisSome. WithNone, the owner is readonly and cannot be debited.✏️ Suggested doc fix
- /// Owner of the Light Token account (signer, payer for top-up) + /// Owner of the Light Token account (signer; writable and payer for top-up only when `max_top_up` is `Some`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/src/instruction/approve.rs` at line 29, Update the doc comment for the owner field in approve.rs to avoid implying the owner is always a payer; clarify that the owner is the signer and payer for top-ups only when max_top_up is Some, and when max_top_up is None the owner is read-only and cannot be debited. Locate the owner field/parameter in the approve.rs instruction (the field documented by the current comment "Owner of the Light Token account (signer, payer for top-up)") and replace that sentence with a conditional description mentioning max_top_up to make the access semantics explicit.
24-36:⚠️ Potential issue | 🟡 MinorSemver-breaking change confirmed — missing
#[non_exhaustive]on public structs.
Approve,ApproveCpi,Revoke, andRevokeCpilack the#[non_exhaustive]attribute. Addingmax_top_upas a mandatory field breaks code that initializes these structs with explicit field syntax. Verification shows test code andFromimplementations are updated, but consider adding#[non_exhaustive]to prevent future breaks when adding fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/src/instruction/approve.rs` around lines 24 - 36, Public structs Approve, ApproveCpi, Revoke, and RevokeCpi were extended with a new field and lack the #[non_exhaustive] attribute, which is a semver-breaking change for downstream code that constructs them by explicit field init; add #[non_exhaustive] above each of these pub struct declarations (Approve, ApproveCpi, Revoke, RevokeCpi) so future fields can be added without breaking consumers, and run tests to ensure no other public structs need the same treatment.sdk-libs/token-client/src/actions/revoke.rs (1)
90-111:⚠️ Potential issue | 🟡 Minor
execute_with_ownersilently ignoresself.owner, unlike the symmetricApprove::execute_with_owner.
Approve::execute_with_ownervalidates thatself.owner(whenSome) matches the providedownerkeypair and returns a descriptiveRpcErroron mismatch.Revoke::execute_with_ownerignoresself.ownerentirely and usesowner.pubkey()unconditionally. A caller who setsRevoke { token_account, owner: Some(pubkey_A) }and callsexecute_with_owner(.., &keypair_B)gets a silent mismatch that only surfaces as an opaque on-chain rejection.🐛 Proposed fix — add the same guard as `Approve::execute_with_owner`
pub async fn execute_with_owner<R: Rpc>( self, rpc: &mut R, payer: &Keypair, owner: &Keypair, ) -> Result<Signature, RpcError> { + // Guard: if self.owner is set, it must match the provided owner keypair + if let Some(expected_owner) = self.owner { + if expected_owner != owner.pubkey() { + return Err(RpcError::CustomError(format!( + "owner mismatch: self.owner ({}) does not match owner.pubkey() ({})", + expected_owner, + owner.pubkey() + ))); + } + } + let ix = RevokeInstruction { token_account: self.token_account, owner: owner.pubkey(), max_top_up: None, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-client/src/actions/revoke.rs` around lines 90 - 111, Revoke::execute_with_owner currently ignores self.owner and unconditionally uses the provided owner keypair; add the same guard used in Approve::execute_with_owner: if let Some(expected) = self.owner { if expected != owner.pubkey() { return Err(RpcError::CustomError(format!("Owner mismatch: expected {}, got {}", expected, owner.pubkey()))); } }—place this check at the start of execute_with_owner (before building RevokeInstruction) so a mismatched self.owner returns a clear RpcError instead of silently proceeding.sdk-libs/token-sdk/tests/instruction_close.rs (1)
1-67: 🧹 Nitpick | 🔵 TrivialAdd unit tests for approve and revoke instruction changes.
This file tests the close instruction changes thoroughly. The analogous
approve/revokeinstruction changes — conditionalownerwritability and variable instruction data depending onmax_top_up— have no equivalent unit tests. The close-test pattern is a good template.Specifically missing:
max_top_up: None→ owner isnew_readonly, data is 9 bytes ([4, amount...])max_top_up: Some(x)→ owner isnew(writable), data is 11 bytes ([4, amount..., x...])- Same two cases for revoke (discriminator
5, noamount, data is 1 or 3 bytes)Do you want me to generate the test file(s) for
instruction_approve.rsandinstruction_revoke.rs?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/tests/instruction_close.rs` around lines 1 - 67, Add unit tests mirroring instruction_close.rs for the Approve and Revoke instructions to cover both max_top_up None/Some cases: create tests (e.g., test_approve_instruction and test_approve_with_max_top_up; test_revoke_instruction and test_revoke_with_max_top_up) that build deterministic pubkeys, construct the instruction via Approve::new(...)/Revoke::new(...), call .custom_rent_sponsor(...) when needed, and assert equality with a hardcoded Instruction where owner AccountMeta is readonly when max_top_up is None and writable when Some(x), and the data bytes match expected lengths/content (Approve discriminator 4: 9 bytes when None [4 + amount..] and 11 bytes when Some [4 + amount.. + max_top_up..]; Revoke discriminator 5: 1 byte when None and 3 bytes when Some). Ensure you reference LIGHT_TOKEN_PROGRAM_ID, LIGHT_TOKEN_RENT_SPONSOR, and use AccountMeta::new/_readonly to assert signer/writable flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-libs/token-sdk/src/instruction/revoke.rs`:
- Line 24: Update the doc comment for the owner field in revoke.rs to match the
fix applied in approve.rs: remove or reword the parenthetical "(payer for
top-up)" so the comment reads only that this is the owner/signing authority of
the Light Token account (e.g., "Owner of the Light Token account (signer)") or
another clarified phrasing used in approve.rs; locate the comment attached to
the owner field/struct in revoke.rs and apply the same wording change.
---
Outside diff comments:
In `@sdk-libs/token-client/src/actions/revoke.rs`:
- Around line 90-111: Revoke::execute_with_owner currently ignores self.owner
and unconditionally uses the provided owner keypair; add the same guard used in
Approve::execute_with_owner: if let Some(expected) = self.owner { if expected !=
owner.pubkey() { return Err(RpcError::CustomError(format!("Owner mismatch:
expected {}, got {}", expected, owner.pubkey()))); } }—place this check at the
start of execute_with_owner (before building RevokeInstruction) so a mismatched
self.owner returns a clear RpcError instead of silently proceeding.
In `@sdk-libs/token-sdk/src/instruction/approve.rs`:
- Line 29: Update the doc comment for the owner field in approve.rs to avoid
implying the owner is always a payer; clarify that the owner is the signer and
payer for top-ups only when max_top_up is Some, and when max_top_up is None the
owner is read-only and cannot be debited. Locate the owner field/parameter in
the approve.rs instruction (the field documented by the current comment "Owner
of the Light Token account (signer, payer for top-up)") and replace that
sentence with a conditional description mentioning max_top_up to make the access
semantics explicit.
- Around line 24-36: Public structs Approve, ApproveCpi, Revoke, and RevokeCpi
were extended with a new field and lack the #[non_exhaustive] attribute, which
is a semver-breaking change for downstream code that constructs them by explicit
field init; add #[non_exhaustive] above each of these pub struct declarations
(Approve, ApproveCpi, Revoke, RevokeCpi) so future fields can be added without
breaking consumers, and run tests to ensure no other public structs need the
same treatment.
In `@sdk-libs/token-sdk/tests/instruction_close.rs`:
- Around line 1-67: Add unit tests mirroring instruction_close.rs for the
Approve and Revoke instructions to cover both max_top_up None/Some cases: create
tests (e.g., test_approve_instruction and test_approve_with_max_top_up;
test_revoke_instruction and test_revoke_with_max_top_up) that build
deterministic pubkeys, construct the instruction via
Approve::new(...)/Revoke::new(...), call .custom_rent_sponsor(...) when needed,
and assert equality with a hardcoded Instruction where owner AccountMeta is
readonly when max_top_up is None and writable when Some(x), and the data bytes
match expected lengths/content (Approve discriminator 4: 9 bytes when None [4 +
amount..] and 11 bytes when Some [4 + amount.. + max_top_up..]; Revoke
discriminator 5: 1 byte when None and 3 bytes when Some). Ensure you reference
LIGHT_TOKEN_PROGRAM_ID, LIGHT_TOKEN_RENT_SPONSOR, and use
AccountMeta::new/_readonly to assert signer/writable flags.
| @@ -22,6 +23,9 @@ pub struct Revoke { | |||
| pub token_account: Pubkey, | |||
| /// Owner of the Light Token account (signer, payer for top-up) | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same "payer for top-up" doc comment issue as in approve.rs.
✏️ Suggested doc fix
- /// Owner of the Light Token account (signer, payer for top-up)
+ /// Owner of the Light Token account (signer; writable and payer for top-up only when `max_top_up` is `Some`)📝 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.
| /// Owner of the Light Token account (signer, payer for top-up) | |
| /// Owner of the Light Token account (signer; writable and payer for top-up only when `max_top_up` is `Some`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/token-sdk/src/instruction/revoke.rs` at line 24, Update the doc
comment for the owner field in revoke.rs to match the fix applied in approve.rs:
remove or reword the parenthetical "(payer for top-up)" so the comment reads
only that this is the owner/signing authority of the Light Token account (e.g.,
"Owner of the Light Token account (signer)") or another clarified phrasing used
in approve.rs; locate the comment attached to the owner field/struct in
revoke.rs and apply the same wording change.
tilo-14
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-libs/token-sdk/Cargo.toml`:
- Line 36: Add a "compressible" feature to the Cargo.toml [features] section and
wire it to the compressible crate so SDK macros get gated correctly (e.g. add a
compressible feature that enables the appropriate light-compressible feature
such as "light-compressible/solana"); also update the light-compressible
dependency declaration to explicitly enable the needed feature flags (e.g.
enable "solana" or the new compressible gate) so the dependency is built with
the correct feature set; reference the existing features block and the
light-compressible and light-token-interface entries to make these changes.
| light-compressed-account = { workspace = true, features = ["std", "solana"] } | ||
| light-compressible = { workspace = true } | ||
| light-token-interface = { workspace = true } | ||
| light-token-interface = { workspace = true, features = ["solana"] } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the sdk-libs/token-sdk/Cargo.toml file
fd "Cargo.toml" -p "sdk-libs/token-sdk" | head -1 | xargs cat -nRepository: Lightprotocol/light-protocol
Length of output: 3397
🏁 Script executed:
# Find the light-token-interface crate Cargo.toml to check feature definitions
fd "Cargo.toml" | xargs grep -l "^name = \"light-token-interface\"" 2>/dev/null | head -1 | xargs cat -nRepository: Lightprotocol/light-protocol
Length of output: 2804
🏁 Script executed:
# Check what the solana feature provides in light-token-interface
fd "Cargo.toml" | xargs grep -l "light-token-interface" | xargs grep -B5 -A15 "light-token-interface" | head -50Repository: Lightprotocol/light-protocol
Length of output: 3603
🏁 Script executed:
fd "Cargo.toml" | xargs grep -l "^name = \"light-compressible\"" | head -1 | xargs cat -nRepository: Lightprotocol/light-protocol
Length of output: 2356
🏁 Script executed:
# Also check if utils.rs uses any compressible features
fd "utils.rs" -p "sdk-libs/token-sdk" -type f | xargs cat -nRepository: Lightprotocol/light-protocol
Length of output: 242
🏁 Script executed:
fd "Cargo.toml" -p "sdk-libs/token-sdk" | head -1 | sed -n '1,40p' | cat -nRepository: Lightprotocol/light-protocol
Length of output: 108
🏁 Script executed:
# Search for utils.rs in token-sdk to understand what features it needs
find sdk-libs/token-sdk -name "*.rs" -type f | head -10Repository: Lightprotocol/light-protocol
Length of output: 538
🏁 Script executed:
# Get the full features section from token-sdk Cargo.toml
sed -n '9,28p' sdk-libs/token-sdk/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 967
🏁 Script executed:
# Check if compressible.rs uses any feature gates
head -30 sdk-libs/token-sdk/src/instruction/compressible.rsRepository: Lightprotocol/light-protocol
Length of output: 1355
🏁 Script executed:
# Check the README for token-sdk to understand the compressible feature
fd "README.md" -p "sdk-libs/token-sdk" | head -1 | xargs catRepository: Lightprotocol/light-protocol
Length of output: 5747
🏁 Script executed:
# Check if there's a compressible feature being added in the change
# Look for any references to compressible in token-sdk to see what needs this feature
grep -r "compressible" sdk-libs/token-sdk/src --include="*.rs" | head -20Repository: Lightprotocol/light-protocol
Length of output: 2144
🏁 Script executed:
# Check the lib.rs to see if there's conditional compilation for compressible
cat sdk-libs/token-sdk/src/lib.rs | head -50Repository: Lightprotocol/light-protocol
Length of output: 4778
🏁 Script executed:
# Check if token-sdk ever had a compressible feature in git history or if it's mentioned elsewhere
rg "compressible.*feature" sdk-libs/token-sdk --type markdown --type rustRepository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Check if there's a CLAUDE.md file with additional context for token-sdk
fd "CLAUDE.md" -p "sdk-libs/token-sdk"Repository: Lightprotocol/light-protocol
Length of output: 54
🏁 Script executed:
# Let me check the actual file in the PR to see exactly what changed
# Look at the entire dependencies section to understand the context
sed -n '30,62p' sdk-libs/token-sdk/Cargo.tomlRepository: Lightprotocol/light-protocol
Length of output: 1385
🏁 Script executed:
# Check if compressible feature is expected to be on token-sdk itself (not on dependencies)
# by looking at README more carefully
grep -A5 -B5 "compressible" sdk-libs/token-sdk/README.mdRepository: Lightprotocol/light-protocol
Length of output: 622
Solana feature addition is correct, but the compressible feature is missing from the [features] section.
The features = ["solana"] addition to light-token-interface is correctly identified as necessary—the anchor feature does not implicitly enable solana in that crate. However, the review misses a requirement from the coding guidelines: the token-sdk Cargo.toml should define and enable a compressible feature to gate utility functions for compressible SDK macros (as documented in the README).
The compressible feature should be added to the [features] section:
Suggested addition
[features]
default = []
v1 = ["light-compressed-token-sdk/v1"]
anchor = ["anchor-lang", "light-token-types/anchor", "light-token-interface/anchor", "light-compressed-token-sdk/anchor", "dep:light-sdk-macros"]
compressible = ["light-compressible/solana"] # Add this line
idl-build = ["anchor", "anchor-lang/idl-build", "light-sdk/idl-build", "light-compressed-token-sdk/idl-build"]
cpi-context = ["light-sdk/cpi-context", "light-compressed-token-sdk/cpi-context"]The light-compressible dependency on line 35 also lacks explicit feature flags—it should enable appropriate features (likely solana or the new compressible when that module is used).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/token-sdk/Cargo.toml` at line 36, Add a "compressible" feature to
the Cargo.toml [features] section and wire it to the compressible crate so SDK
macros get gated correctly (e.g. add a compressible feature that enables the
appropriate light-compressible feature such as "light-compressible/solana");
also update the light-compressible dependency declaration to explicitly enable
the needed feature flags (e.g. enable "solana" or the new compressible gate) so
the dependency is built with the correct feature set; reference the existing
features block and the light-compressible and light-token-interface entries to
make these changes.
program-tests/compressed-token-test/tests/light_token/approve_revoke.rs
Outdated
Show resolved
Hide resolved
| let owner_meta = if self.max_top_up.is_some() { | ||
| AccountMeta::writable_signer(self.owner.key()) | ||
| } else { | ||
| AccountMeta::readonly_signer(self.owner.key()) | ||
| }; |
There was a problem hiding this comment.
shouldn't we have an additional fee payer instead of making owner mutable?
tilo-14
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/token-sdk/src/instruction/transfer_checked.rs (1)
140-174: 🧹 Nitpick | 🔵 TrivialConsider extracting the repeated authority-writability + fee_payer account pattern.
All six instruction modules (Transfer, TransferChecked, Burn, BurnChecked, MintTo, MintToChecked) duplicate the same authority-meta logic and fee_payer append pattern. A small shared helper could reduce the surface area for future divergence:
fn authority_meta(authority: Pubkey, fee_payer: &Option<Pubkey>) -> AccountMeta { if fee_payer.is_none() { AccountMeta::new(authority, true) } else { AccountMeta::new_readonly(authority, true) } }Not blocking — the current code is clear and each module is self-contained — but worth considering if more instruction types follow the same pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/src/instruction/transfer_checked.rs` around lines 140 - 174, The authority-writability and fee_payer append logic is duplicated in TransferChecked::instruction (authority_meta local variable and fee_payer handling) and five other modules (Transfer, Burn, BurnChecked, MintTo, MintToChecked); extract a small helper like authority_meta(authority: Pubkey, fee_payer: &Option<Pubkey>) -> AccountMeta that returns AccountMeta::new(...) when fee_payer.is_none() and AccountMeta::new_readonly(...) otherwise, and replace the inline authority_meta creation in TransferChecked and the other instruction methods, then centralize the fee_payer append pattern by reusing the same optional push logic to keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk-libs/token-sdk/src/instruction/transfer_checked.rs`:
- Around line 140-174: The authority-writability and fee_payer append logic is
duplicated in TransferChecked::instruction (authority_meta local variable and
fee_payer handling) and five other modules (Transfer, Burn, BurnChecked, MintTo,
MintToChecked); extract a small helper like authority_meta(authority: Pubkey,
fee_payer: &Option<Pubkey>) -> AccountMeta that returns AccountMeta::new(...)
when fee_payer.is_none() and AccountMeta::new_readonly(...) otherwise, and
replace the inline authority_meta creation in TransferChecked and the other
instruction methods, then centralize the fee_payer append pattern by reusing the
same optional push logic to keep behavior identical.
| let transfer_ix = TransferChecked { | ||
| source, | ||
| mint, | ||
| destination, | ||
| amount: 100, | ||
| decimals: 9, | ||
| authority: owner_keypair.pubkey(), | ||
| max_top_up: Some(1), | ||
| fee_payer: None, | ||
| } | ||
| .instruction() | ||
| .unwrap(); | ||
| // Build raw instruction with low max_top_up to test on-chain error path | ||
| // (TransferChecked struct no longer exposes max_top_up) | ||
| let transfer_ix = { | ||
| use anchor_lang::prelude::AccountMeta; | ||
| use solana_sdk::instruction::Instruction; | ||
|
|
||
| // Discriminator (12) + amount (8) + decimals (1) + max_top_up (2) | ||
| let mut data = vec![12u8]; | ||
| data.extend_from_slice(&100u64.to_le_bytes()); | ||
| data.push(9u8); // decimals | ||
| data.extend_from_slice(&1u16.to_le_bytes()); // max_top_up = 1 | ||
|
|
||
| Instruction { | ||
| program_id: light_compressed_token::ID, | ||
| accounts: vec![ | ||
| AccountMeta::new(source, false), | ||
| AccountMeta::new_readonly(mint, false), | ||
| AccountMeta::new(destination, false), | ||
| AccountMeta::new(owner_keypair.pubkey(), true), | ||
| AccountMeta::new_readonly(solana_sdk::system_program::ID, false), | ||
| ], | ||
| data, | ||
| } | ||
| }; |
| AccountMeta::new(ata, false), // source | ||
| AccountMeta::new(mint_pda, false), // mint | ||
| AccountMeta::new_readonly(pda_owner, false), // PDA authority (program signs) | ||
| AccountMeta::new(pda_owner, false), // PDA authority (writable, pays for top-ups) |
There was a problem hiding this comment.
also likely unrelated
There was a problem hiding this comment.
why did you change anything in this file it doesn't seem related
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk-libs/token-sdk/src/instruction/approve.rs (1)
25-38: 🧹 Nitpick | 🔵 TrivialMandatory
fee_payerfield is accepted but silently ignored — consider documenting this more prominently.
fee_payeris required in bothApproveandApproveCpi, butinstruction()(line 116–127) never includes it in theAccountMetalist, andinvoke()/invoke_signed()never includes it in theaccount_infosarray. The TODO comments explain the rationale, but a caller constructing this struct has no compile-time or runtime signal that the field is unused.Consider adding a doc comment on the struct itself (not just inline TODO) clarifying that
fee_payeris reserved for future use and currently has no effect, or alternatively, make itOption<Pubkey>withNoneas the default until on-chain support lands. This would prevent callers from assuming their fee_payer account is actually funding top-ups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/src/instruction/approve.rs` around lines 25 - 38, The Approve struct declares a mandatory fee_payer but the instruction() function and CPI paths (Approve and ApproveCpi, and their invoke()/invoke_signed() uses) never include it in AccountMeta/account_infos, so callers are misled; either document this on the struct-level doc comment stating fee_payer is reserved and currently ignored, or change the field to Option<Pubkey> (e.g., fee_payer: Option<Pubkey>) and update constructors/serializers to default to None until on-chain support exists; also update the instruction() method and any CPI constructors to accept the Option and continue omitting the account when None so behavior is explicit.sdk-libs/token-sdk/src/instruction/revoke.rs (1)
59-74: 🧹 Nitpick | 🔵 TrivialCPI
account_infosexcludesfee_payer— ensure callers providing a realfee_payeraren't surprised.
invokeandinvoke_signedbuildaccount_infosfrom only[token_account, owner, system_program], silently droppingself.fee_payer. A caller who constructsRevokeCpi { ..., fee_payer: some_real_account }might expect that account to participate in the CPI. The struct requires it, but the invocation ignores it.This is the same observation as the pinocchio layer. Since the on-chain program doesn't support it yet, this is correct behavior — but worth a brief inline comment at the
account_infosconstruction site explaining whyfee_payeris excluded:Suggested clarification
pub fn invoke(self) -> Result<(), ProgramError> { let instruction = Revoke::from(&self).instruction()?; + // fee_payer is not included — on-chain revoke doesn't support a separate fee payer yet. let account_infos = [self.token_account, self.owner, self.system_program]; invoke(&instruction, &account_infos) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk-libs/token-sdk/src/instruction/revoke.rs` around lines 59 - 74, The CPI methods invoke and invoke_signed on RevokeCpi currently build account_infos from only [self.token_account, self.owner, self.system_program], omitting self.fee_payer; add a brief inline comment at the account_infos construction in both RevokeCpi::invoke and RevokeCpi::invoke_signed explaining that fee_payer is intentionally excluded because the on-chain Revoke instruction does not accept a fee_payer account (so callers supplying one will be ignored), and keep the current behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk-libs/token-pinocchio/src/instruction/revoke.rs`:
- Around line 28-34: The RevokeCpi struct's pub field fee_payer is required but
never used by the invoke logic (see RevokeCpi and the invoke call paths), which
can silently mislead callers; annotate the field to surface that fact at compile
time and in docs by adding attributes to the fee_payer field (e.g.,
#[deprecated(note = "fee_payer is accepted for future on-chain support but is
not used; owner pays rent today")] and/or #[doc(hidden)]) and update its doc
comment to clearly state it's ignored by current invoke paths; keep the field
pub but ensure the message directs users to not rely on it until on-chain
support is implemented.
In `@sdk-libs/token-sdk/src/instruction/transfer.rs`:
- Around line 106-150: Multiple instruction types (e.g., Transfer, MintTo,
MintToChecked, Burn, BurnChecked) duplicate the same impl pattern
(with_max_top_up, FooWithTopUp, From<&FooCpi>, build_instruction(Option<u16>),
instruction()); refactor by extracting a reusable declarative macro or trait
that generates these impls to remove boilerplate: define a macro that takes the
struct name, discriminator byte, which fields differ (e.g., include_decimals
flag, amount vs decimals), and auto-generates with_max_top_up, FooWithTopUp,
From<&FooCpi>, instruction, and build_instruction(Option<u16>) implementations,
then replace the per-type impls (e.g., Transfer, TransferWithTopUp,
build_instruction) with macro invocations to produce identical behavior while
preserving existing function names and signatures.
---
Outside diff comments:
In `@sdk-libs/token-sdk/src/instruction/approve.rs`:
- Around line 25-38: The Approve struct declares a mandatory fee_payer but the
instruction() function and CPI paths (Approve and ApproveCpi, and their
invoke()/invoke_signed() uses) never include it in AccountMeta/account_infos, so
callers are misled; either document this on the struct-level doc comment stating
fee_payer is reserved and currently ignored, or change the field to
Option<Pubkey> (e.g., fee_payer: Option<Pubkey>) and update
constructors/serializers to default to None until on-chain support exists; also
update the instruction() method and any CPI constructors to accept the Option
and continue omitting the account when None so behavior is explicit.
In `@sdk-libs/token-sdk/src/instruction/revoke.rs`:
- Around line 59-74: The CPI methods invoke and invoke_signed on RevokeCpi
currently build account_infos from only [self.token_account, self.owner,
self.system_program], omitting self.fee_payer; add a brief inline comment at the
account_infos construction in both RevokeCpi::invoke and
RevokeCpi::invoke_signed explaining that fee_payer is intentionally excluded
because the on-chain Revoke instruction does not accept a fee_payer account (so
callers supplying one will be ignored), and keep the current behavior unchanged.
| impl Transfer { | ||
| pub fn with_max_top_up(self, max_top_up: u16) -> TransferWithTopUp { | ||
| TransferWithTopUp { | ||
| inner: self, | ||
| max_top_up, | ||
| } | ||
| } | ||
|
|
||
| pub fn instruction(self) -> Result<Instruction, ProgramError> { | ||
| // Authority is writable only when max_top_up is set AND no fee_payer | ||
| // (authority pays for top-ups only if no separate fee_payer) | ||
| let authority_meta = if self.max_top_up.is_some() && self.fee_payer.is_none() { | ||
| AccountMeta::new(self.authority, true) | ||
| } else { | ||
| AccountMeta::new_readonly(self.authority, true) | ||
| }; | ||
| self.build_instruction(None) | ||
| } | ||
|
|
||
| let mut accounts = vec![ | ||
| fn build_instruction(self, max_top_up: Option<u16>) -> Result<Instruction, ProgramError> { | ||
| let accounts = vec![ | ||
| AccountMeta::new(self.source, false), | ||
| AccountMeta::new(self.destination, false), | ||
| authority_meta, | ||
| // System program required for rent top-up CPIs | ||
| AccountMeta::new_readonly(self.authority, true), | ||
| AccountMeta::new_readonly(Pubkey::default(), false), | ||
| AccountMeta::new(self.fee_payer, true), | ||
| ]; | ||
|
|
||
| // Add fee_payer if provided (must be signer and writable) | ||
| if let Some(fee_payer) = self.fee_payer { | ||
| accounts.push(AccountMeta::new(fee_payer, true)); | ||
| let mut data = vec![3u8]; | ||
| data.extend_from_slice(&self.amount.to_le_bytes()); | ||
| if let Some(max_top_up) = max_top_up { | ||
| data.extend_from_slice(&max_top_up.to_le_bytes()); | ||
| } | ||
|
|
||
| Ok(Instruction { | ||
| program_id: Pubkey::from(LIGHT_TOKEN_PROGRAM_ID), | ||
| accounts, | ||
| data: { | ||
| let mut data = vec![3u8]; | ||
| data.extend_from_slice(&self.amount.to_le_bytes()); | ||
| // Include max_top_up if set (10-byte format) | ||
| if let Some(max_top_up) = self.max_top_up { | ||
| data.extend_from_slice(&max_top_up.to_le_bytes()); | ||
| } | ||
| data | ||
| }, | ||
| data, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| pub struct TransferWithTopUp { | ||
| inner: Transfer, | ||
| max_top_up: u16, | ||
| } | ||
|
|
||
| impl TransferWithTopUp { | ||
| pub fn instruction(self) -> Result<Instruction, ProgramError> { | ||
| self.inner.build_instruction(Some(self.max_top_up)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a macro or trait to reduce boilerplate across instruction types.
Transfer, MintTo, MintToChecked, Burn, and BurnChecked all implement the same structural pattern: a Foo struct, FooCpi, From<&FooCpi>, build_instruction(Option<u16>), with_max_top_up, and FooWithTopUp. The only differences are field names, discriminators, and whether decimals is included. A declarative macro generating these impls could cut ~500 lines of near-identical code and prevent future drift between instruction types.
Not a blocker — the current approach is clear and each file is self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk-libs/token-sdk/src/instruction/transfer.rs` around lines 106 - 150,
Multiple instruction types (e.g., Transfer, MintTo, MintToChecked, Burn,
BurnChecked) duplicate the same impl pattern (with_max_top_up, FooWithTopUp,
From<&FooCpi>, build_instruction(Option<u16>), instruction()); refactor by
extracting a reusable declarative macro or trait that generates these impls to
remove boilerplate: define a macro that takes the struct name, discriminator
byte, which fields differ (e.g., include_decimals flag, amount vs decimals), and
auto-generates with_max_top_up, FooWithTopUp, From<&FooCpi>, instruction, and
build_instruction(Option<u16>) implementations, then replace the per-type impls
(e.g., Transfer, TransferWithTopUp, build_instruction) with macro invocations to
produce identical behavior while preserving existing function names and
signatures.
…se, approve, revoke close: owner changed from writable to readonly — on-chain uses next_signer (signer-only check), never writes to owner account. approve/revoke: added max_top_up: Option<u16> field. Owner is now readonly by default (max_top_up: None) and only writable when max_top_up is Some, matching the existing pattern in transfer and burn. This prevents privilege escalation failures when calling programs pass authority as read-only.
Delegate to Token::amount_from_account_info and return ProgramError instead of TokenSdkError so callers can use ? directly in Anchor contexts without redundant .map_err().
Authority must be writable when no fee_payer is set since it pays for compressible account rent top-ups.
Restore the raw builder test pattern that verifies the on-chain MaxTopUpExceeded error path. These tests use the SDK instruction builder, then append max_top_up bytes directly to test the on-chain limit — same pattern used for Transfer.
…er max_top_up Redesign all token instruction structs so that fee_payer is a mandatory field and authority/owner is always readonly (fee_payer pays for top-ups instead). Remove max_top_up from struct fields and add .with_max_top_up() builder pattern that appends 2 bytes to the wire format. Exception: Approve/Revoke keep owner writable (on-chain doesn't support fee_payer yet), but the fee_payer field exists for API consistency. Update all CPI structs (solana AccountInfo + pinocchio) accordingly, and fix invoke_signed test programs to pass a separate fee_payer account since PDA authority != transaction fee payer.
tilo-14
force-pushed
the
fix/authority-owner-mutability
branch
from
e16cfb6 to
2fff677
Compare
fee_payer must be at index 4 to match TransferCpi handler field order. LIGHT_TOKEN_PROGRAM_ID at index 5 is still resolved by the CPI runtime.
| @@ -52,6 +55,7 @@ impl<'info> ApproveCpi<'info> { | |||
| AccountMeta::readonly(self.delegate.key()), | |||
| AccountMeta::writable_signer(self.owner.key()), | |||
There was a problem hiding this comment.
| AccountMeta::writable_signer(self.owner.key()), | |
| AccountMeta::readonly_signer(self.owner.key()), |
…se; align transfer_checked - token-sdk approve/revoke: owner AccountMeta new -> new_readonly (readonly signer) - token-pinocchio approve/revoke/close: owner writable_signer -> readonly_signer - token-pinocchio transfer_checked: remove optional fee_payer, make mandatory; authority always readonly_signer; use Pubkey::default() sentinel for system_program in account_metas - token-client revoke: add owner-mismatch guard in execute_with_owner matching approve
Add separate fee_payer support (non-PDA authority) to the pinocchio test program and integration tests: - src: add process_*_invoke_with_fee_payer handlers for approve, revoke, transfer, burn, ctoken_mint_to; add InstructionType variants 36-40 and dispatch in lib.rs - tests: fix invoke_signed tests to include fee_payer account; add *_invoke_with_separate_fee_payer tests for all five operations demonstrating authority != fee_payer separation - pda_owner accounts in invoke_signed tests changed to new_readonly
The TransferCheckedCpi handler was updated to require a separate fee_payer at accounts[5], but the three transfer_checked tests still passed light_token_program there, causing PrivilegeEscalation. Add payer as writable fee_payer at [5] and move light_token_program to [6] in all three test_ctoken_transfer_checked_* tests.
- Fix approve/revoke invoke_signed: add separate fee_payer at accounts[5]/[4]
- Add process_{transfer,burn,mint_to,approve,revoke}_invoke_with_fee_payer handlers
- Add discriminators 36-40 to InstructionType enum and dispatch
- Add test_{transfer,burn,mint_to,approve,revoke}_invoke_with_separate_fee_payer tests
2 participants
Summary
max_top_up: Option<u16>from all user-facing token-sdk instruction structs (Transfer, TransferChecked, Burn, BurnChecked, MintTo, MintToChecked, TransferInterface, Approve, Revoke)fee_payeris provided (matches pinocchio behavior)u16::MAX(matches pinocchio)ProgramErrorfromget_token_account_balanceDetails
max_top_upis an implementation detail — the on-chain program defaults tou16::MAXwhen not specified (short format). Pinocchio already sends short format withfee_payer-based authority mutability. Token-sdk on main had a latent bug: authority was readonly in the no-fee-payer path, but on-chain would attempt top-ups (defaulting tou16::MAX), causing transactions to fail when top-up was actually needed.Test plan
cargo test -p light-token— 52 passed (unit tests + doctests)cargo test-sbf -p compressed-token-test --test light_token— 77 passedcargo test-sbf -p sdk-light-token-test— all passedSummary by CodeRabbit
Breaking Changes
Bug Fixes
Chores