Skip to content

fix(sdk): align token-sdk authority mutability and wire format with pinocchio#2301

Open
tilo-14 wants to merge 29 commits intomainfrom
fix/authority-owner-mutability
Open

fix(sdk): align token-sdk authority mutability and wire format with pinocchio#2301
tilo-14 wants to merge 29 commits intomainfrom
fix/authority-owner-mutability

Conversation

@tilo-14
Copy link
Member

@tilo-14 tilo-14 commented Feb 19, 2026

Summary

  • Remove max_top_up: Option<u16> from all user-facing token-sdk instruction structs (Transfer, TransferChecked, Burn, BurnChecked, MintTo, MintToChecked, TransferInterface, Approve, Revoke)
  • Authority is writable when no fee_payer is provided (matches pinocchio behavior)
  • Use short wire format (no max_top_up bytes) — on-chain defaults to u16::MAX (matches pinocchio)
  • Return ProgramError from get_token_account_balance

Details

max_top_up is an implementation detail — the on-chain program defaults to u16::MAX when not specified (short format). Pinocchio already sends short format with fee_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 to u16::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 passed
  • cargo test-sbf -p sdk-light-token-test — all passed
  • Max top-up exceeded tests preserved using raw instruction builders (approve, revoke, transfer, transfer_checked)

Summary by CodeRabbit

  • Breaking Changes

    • Removed max_top_up from token instructions; fee_payer is now a required field in many instruction/CPI APIs.
    • Token instructions and CPIs now always include fee_payer in account lists.
    • Token balance helper now returns generic program errors instead of SDK-specific errors.
  • Bug Fixes

    • Owner account in close instruction marked read-only while retaining signer status.
  • Chores

    • Enabled "solana" feature on a dependency.

Open with Devin

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 19, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Removed optional max_top_up from many token instruction structs/CPI paths and made fee_payer mandatory in multiple CPIs; instruction data no longer encodes max_top_up. Close instruction changed owner AccountMeta from writable to readonly (still signer). A utility function error type changed to ProgramError. Dependency feature flag added for light-token-interface.

Changes

