chore: check compress only is applied correctly#2238
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:
📝 WalkthroughWalkthroughAdds a new Anchor error variant and tightens compress-and-close validation: determine when a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Program as compress_and_close program
participant TokenLib as token-interface helpers
participant Account as CompressedTokenAccount
Client->>Program: invoke compress_and_close
Program->>Account: read account & compression ext
Program->>TokenLib: call compression.requires_compressed_only_ext()
Program->>TokenLib: call ZTokenMut.has_state_requiring_compressed_only()
TokenLib-->>Program: returns needs_compressed_only_ext (bool)
Program->>Account: check compression_only ext presence
alt required but missing
Program-->>Client: error CompressAndCloseMissingCompressedOnlyExtension
else present but not required
Program-->>Client: error CompressAndCloseUnexpectedCompressedOnlyExtension
else ok
Program->>Program: validate compressed-only ext fields (delegate, amount, withheld, frozen, is_ata)
Program-->>Client: success / proceed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
| if (compression.compression_only() || compression.is_ata()) && compression_only_ext.is_none() { | ||
| return Err(ErrorCode::CompressAndCloseMissingCompressedOnlyExtension.into()); | ||
| } | ||
| if !compression.compression_only() && !compression.is_ata() && compression_only_ext.is_some() { |
There was a problem hiding this comment.
We'll also need the extension for a frozen account or when a delegate exists
7f5bc08 to
64efc53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@programs/compressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs`:
- Around line 157-167: The check for the four "marker" extension types is
duplicated and should be extracted: add a helper method (e.g.,
ZTokenMut::has_marker_extensions(&self) -> bool) or a standalone utility
function (e.g., has_marker_extensions(exts: &Option<Vec<ZExtensionStructMut>>)
-> bool) that encapsulates the matches against
ZExtensionStructMut::PausableAccount, PermanentDelegateAccount,
TransferHookAccount, and TransferFeeAccount; then replace the inline logic that
sets has_marker_extensions in compress_and_close.rs with a call to that helper
(referencing has_marker_extensions, ZTokenMut, and ZExtensionStructMut to locate
the code).
...ressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs
Outdated
Show resolved
Hide resolved
...ressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs
Show resolved
Hide resolved
64efc53 to
470865e
Compare
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 (1)
program-libs/token-interface/src/state/extensions/extension_struct.rs (1)
4-16:⚠️ Potential issue | 🟡 MinorFix the lint-reported formatting issue in the imports.
The pipeline flags a formatting diff on these import lines. Run
cargo fmtto resolve.
🤖 Fix all issues with AI agents
In `@program-libs/token-interface/src/state/extensions/extension_type.rs`:
- Around line 70-91: Add a compile-time sanity check to ensure
MARKER_EXTENSION_TYPES and is_marker_extension stay in sync by asserting the
array length equals the number of variants handled; for example, add a const
assertion like `const _: [(); MARKER_EXTENSION_TYPES.len()] = [(); 4];` adjacent
to the `MARKER_EXTENSION_TYPES`/`is_marker_extension` definitions (or use a
`const_assert!` if your project provides one) so any future addition forces
updating both places.
In
`@programs/compressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs`:
- Around line 154-163: The if condition starting with "if
!compression.compression_only() && !compression.is_ata() &&
compression_only_ext.is_some()" exceeds the rustfmt 100-char limit; reformat it
into multiple lines (e.g., break after each &&) so the same logic is preserved
and rustfmt passes, keeping the inner check using ctoken.is_frozen(),
ctoken.delegate().is_none(), ctoken.has_marker_extensions(), and returning
ErrorCode::CompressAndCloseUnexpectedCompressedOnlyExtension when appropriate.
program-libs/token-interface/src/state/extensions/extension_type.rs
Outdated
Show resolved
Hide resolved
...ressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@program-libs/token-interface/src/state/extensions/extension_struct.rs`:
- Around line 63-83: The match in ExtensionStruct::extension_type() currently
uses a catch-all arm returning ExtensionType::Placeholder0 which can silently
hide future real variants; update extension_type() to explicitly match each
Placeholder variant (e.g., ExtensionStruct::Placeholder0, Placeholder1, etc.)
instead of `_` so the compiler forces updates when new variants are added, or
replace the `_ => ExtensionType::Placeholder0` with a branch that
asserts/unreachable for non-placeholder discriminants (using debug_assert! or
unreachable!) and only return Placeholder0 for the explicit Placeholder0
variant; ensure is_marker_extension() remains calling extension_type().
- Around line 133-156: The catch-all `_ => ExtensionType::Placeholder0` in
ZExtensionStructMut::extension_type() silently maps any future/new variant to
Placeholder0; replace the wildcard with explicit match arms for every
ZExtensionStructMut variant (mapping the Placeholder0 variant to
ExtensionType::Placeholder0) and add a final unreachable panic (e.g.,
panic!("unhandled ZExtensionStructMut variant: {:?}", self)) or use
unreachable!() so any new variant fails loudly instead of silently mapping to
Placeholder0; update the match in ZExtensionStructMut::extension_type()
accordingly.
In
`@programs/compressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs`:
- Around line 154-163: The code's validation is correct but the file isn't
formatted per rustfmt; run cargo fmt (or rustfmt) to reformat the file
containing compress_and_close.rs so the guard block and surrounding whitespace
match the project style, then commit the formatting changes; the relevant
symbols to locate the code are compression.compression_only(),
compression.is_ata(), compression_only_ext, ctoken.is_frozen(),
ctoken.delegate(), ctoken.has_marker_extensions(), and
ErrorCode::CompressAndCloseUnexpectedCompressedOnlyExtension.
program-libs/token-interface/src/state/extensions/extension_struct.rs
Outdated
Show resolved
Hide resolved
program-libs/token-interface/src/state/extensions/extension_struct.rs
Outdated
Show resolved
Hide resolved
...ressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs
Outdated
Show resolved
Hide resolved
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)
programs/compressed-token/program/src/compressed_token/transfer2/compression/ctoken/compress_and_close.rs (1)
163-171: 🧹 Nitpick | 🔵 TrivialStep 6 comment at line 165 could be slightly more precise.
The comment says "ctoken state (frozen, delegate, markers) is already validated in step 5" — but step 5 validates that the extension presence matches the requirement. The actual implication is that reaching this code path means
needs_compressed_only_extwas false, which transitively means the ctoken is not frozen, has no delegate, and has no marker extensions. Might be worth clarifying that nuance for future readers.📝 Suggested comment wording
let Some(ext) = compression_only_ext else { - // ctoken state (frozen, delegate, markers) is already validated in step 5. - // Here we only check that the output doesn't specify a delegate. + // Reaching here means needs_compressed_only_ext was false, i.e., the ctoken + // is not frozen, has no delegate, and has no marker extensions (step 5). + // We only need to verify the output doesn't fabricate a delegate. if compressed_token_account.has_delegate() {
8561b28 to
42cbcef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@programs/compressed-token/anchor/src/lib.rs`:
- Around line 583-586: Update the diagnostic string for the enum variant
CompressAndCloseUnexpectedCompressedOnlyExtension to more accurately reflect the
validation logic: replace the current message about "non-compression_only
accounts" with a broader developer-facing message such as "CompressAndClose:
CompressedOnly extension present but not required by account state" (or similar)
so it covers ATA accounts, frozen/delegated accounts, and accounts with marker
extensions while keeping this as a debug-only message.
42cbcef to
8740c68
Compare
8740c68 to
5b050f2
Compare
Summary by CodeRabbit
Bug Fixes
New Features