Conversation
📝 WalkthroughWalkthroughThis PR changes the sentinel for "no limit" on max_top_up from 0 to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
programs/compressed-token/program/docs/ctoken/MINT_TO_CHECKED.md (2)
8-8:⚠️ Potential issue | 🟡 MinorDocumentation still references old semantics: "0 means no limit".
Line 8 says "where 0 means no limit" but the new semantics are
u16::MAX = no limit, 0 = no top-ups allowed. This contradicts Line 22 within the same file and the actual code inmint_to.rs.Proposed fix
-Mints tokens from a decompressed CMint account to a destination CToken account with decimals validation, fully compatible with SPL Token MintToChecked semantics. Uses pinocchio-token-program to process the mint_to_checked operation which handles balance/supply updates, authority validation, frozen account checks, and decimals validation. After minting, automatically tops up compressible accounts with additional lamports if needed to prevent accounts from becoming compressible during normal operations. Both CMint and destination CToken can receive top-ups based on their current slot and account balance. Supports max_top_up parameter to limit rent top-up costs where 0 means no limit. +Mints tokens from a decompressed CMint account to a destination CToken account with decimals validation, fully compatible with SPL Token MintToChecked semantics. Uses pinocchio-token-program to process the mint_to_checked operation which handles balance/supply updates, authority validation, frozen account checks, and decimals validation. After minting, automatically tops up compressible accounts with additional lamports if needed to prevent accounts from becoming compressible during normal operations. Both CMint and destination CToken can receive top-ups based on their current slot and account balance. Supports max_top_up parameter to limit rent top-up costs where u16::MAX means no limit and 0 means no top-ups allowed.
66-66:⚠️ Potential issue | 🟡 MinorLegacy default description is stale — says "0 (no limit)" but code now uses
u16::MAX.Line 66 states "Default to 0 (no limit) if only 9 bytes provided" but the implementation defaults to
u16::MAXfor the legacy 9-byte format. This directly contradicts the updated semantics on Line 22.Proposed fix
- - Default to 0 (no limit) if only 9 bytes provided (legacy format) + - Default to u16::MAX (no limit) if only 9 bytes provided (legacy format)programs/compressed-token/program/docs/ctoken/REVOKE.md (1)
57-60:⚠️ Potential issue | 🟡 MinorDoc-code inconsistency: guard condition doesn't match implementation.
Line 58 states:
If max_top_up > 0 and transfer_amount > max_top_up: return MaxTopUpExceeded
But the actual code in
approve_revoke.rs(line 116) checks:if max_top_up != u16::MAX && transfer_amount > max_top_up as u64 {The difference matters:
max_top_up > 0would skip the check whenmax_top_up == 0(old "no limit" semantics), whilemax_top_up != u16::MAXenforces the check whenmax_top_up == 0(new "no top-ups allowed" semantics). The doc should read:If max_top_up != u16::MAX and transfer_amount > max_top_up: return MaxTopUpExceeded
📝 Suggested fix
- - If max_top_up > 0 and transfer_amount > max_top_up: return MaxTopUpExceeded + - If max_top_up != u16::MAX and transfer_amount > max_top_up: return MaxTopUpExceededprograms/compressed-token/program/docs/ctoken/BURN.md (2)
8-8:⚠️ Potential issue | 🟡 MinorStale semantics in description: "0 = no limit" should be updated.
The description paragraph still contains the old interpretation:
Supports max_top_up parameter to limit rent top-up costs (0 = no limit).
This contradicts the updated semantics elsewhere in this same document (line 18) where
u16::MAX = no limit, 0 = no top-ups allowed. The description should be updated to match.📝 Suggested fix
- Supports max_top_up parameter to limit rent top-up costs (0 = no limit). + Supports max_top_up parameter to limit rent top-up costs (u16::MAX = no limit, 0 = no top-ups allowed).
95-98:⚠️ Potential issue | 🟠 MajorCorrect the budget guard sentinel in BURN.md line 98.
Line 98 documents the wrong sentinel value. The condition should be
max_top_up != u16::MAX, notmax_top_up != 0. The actual implementation incompressible_top_up.rs(line 77) correctly usesu16::MAX. With the documented condition,max_top_up = 0("no top-ups allowed") would paradoxically skip the budget check and permit top-ups, violating the intended semantics.Update line 98 to:
If max_top_up != u16::MAX and lamports_budget == 0, fail with MaxTopUpExceeded
programs/compressed-token/program/docs/ctoken/MINT_TO.md (3)
8-8:⚠️ Potential issue | 🟡 MinorStale semantics in description: "0 means no limit".
Same issue as BURN.md — the description paragraph retains the old interpretation:
Supports max_top_up parameter to limit rent top-up costs where 0 means no limit
Should reflect the new semantics (
u16::MAX = no limit, 0 = no top-ups allowed), consistent with line 20 of this same file.
62-64:⚠️ Potential issue | 🟡 MinorStale default value in parsing description.
Lines 62-64 state:
Parse max_top_up from bytes 8-10 if present (10-byte format)
Default to 0 (no limit) if only 8 bytes provided (legacy format)Line 56 was correctly updated to say
u16::MAX (no limit, legacy format), but lines 63 still says "Default to 0 (no limit)". This should be "Default to u16::MAX (no limit)".📝 Suggested fix
- Parse max_top_up from bytes 8-10 if present (10-byte format) - - Default to 0 (no limit) if only 8 bytes provided (legacy format) + - Default to u16::MAX (no limit) if only 8 bytes provided (legacy format) - Return InvalidInstructionData if length is invalid (not 8 or 10 bytes)
92-96:⚠️ Potential issue | 🟡 MinorUpdate line 96 to use the correct sentinel value for the budget guard:
Line 96 should read:
If budget reaches 0 and max_top_up is not u16::MAX, fail with MaxTopUpExceeded
The current condition ("max_top_up is not 0") is incorrect. With max_top_up semantics where u16::MAX = no limit and 0 = no top-ups allowed, the guard must check
max_top_up != u16::MAX. Otherwise, when max_top_up = 0 (no top-ups), the budget check would be skipped, allowing top-ups anyway.This matches the actual implementation across multiple files (compressible_top_up.rs, process_actions.rs, transfer/shared.rs, approve_revoke.rs, etc.), all of which correctly use
if max_top_up != u16::MAX && lamports_budget == 0.programs/compressed-token/program/docs/ctoken/TRANSFER.md (1)
107-108:⚠️ Potential issue | 🟡 MinorAmbiguous budget check description — likely stale semantics.
Line 107:
Check max_top_up budget if set (non-zero)
With the new semantics, "non-zero" is misleading.
0now means "no top-ups allowed" (which is set — to block top-ups), whileu16::MAXmeans "no limit" (effectively unset). Consider:📝 Suggested fix
- - Check max_top_up budget if set (non-zero) + - Check max_top_up budget if not unlimited (not u16::MAX)programs/compressed-token/program/docs/ctoken/APPROVE.md (2)
65-66:⚠️ Potential issue | 🟡 MinorDoc-code inconsistency: guard condition uses old semantics.
Same issue as REVOKE.md — line 66 states:
If max_top_up > 0 and transfer_amount > max_top_up: return MaxTopUpExceeded
The actual code checks
max_top_up != u16::MAX. With the old> 0check,max_top_up = 0would skip enforcement, contradicting the new "0 = no top-ups allowed" semantics.📝 Suggested fix
- - If max_top_up > 0 and transfer_amount > max_top_up: return MaxTopUpExceeded + - If max_top_up != u16::MAX and transfer_amount > max_top_up: return MaxTopUpExceeded
114-138:⚠️ Potential issue | 🟡 MinorCode snippets and description in "Comparison" section still use old semantics.
Three stale references remain in this section:
- Line 116:
if max_top_up > 0 && transfer_amount > max_top_up as u64— should bemax_top_up != u16::MAX- Line 132:
max_top_up (u16, 0 = no limit)— should beu16::MAX = no limit, 0 = no top-ups allowed- Line 136:
if max_top_up > 0 && transfer_amount > max_top_up as u64— should bemax_top_up != u16::MAXThese are the "Comparison with Token-2022" code examples. Since this documentation is meant to be an authoritative reference (per coding guidelines requiring doc-code accuracy), they should reflect the current implementation.
🤖 Fix all issues with AI agents
In `@programs/compressed-token/program/docs/ctoken/TRANSFER_CHECKED.md`:
- Around line 84-86: The wording is ambiguous because "Non-zero" includes
u16::MAX which is defined as "no limit"; update the phrasing around max_top_up
to explicitly treat u16::MAX as the no-limit sentinel and other non-zero values
as the actual cap: state "u16::MAX = no limit on top-up lamports, 0 = no top-ups
allowed, any other non-zero value = maximum combined lamports for source +
destination top-up" and ensure the sentence about transaction failure still
references max_top_up when the calculated top-up exceeds that cap.
In `@sdk-libs/token-sdk/src/instruction/burn_checked.rs`:
- Around line 37-39: Update the doc comment for the max_top_up field in
burn_checked.rs to reflect the new semantics: explain that the Option<u16>
distinguishes between omitted and explicit values so Some(0) is meaningful
(means "no top-ups allowed") and will be serialized into the 11-byte instruction
format; remove the phrase "When set to a non-zero value" and replace it with
wording that Any Some(value) (including Some(0)) is included in instruction data
while None omits it, and keep the existing note that u16::MAX means no limit.
Reference the max_top_up field to locate where to change the comment.
In `@sdk-libs/token-sdk/src/instruction/burn.rs`:
- Around line 34-36: Update the doc comment for the max_top_up field in burn.rs
to reflect current behavior: state that any Some(value) — including Some(0)
which means "no top-ups allowed" — is serialized into the instruction data, and
only None omits the field; reference the max_top_up field and the serialization
behavior in the struct so callers aren't misled by the old "non-zero" wording.
In `@sdk-libs/token-sdk/src/instruction/mint_to_checked.rs`:
- Around line 37-39: Update the field doc for max_top_up to accurately reflect
semantics: explain that max_top_up is an Option<u16> where None means the field
is omitted from instruction data, Some(0) is a valid explicit value that
disallows top-ups and will be serialized, and Some(n>0) includes that numeric
cap; reference the max_top_up field and the serialization logic that writes
max_top_up whenever it is Some(_) so callers understand that Some(0) is
preserved during serialization.
In `@sdk-libs/token-sdk/src/instruction/mint_to.rs`:
- Around line 34-36: The documentation for the struct field max_top_up in
mint_to.rs is misleading: it says "When set to a non-zero value" but the code
includes max_top_up in serialized instruction data for any Some(_) value
(including Some(0)). Update the comment for max_top_up to accurately reflect
that the presence of Some(value) controls inclusion in the instruction data and
that Some(0) explicitly means "no top-ups allowed" while None omits the field
(no top-up limit behavior), so change the wording around the field max_top_up to
state that serialization occurs for any Some(value) and clarify the semantics of
0 versus None.
In `@sdk-libs/token-sdk/src/instruction/transfer_checked.rs`:
- Around line 35-37: Update the doc comment for the max_top_up field in
transfer_checked.rs to remove the misleading "non-zero value" wording and
clearly describe serialization semantics: state that max_top_up is an
Option<u16>, that Some(0) is serialized and means "no top-ups allowed", and that
Some(n>0) is serialized and represents the maximum top-up; reference the
max_top_up field on the TransferChecked instruction to locate the comment to
edit.
ananas-block
force-pushed
the
jorrit/refactor-max_top_up
branch
from
3c250c4 to
11124dd
Compare
ananas-block
force-pushed
the
jorrit/refactor-max_top_up
branch
from
11124dd to
7eff552
Compare
SwenSchaeferjohann
left a comment
SwenSchaeferjohann
left a comment
There was a problem hiding this comment.
lgtm, added some notes on element
Since Some(0) is now meaningful (no top-ups allowed), the doc comments saying "non-zero value" were misleading. Updated SDK structs to say "When set (Some)" and TRANSFER_CHECKED.md to specify [1, u16::MAX-1].
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
programs/compressed-token/program/src/compressed_token/mint_action/actions/process_actions.rs (1)
52-56: 🧹 Nitpick | 🔵 TrivialConsider adding a comment linking the budget initialization to the sentinel check.
Lines 52–56 initialize the budget, but the "no limit" bypass lives 125 lines away on line 181. A one-line comment here would help future maintainers understand why the
u16::MAXcase doesn't need a special branch at initialization time.Suggested comment
// Initialize budget: +1 allows exact match (total == max_top_up) let max_top_up: u16 = parsed_instruction_data.max_top_up.get(); // max_top_up is in units of 1,000 lamports (max ~65.5M lamports). // +1 allows exact match (total == max_top_up * 1000). + // When max_top_up == u16::MAX (no limit), budget is still finite here; + // the sentinel is checked separately before the MaxTopUpExceeded error path. let mut lamports_budget = (max_top_up as u64).saturating_mul(1000).saturating_add(1);programs/compressed-token/program/docs/ctoken/MINT_TO.md (1)
92-96:⚠️ Potential issue | 🟡 MinorFix documentation to match implementation: update the max_top_up budget check condition in MINT_TO.md line 96.
The budget validation condition is inconsistent with both the actual implementation and BURN.md. Line 96 currently states:
"If budget reaches 0 and max_top_up is not 0, fail with MaxTopUpExceeded"
The implementation in
compressible_top_up.rs(line 78) checks:if max_top_up != u16::MAX && lamports_budget == 0 { return Err(TokenError::MaxTopUpExceeded.into()); }The comment in the code explicitly clarifies: "u16::MAX means no limit, 0 means no top-ups allowed." BURN.md documents this correctly at line 98. Update MINT_TO.md to match:
Suggested fix
- - If budget reaches 0 and max_top_up is not 0, fail with MaxTopUpExceeded + - If max_top_up != u16::MAX and lamports_budget == 0, fail with MaxTopUpExceededThe current wording is logically backwards: when
max_top_up = 0(no top-ups allowed), the condition "not 0" would be false, incorrectly allowing top-ups. The sentinel for unlimited isu16::MAX, not0.programs/compressed-token/program/docs/ctoken/BURN_CHECKED.md (1)
76-78:⚠️ Potential issue | 🟡 MinorClarify the
lamports_budgetformula in the documentation.Line 78 describes the budget initialization as "max_top_up + 1," which omits critical details. The actual implementation is
(max_top_up as u64).saturating_mul(1000).saturating_add(1), converting the u16 to u64 before multiplying by 1000 (the unit scaling mentioned on line 20). Sincelamports_budgetis u64, not u16, there's no overflow whenmax_top_up = u16::MAX. However, the doc should explicitly state:
lamports_budgetis a u64 type- The full formula:
(max_top_up as u64) * 1000 + 1- The +1 allows an exact match when total budget consumed equals
max_top_up * 1000This prevents reader confusion about type safety and budget calculation.
🤖 Fix all issues with AI agents
In `@programs/compressed-token/program/docs/ctoken/APPROVE.md`:
- Around line 64-68: Update the Step 4 bullet to match the actual check: when
comparing transfer_amount against max_top_up, include the 1,000-lamport unit
conversion used in the code (i.e., compare transfer_amount > (max_top_up as
u64).saturating_mul(1000)) and preserve the u16::MAX bypass check; reference the
same symbols top_up_lamports_from_account_info_unchecked, transfer_amount, and
max_top_up so the prose mirrors the implementation exactly.
In `@sdk-libs/token-sdk/src/instruction/burn_checked.rs`:
- Around line 72-73: The doc for BurnCheckedCpi.max_top_up is missing the second
sentence present on BurnChecked.max_top_up; update the doc comment on the field
BurnCheckedCpi::max_top_up to match BurnChecked::max_top_up by adding the line
"When set (Some), includes max_top_up in instruction data" so callers understand
the same serialization behavior (BurnCheckedCpi delegates to
BurnChecked::instruction()).
In `@sdk-libs/token-sdk/src/instruction/transfer_checked.rs`:
- Around line 73-74: The docs for TransferCheckedCpi are missing the explanatory
second line present on TransferChecked about how setting max_top_up changes the
serialized instruction length; update the TransferCheckedCpi struct
documentation to mirror TransferChecked by adding a second doc-line like "When
set (Some), includes max_top_up in instruction data" (referencing the
TransferCheckedCpi type and its max_top_up field) so CPI users understand the
9-byte vs 11-byte format toggle.
| - Calculate transfer_amount using `top_up_lamports_from_account_info_unchecked` | ||
| - If transfer_amount > 0: | ||
| - If max_top_up > 0 and transfer_amount > max_top_up: return MaxTopUpExceeded | ||
| - If max_top_up != u16::MAX and transfer_amount > max_top_up: return MaxTopUpExceeded | ||
| - Get payer account (index 2), return MissingPayer if not present | ||
| - Transfer lamports from payer to source via CPI |
There was a problem hiding this comment.
Step 4 bullet omits the 1,000-lamport unit conversion present in the actual check.
Line 66 states the condition as transfer_amount > max_top_up, but the real enforcement (shown in your own code example at line 117) is transfer_amount > (max_top_up as u64).saturating_mul(1000). This mismatch between the prose and the code snippet within the same doc can mislead readers.
Consider updating the bullet to:
- - If max_top_up != u16::MAX and transfer_amount > max_top_up: return MaxTopUpExceeded
+ - If max_top_up != u16::MAX and transfer_amount > max_top_up * 1000: return MaxTopUpExceededAs per coding guidelines: "Verify that all function signatures, struct definitions, and behavior described in the documentation accurately match the actual implementation."
🤖 Prompt for AI Agents
In `@programs/compressed-token/program/docs/ctoken/APPROVE.md` around lines 64 -
68, Update the Step 4 bullet to match the actual check: when comparing
transfer_amount against max_top_up, include the 1,000-lamport unit conversion
used in the code (i.e., compare transfer_amount > (max_top_up as
u64).saturating_mul(1000)) and preserve the u16::MAX bypass check; reference the
same symbols top_up_lamports_from_account_info_unchecked, transfer_amount, and
max_top_up so the prose mirrors the implementation exactly.
| /// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed) | ||
| pub max_top_up: Option<u16>, |
There was a problem hiding this comment.
Doc for BurnCheckedCpi.max_top_up is missing the second line present on BurnChecked.max_top_up.
BurnChecked (line 38) has the additional doc line "When set (Some), includes max_top_up in instruction data", but BurnCheckedCpi omits it. Since BurnCheckedCpi delegates to BurnChecked.instruction(), the same serialization behavior applies — keeping the docs consistent helps callers understand both entry points equally.
Proposed fix
/// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed)
+ /// When set (Some), includes max_top_up in instruction data
pub max_top_up: Option<u16>,🤖 Prompt for AI Agents
In `@sdk-libs/token-sdk/src/instruction/burn_checked.rs` around lines 72 - 73, The
doc for BurnCheckedCpi.max_top_up is missing the second sentence present on
BurnChecked.max_top_up; update the doc comment on the field
BurnCheckedCpi::max_top_up to match BurnChecked::max_top_up by adding the line
"When set (Some), includes max_top_up in instruction data" so callers understand
the same serialization behavior (BurnCheckedCpi delegates to
BurnChecked::instruction()).
| /// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed) | ||
| pub max_top_up: Option<u16>, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor doc asymmetry with the non-CPI struct.
TransferChecked (line 35–36) has a second doc line — /// When set (Some), includes max_top_up in instruction data — that clarifies the serialization behaviour. TransferCheckedCpi here omits it. Since CPI users also need to understand the 9-byte vs 11-byte format toggle, consider mirroring the second line for consistency.
Suggested addition
/// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed)
+ /// When set (Some), includes max_top_up in instruction data (11-byte format)
pub max_top_up: Option<u16>,📝 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.
| /// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed) | |
| pub max_top_up: Option<u16>, | |
| /// Maximum lamports for rent and top-up combined. Transaction fails if exceeded. (u16::MAX = no limit, 0 = no top-ups allowed) | |
| /// When set (Some), includes max_top_up in instruction data (11-byte format) | |
| pub max_top_up: Option<u16>, |
🤖 Prompt for AI Agents
In `@sdk-libs/token-sdk/src/instruction/transfer_checked.rs` around lines 73 - 74,
The docs for TransferCheckedCpi are missing the explanatory second line present
on TransferChecked about how setting max_top_up changes the serialized
instruction length; update the TransferCheckedCpi struct documentation to mirror
TransferChecked by adding a second doc-line like "When set (Some), includes
max_top_up in instruction data" (referencing the TransferCheckedCpi type and its
max_top_up field) so CPI users understand the 9-byte vs 11-byte format toggle.
PR #2279 fixed mint-to but missed 7 other instruction builders that hardcoded maxTopUp: 0 (no top-ups allowed). The on-chain program rejects any rent top-up with MaxTopUpExceeded (0x467b) when maxTopUp is 0. Set to 65535 (u16::MAX = no limit) to match the Rust SDK default. Files changed: - wrap.ts, unwrap.ts, create-decompress-interface-instruction.ts - create-mint.ts, decompress-mint.ts, mint-to-compressed.ts - update-mint.ts, update-metadata.ts
) PR #2279 fixed mint-to but missed 7 other instruction builders that hardcoded maxTopUp: 0 (no top-ups allowed). The on-chain program rejects any rent top-up with MaxTopUpExceeded (0x467b) when maxTopUp is 0. Set to 65535 (u16::MAX = no limit) to match the Rust SDK default. Files changed: - wrap.ts, unwrap.ts, create-decompress-interface-instruction.ts - create-mint.ts, decompress-mint.ts, mint-to-compressed.ts - update-mint.ts, update-metadata.ts Co-authored-by: tilo-14 <tilo@luminouslabs.com>
2 participants
Summary by CodeRabbit
Documentation
Bug Fixes