Cohort / File(s) Summary
Close & utils
sdk-libs/token-sdk/src/instruction/close.rs, sdk-libs/token-sdk/src/utils.rs
CloseAccount owner AccountMeta changed from writable to readonly signer. get_token_account_balance now returns Result<u64, ProgramError> and uses Token::amount_from_account_info.
Core instructions — removed max_top_up & fee_payer mandatory
sdk-libs/token-sdk/src/instruction/burn.rs, .../burn_checked.rs, .../mint_to.rs, .../mint_to_checked.rs, .../transfer.rs, .../transfer_checked.rs, .../transfer_interface.rs
Dropped max_top_up: Option<u16> fields; converted optional fee_payer to mandatory fee_payer (Pubkey / AccountInfo) and unified build_instruction flow; added WithTopUp wrapper types for explicit top-up variants.
Token-client action updates
sdk-libs/token-client/src/actions/mint_to.rs, .../transfer.rs, .../transfer_checked.rs, .../transfer_interface.rs, .../approve.rs, .../revoke.rs
Instruction constructors now include fee_payer = payer.pubkey() (removed conditional fee_payer branches); max_top_up omitted from payloads.
Pinocchio CPI structs & examples
sdk-libs/token-pinocchio/src/instruction/* (burn*, mint_to*, transfer*, approve.rs, revoke.rs, transfer_interface.rs), sdk-libs/token-pinocchio/src/lib.rs
CPI structs updated to require fee_payer: &AccountInfo (removed Option), examples/docs updated; unified single invocation path (no fee_payer branching).
Approve & Revoke public APIs
sdk-libs/token-sdk/src/instruction/approve.rs, sdk-libs/token-sdk/src/instruction/revoke.rs
Added fee_payer: Pubkey to public structs and fee_payer: AccountInfo<'info> to CPI structs; From<&...Cpi> mappings updated.
Tests
sdk-libs/token-sdk/tests/instruction_close.rs, sdk-libs/token-sdk/tests/instruction_transfer.rs, sdk-libs/token-sdk/tests/transfer_type.rs
Updated tests to expect owner readonly for Close; transfer tests updated to include fee_payer as writable signer and removed/adjusted max_top_up expectations.
Cargo / dependency
sdk-libs/token-sdk/Cargo.toml
Enabled features = ["solana"] for light-token-interface workspace dependency.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

ai-review

Suggested reviewers

  • ananas-block

Poem

✂️ max_top_up trimmed, fields simplified and neat,
Fee_payer now present, accounts line up complete.
Close owner stays readonly, signer kept in place,
Errors unified, data trimmed—clean API face.
A small refactor, tidy steps toward grace.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: aligning token-sdk authority mutability and wire format with pinocchio behavior, which is the core objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 70.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/authority-owner-mutability

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 when max_top_up is None.

The owner is only writable (and therefore can pay for a top-up) when max_top_up is Some. With None, 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 | 🟡 Minor

Semver-breaking change confirmed — missing #[non_exhaustive] on public structs.

Approve, ApproveCpi, Revoke, and RevokeCpi lack the #[non_exhaustive] attribute. Adding max_top_up as a mandatory field breaks code that initializes these structs with explicit field syntax. Verification shows test code and From implementations 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_owner silently ignores self.owner, unlike the symmetric Approve::execute_with_owner.

Approve::execute_with_owner validates that self.owner (when Some) matches the provided owner keypair and returns a descriptive RpcError on mismatch. Revoke::execute_with_owner ignores self.owner entirely and uses owner.pubkey() unconditionally. A caller who sets Revoke { token_account, owner: Some(pubkey_A) } and calls execute_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 | 🔵 Trivial

Add unit tests for approve and revoke instruction changes.

This file tests the close instruction changes thoroughly. The analogous approve/revoke instruction changes — conditional owner writability and variable instruction data depending on max_top_up — have no equivalent unit tests. The close-test pattern is a good template.

Specifically missing:

  • max_top_up: None → owner is new_readonly, data is 9 bytes ([4, amount...])
  • max_top_up: Some(x) → owner is new (writable), data is 11 bytes ([4, amount..., x...])
  • Same two cases for revoke (discriminator 5, no amount, data is 1 or 3 bytes)

Do you want me to generate the test file(s) for instruction_approve.rs and instruction_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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
/// 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 tilo-14 changed the title fix(token-sdk, token-pinocchio): make authority/owner readonly in close, approve, revoke fix(token-sdk): make authority/owner readonly, return ProgramError from get_token_account_balance Feb 19, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -n

Repository: 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 -n

Repository: 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 -50

Repository: Lightprotocol/light-protocol

Length of output: 3603


🏁 Script executed:

fd "Cargo.toml" | xargs grep -l "^name = \"light-compressible\"" | head -1 | xargs cat -n

Repository: 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 -n

Repository: Lightprotocol/light-protocol

Length of output: 242


🏁 Script executed:

fd "Cargo.toml" -p "sdk-libs/token-sdk" | head -1 | sed -n '1,40p' | cat -n

Repository: 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 -10

Repository: 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.toml

Repository: 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.rs

Repository: 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 cat

Repository: 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 -20

Repository: 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 -50

Repository: 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 rust

Repository: 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.toml

Repository: 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.md

Repository: 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.

@tilo-14 tilo-14 changed the title fix(token-sdk): make authority/owner readonly, return ProgramError from get_token_account_balance fix(sdk): make authority/owner readonly, return ProgramError from get_token_account_balance Feb 19, 2026
@tilo-14 tilo-14 changed the title fix(sdk): make authority/owner readonly, return ProgramError from get_token_account_balance fix(sdk): align token-sdk authority mutability and wire format with pinocchio Feb 19, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔵 Trivial

Consider 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.

coderabbitai[bot]

This comment was marked as resolved.

tilo-14 added 15 commits February 20, 2026 18:53
…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 tilo-14 force-pushed the fix/authority-owner-mutability branch from e16cfb6 to 2fff677 Compare February 20, 2026 18:55
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.
…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
tilo-14 added 2 commits February 24, 2026 19:02
Add top-level mint field to TransferInterface/TransferInterfaceCpi so
LightToLight path uses TransferChecked (disc 12, validates decimals)
instead of plain Transfer (disc 3). Require mint account in test
handlers (min accounts 7→8) and pass it in LightToLight test callsites.
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

@tilo-14 tilo-14 force-pushed the fix/authority-owner-mutability branch from 1712583 to 002e9c8 Compare February 25, 2026 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants