refactor: sdk macros 2 #2175
Merged
refactor: sdk macros 2 #2175
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive documentation for the light-sdk-macros crate and refactors its internal architecture. Nine new documentation files explain the macro system's design, while code changes restructure the rentfree derive infrastructure using a builder pattern, modularize accounts/traits/program generation logic, update type signatures for better precision, and remove legacy test files. The public API surface is maintained while reorganizing internals for improved maintainability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: This refactoring is substantial and interconnected, affecting how the macro system handles type preservation, infrastructure field classification, and code generation across accounts/program/traits. Type signature changes propagate across multiple modules (Vec Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
SwenSchaeferjohann
approved these changes
Jan 17, 2026
| println!("All accounts compressed successfully!"); | ||
|
|
||
| // ========================================================================== | ||
| // PHASE 9: Decompress accounts |
Contributor
There was a problem hiding this comment.
assuming we cover decompression in a separate pr?
Contributor
Author
There was a problem hiding this comment.
let me add it actually
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@sdk-libs/macros/CLAUDE.md`:
- Around line 32-41: The fenced code block showing the project tree (the block
starting with "```" then "src/ ├── lib.rs ...") lacks a language specifier;
update the opening fence to include a language (e.g., change the opening "```"
to "```text" or "```bash") so the block is properly marked as plain text—edit
the CLAUDE.md fenced block that contains the src/ tree and only modify the
opening triple-backtick to include the chosen language specifier.
- Around line 81-85: Update the Requirements section to briefly explain the
consequences of missing constants: state that if LIGHT_CPI_SIGNER or ID are not
defined the macro expansion will fail and Rust will produce a compile-time error
(e.g., unresolved constant or missing `declare_id!` symbol), and suggest the
user define `const LIGHT_CPI_SIGNER: Pubkey = ...;` and use `declare_id!` for
`ID` as the remedy; reference the symbols LIGHT_CPI_SIGNER and ID so readers
know exactly which items will trigger the compile-time failure.
In `@sdk-libs/macros/docs/rentfree_program/codegen.md`:
- Around line 7-29: Update the fenced code block in codegen.md that displays the
directory tree to include a language specifier by changing the opening triple
backticks to include "text" (i.e., replace the current ``` with ```text) so the
directory listing is consistently marked as plain text; the target is the
directory tree block beginning with "sdk-libs/macros/src/rentfree/program/" in
the codegen.md file.
- Around line 50-54: Update the line number references in the documentation so
they match the current source: change the reference for rentfree_program_impl()
from line 405 to 406 and change the reference for codegen() from line 38 to 40
in the docs; locate the entries referencing rentfree_program_impl() and
codegen() and update the numeric line annotations accordingly, and optionally
replace fragile line-number comments with stable symbol-based references or
anchors for future-proofing.
In `@sdk-libs/macros/docs/traits/compressible_pack.md`:
- Around line 213-229: Update the documentation around the impl Unpack for
PackedUserRecord to explicitly note that unpack uses direct indexing into
remaining_accounts (remaining_accounts[self.owner as usize] and
remaining_accounts[self.authority as usize]) and therefore will panic on
out-of-bounds indices; state that this relies on pack/unpack symmetry and valid
packed data, and advise that corrupted or tampered packed data can cause a
runtime panic so callers should validate indices or ensure data provenance
before calling unpack.
In `@sdk-libs/macros/docs/traits/compressible.md`:
- Around line 17-32: The markdown has untyped fenced code blocks containing
ASCII diagrams which trigger markdownlint MD040; update each triple-backtick
fence used for diagrams (e.g., the blocks showing the "Input Struct ->
Compressible Macro -> 4 Trait Impls" ASCII diagram and the other diagram blocks
at lines 36-68 and 74-107) to include a language tag such as text (change ``` to
```text) so the fences are typed and the linter passes while leaving the diagram
content unchanged.
In `@sdk-libs/macros/src/rentfree/accounts/builder.rs`:
- Around line 108-115: Replace the panic-prone
`.as_ref().unwrap().first().unwrap()` chain used to set `params_ident` with a
helper method (e.g., `first_instruction_arg(&self) -> Result<&InstructionArg,
syn::Error>`) that returns the first `InstructionArg` or a `syn::Error`; have
the helper use `self.parsed.instruction_args.as_ref().and_then(|a|
a.first()).ok_or_else(|| syn::Error::new_spanned(&self.parsed.struct_name,
"Missing #[instruction(...)] attribute"))`, and then update the code that
assigns `params_ident` to call this helper and propagate or handle the `Result`
so compilation emits a clear compile-time error instead of panicking.
- Around line 101-131: The code indexes self.parsed.light_mint_fields[0] in
generate_pre_init_pdas_and_mints (and similarly in generate_pre_init_mints_only)
without checking for emptiness, which can panic; change the functions to return
Result<TokenStream, syn::Error> and guard the access by checking
self.parsed.light_mint_fields.is_empty() (or call has_mints()) and return a
syn::Error::new_spanned(...) describing the missing #[light_mint] when empty;
then use safe access like self.parsed.light_mint_fields.first().unwrap() (after
the guard) when constructing the LightMintBuilder invocation and update any
callers to handle the Result.
In `@sdk-libs/macros/src/rentfree/accounts/parse.rs`:
- Around line 67-79: InfraFields::set currently overwrites existing entries
silently; change it to detect duplicates by checking the target option (e.g.,
self.fee_payer, self.payer equivalents) before assignment and either emit a
compile-time warning/error or document "last wins" behavior; specifically, in
InfraFields::set match on InfraFieldType to get a mutable reference to the
target Option, if target.is_some() then call your chosen diagnostics (e.g., emit
a warning via proc_macro_error::emit_warning! or return/emit a compile_error!
with Ident span) and only then set *target = Some(ident), so duplicate field
matches are surfaced instead of silently overridden.
In `@sdk-libs/macros/src/rentfree/program/instructions.rs`:
- Around line 459-471: The loop that deduplicates PDA specs by variant_name
(iterating pda_specs and using seen_variants) currently silently skips
duplicates; change it to detect configuration conflicts by storing the
first-seen configuration (e.g., inner_type and seed spec) keyed by variant_name
(instead of only seen_variants) and on encountering a duplicate compare the new
pda.inner_type and seed details against the stored values; if they differ,
return or panic with a clear error mentioning the variant_name (or emit a
warning/log with process context) otherwise continue deduplicating as before
(update references to found_pda_seeds, found_data_fields, and the
duplicate-check logic to use this map-based conflict detection).
In `@sdk-libs/macros/src/rentfree/program/parsing.rs`:
- Around line 411-431: The pre-init boolean returned by light_pre_init is being
discarded; preserve it (e.g., assign to a variable like __has_pre_init) so
downstream logic can know whether pre-init ran, and then re-enable the finalize
path to call ctx.accounts.light_finalize(ctx.remaining_accounts, ¶ms,
__has_pre_init) after the handler only when the handler result
(__light_handler_result or equivalent) is Ok; to avoid “moved ctx” issues,
capture/cloned the AccountInfo references from ctx.remaining_accounts (or
otherwise extract the minimal data needed) into a local structure before
executing `#fn_block` so ctx can be safely used for light_finalize, and ensure you
reference params_ident consistently when passing params to light_finalize.
In `@sdk-libs/macros/src/rentfree/program/variant_enum.rs`:
- Around line 99-106: Precompute and reuse the qualified inner type and packed
inner type inside the pda_ctx_seeds iterator instead of calling
qualify_type_with_crate(&info.inner_type) multiple times: inside the closure
that maps pda_ctx_seeds, bind let inner_type =
qualify_type_with_crate(&info.inner_type) and let packed_inner_type =
make_packed_type(&info.inner_type) (or construct Packed{Variant} type once),
then use inner_type and packed_inner_type in all generated match arms (e.g., in
the RentFreeAccountVariant::`#variant_name` and
RentFreeAccountVariant::`#packed_variant_name` arms) to avoid repeated
qualification calls.
In `@sdk-libs/macros/src/rentfree/shared_utils.rs`:
- Around line 22-41: The function qualify_type_with_crate currently prepends
crate:: to any non-absolute Type::Path, which corrupts external multi-segment
paths like other_crate::Type; modify qualify_type_with_crate to only prepend the
crate:: segment when the path is a single-segment (e.g., segments.len() == 1)
and not already qualified (keep the existing checks for leading_colon and first
segment being crate/super/self), leaving multi-segment paths untouched; locate
the logic in qualify_type_with_crate operating on Type::Path and add the
segments length check before inserting the crate segment.
In `@sdk-libs/macros/src/rentfree/traits/seed_extraction.rs`:
- Around line 321-331: The current heuristic in the loop over args.args checks
inner_seg.ident != "info" to skip the `'info` lifetime (in the block referencing
inner_ty, inner_path, inner_seg), which is fragile; change the loop to
explicitly skip GenericArgument::Lifetime by checking matches!(arg,
syn::GenericArgument::Lifetime(_)) (or equivalent) before handling
GenericArgument::Type, and then continue to the existing Type::Path / inner_seg
logic to return the type (preserving the existing return of Some((false,
inner_ty.clone()))) so lifetimes are reliably ignored instead of relying on the
"info" identifier check.
Comment on lines
+32
to
+41
| ``` | ||
| src/ | ||
| ├── lib.rs # Macro entry points | ||
| ├── rentfree/ # RentFree macro system | ||
| │ ├── accounts/ # #[derive(RentFree)] for Accounts structs | ||
| │ ├── program/ # #[rentfree_program] attribute macro | ||
| │ ├── traits/ # Trait derive macros | ||
| │ └── shared_utils.rs # Common utilities | ||
| └── hasher/ # LightHasherSha derive macro | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier to source structure block.
Per the static analysis hint, the fenced code block should have a language specified.
Suggested fix
-```
+```text
src/
├── lib.rs # Macro entry points🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@sdk-libs/macros/CLAUDE.md` around lines 32 - 41, The fenced code block
showing the project tree (the block starting with "```" then "src/ ├── lib.rs
...") lacks a language specifier; update the opening fence to include a language
(e.g., change the opening "```" to "```text" or "```bash") so the block is
properly marked as plain text—edit the CLAUDE.md fenced block that contains the
src/ tree and only modify the opening triple-backtick to include the chosen
language specifier.
Comment on lines
+81
to
+85
| ## Requirements | ||
|
|
||
| Programs using these macros must define: | ||
| - `LIGHT_CPI_SIGNER: Pubkey` - CPI signer constant | ||
| - `ID` - Program ID (from `declare_id!`) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding context for requirements.
The requirements section lists LIGHT_CPI_SIGNER and ID but doesn't explain what happens if they're missing. Consider adding a brief note about the compile-time error users will see, which would help with debugging.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/CLAUDE.md` around lines 81 - 85, Update the Requirements
section to briefly explain the consequences of missing constants: state that if
LIGHT_CPI_SIGNER or ID are not defined the macro expansion will fail and Rust
will produce a compile-time error (e.g., unresolved constant or missing
`declare_id!` symbol), and suggest the user define `const LIGHT_CPI_SIGNER:
Pubkey = ...;` and use `declare_id!` for `ID` as the remedy; reference the
symbols LIGHT_CPI_SIGNER and ID so readers know exactly which items will trigger
the compile-time failure.
Comment on lines
+7
to
+29
| ``` | ||
| sdk-libs/macros/src/rentfree/program/ | ||
| |-- mod.rs # Module exports, main entry point rentfree_program_impl | ||
| |-- instructions.rs # Main orchestration: codegen(), rentfree_program_impl() | ||
| |-- parsing.rs # Core types (TokenSeedSpec, SeedElement, InstructionDataSpec) | ||
| | # Expression analysis, seed conversion, function wrapping | ||
| |-- compress.rs # CompressAccountsIdempotent generation | ||
| | # CompressContext trait impl, compress processor | ||
| |-- decompress.rs # DecompressAccountsIdempotent generation | ||
| | # DecompressContext trait impl, PDA seed provider impls | ||
| |-- variant_enum.rs # RentFreeAccountVariant enum generation | ||
| | # TokenAccountVariant/PackedTokenAccountVariant generation | ||
| | # Pack/Unpack trait implementations | ||
| |-- seed_codegen.rs # Client seed function generation | ||
| | # TokenSeedProvider implementation generation | ||
| |-- crate_context.rs # Anchor-style crate parsing (CrateContext, ParsedModule) | ||
| | # Module file discovery and parsing | ||
| |-- expr_traversal.rs # AST expression transformation (ctx.field -> ctx_seeds.field) | ||
| |-- seed_utils.rs # Seed expression conversion utilities | ||
| | # SeedConversionConfig, seed_element_to_ref_expr() | ||
| |-- visitors.rs # Visitor-based AST traversal (FieldExtractor) | ||
| | # ClientSeedInfo classification and code generation | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifier for directory tree code block.
For consistency with markdown lint rules, consider adding a language specifier. Since this is a directory structure, you can use text or leave blank with an explicit marker.
Suggested fix
-```
+```text
sdk-libs/macros/src/rentfree/program/🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@sdk-libs/macros/docs/rentfree_program/codegen.md` around lines 7 - 29, Update
the fenced code block in codegen.md that displays the directory tree to include
a language specifier by changing the opening triple backticks to include "text"
(i.e., replace the current ``` with ```text) so the directory listing is
consistently marked as plain text; the target is the directory tree block
beginning with "sdk-libs/macros/src/rentfree/program/" in the codegen.md file.
Comment on lines
+50
to
+54
| +-----------------------------+ | ||
| | rentfree_program_impl() | | ||
| | (instructions.rs:405) | | ||
| +-----------------------------+ | ||
| | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the referenced functions exist at the documented locations
echo "=== Searching for rentfree_program_impl() ==="
rg -n "pub fn rentfree_program_impl" sdk-libs/macros/src/rentfree/program/instructions.rs
echo -e "\n=== Searching for codegen() function ==="
rg -n "^pub fn codegen\b|^fn codegen\b" sdk-libs/macros/src/rentfree/program/instructions.rs
echo -e "\n=== File structure overview ==="
wc -l sdk-libs/macros/src/rentfree/program/instructions.rsRepository: Lightprotocol/light-protocol
Length of output: 363
Update line number references in documentation to match current code.
The referenced functions exist in the correct file but at slightly different line numbers:
rentfree_program_impl()is at line 406 (not 405)codegen()is at line 40 (not 38)
Update these references to keep documentation accurate as the codebase evolves. For future-proofing, consider whether symbol references or comments in the source code would serve better than line numbers.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/docs/rentfree_program/codegen.md` around lines 50 - 54,
Update the line number references in the documentation so they match the current
source: change the reference for rentfree_program_impl() from line 405 to 406
and change the reference for codegen() from line 38 to 40 in the docs; locate
the entries referencing rentfree_program_impl() and codegen() and update the
numeric line annotations accordingly, and optionally replace fragile line-number
comments with stable symbol-based references or anchors for future-proofing.
Comment on lines
+213
to
+229
| // Unpack packed -> original | ||
| impl light_sdk::compressible::Unpack for PackedUserRecord { | ||
| type Unpacked = UserRecord; | ||
|
|
||
| #[inline(never)] | ||
| fn unpack( | ||
| &self, | ||
| remaining_accounts: &[anchor_lang::prelude::AccountInfo], | ||
| ) -> std::result::Result<Self::Unpacked, anchor_lang::prelude::ProgramError> { | ||
| Ok(UserRecord { | ||
| owner: *remaining_accounts[self.owner as usize].key, | ||
| authority: *remaining_accounts[self.authority as usize].key, | ||
| score: self.score, | ||
| compression_info: None, | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document potential panic on invalid index.
The unpack implementation at lines 223-224 uses direct array indexing (remaining_accounts[self.owner as usize]) which will panic if the index is out of bounds. While this is correct behavior (invalid packed data shouldn't reach here), consider adding a note in the documentation that this assumes valid packed data and will panic otherwise.
This is a good teaching moment for users - the pack/unpack symmetry means indices should always be valid if the data was packed correctly, but corrupted data would cause a panic.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/docs/traits/compressible_pack.md` around lines 213 - 229,
Update the documentation around the impl Unpack for PackedUserRecord to
explicitly note that unpack uses direct indexing into remaining_accounts
(remaining_accounts[self.owner as usize] and remaining_accounts[self.authority
as usize]) and therefore will panic on out-of-bounds indices; state that this
relies on pack/unpack symmetry and valid packed data, and advise that corrupted
or tampered packed data can cause a runtime panic so callers should validate
indices or ensure data provenance before calling unpack.
Comment on lines
+459
to
+471
| // Deduplicate based on variant_name (field name) - field names must be globally unique | ||
| let mut found_pda_seeds: Vec<TokenSeedSpec> = Vec::new(); | ||
| let mut found_data_fields: Vec<InstructionDataSpec> = Vec::new(); | ||
| let mut account_types: Vec<Ident> = Vec::new(); | ||
| let mut account_types: Vec<Type> = Vec::new(); | ||
| let mut seen_variants: std::collections::HashSet<String> = std::collections::HashSet::new(); | ||
|
|
||
| for pda in &pda_specs { | ||
| // Deduplicate based on variant_name (derived from field name) | ||
| // If same field name is used in multiple instruction structs, only add once | ||
| let variant_str = pda.variant_name.to_string(); | ||
| if !seen_variants.insert(variant_str) { | ||
| continue; // Skip duplicate field names | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent deduplication could mask configuration conflicts.
The deduplication correctly prevents duplicate variants, but it silently drops subsequent occurrences. If a developer accidentally defines the same field name in multiple instruction structs with different inner_type or seed configurations, only the first one wins—and there's no warning.
Consider either:
- Erroring if a duplicate has a different
inner_typeor seeds (configuration conflict) - Adding a debug/warning message when skipping duplicates
This would help catch subtle misconfigurations early.
🔧 Suggested enhancement for conflict detection
for pda in &pda_specs {
// Deduplicate based on variant_name (derived from field name)
// If same field name is used in multiple instruction structs, only add once
let variant_str = pda.variant_name.to_string();
- if !seen_variants.insert(variant_str) {
- continue; // Skip duplicate field names
+ if seen_variants.contains(&variant_str) {
+ // Could optionally verify that the duplicate has the same inner_type
+ // to catch configuration mismatches
+ continue; // Skip duplicate field names
}
+ seen_variants.insert(variant_str);
account_types.push(pda.inner_type.clone());Or for stricter enforcement, store the inner_type in a map and compare on duplicate detection.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/rentfree/program/instructions.rs` around lines 459 - 471,
The loop that deduplicates PDA specs by variant_name (iterating pda_specs and
using seen_variants) currently silently skips duplicates; change it to detect
configuration conflicts by storing the first-seen configuration (e.g.,
inner_type and seed spec) keyed by variant_name (instead of only seen_variants)
and on encountering a duplicate compare the new pda.inner_type and seed details
against the stored values; if they differ, return or panic with a clear error
mentioning the variant_name (or emit a warning/log with process context)
otherwise continue deduplicating as before (update references to
found_pda_seeds, found_data_fields, and the duplicate-check logic to use this
map-based conflict detection).
Comment on lines
411
to
431
| // Phase 1: Pre-init (creates mints via CPI context write, registers compressed addresses) | ||
| use light_sdk::compressible::{LightPreInit, LightFinalize}; | ||
| let __has_pre_init = ctx.accounts.light_pre_init(ctx.remaining_accounts, &#params_ident) | ||
| let _ = ctx.accounts.light_pre_init(ctx.remaining_accounts, &#params_ident) | ||
| .map_err(|e| { | ||
| let pe: solana_program_error::ProgramError = e.into(); | ||
| pe | ||
| })?; | ||
|
|
||
| // Execute the original handler body in a closure | ||
| let __light_handler_result = (|| #fn_block)(); | ||
|
|
||
| // Phase 2: On success, finalize compression | ||
| if __light_handler_result.is_ok() { | ||
| ctx.accounts.light_finalize(ctx.remaining_accounts, &#params_ident, __has_pre_init) | ||
| .map_err(|e| { | ||
| let pe: solana_program_error::ProgramError = e.into(); | ||
| pe | ||
| })?; | ||
| } | ||
|
|
||
| __light_handler_result | ||
| // Execute the original handler body | ||
| #fn_block | ||
|
|
||
| // TODO(diff-pr): Reactivate light_finalize for top up transfers. | ||
| // Currently disabled because user code may move ctx, making it | ||
| // inaccessible after the handler body executes. When top up | ||
| // transfers are implemented, we'll need to store AccountInfo | ||
| // references before user code runs. | ||
| // | ||
| // if __light_handler_result.is_ok() { | ||
| // ctx.accounts.light_finalize(ctx.remaining_accounts, ¶ms, __has_pre_init)?; | ||
| // } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pre-init result is discarded; finalize logic is disabled with TODO.
A few observations on this wrapped function:
-
Discarded pre-init result (line 413): The
let _ = ...discards theboolreturned bylight_pre_init, which indicates whether pre-initialization actually occurred. If downstream logic ever needs to know this, it won't have access to it. -
Disabled finalize (lines 422-430): The TODO explains that
light_finalizeis disabled because user code may movectx, making it inaccessible. This is a valid concern, but it leaves the lifecycle incomplete.
The TODO references "diff-pr" for reactivation—consider creating a tracking issue so this doesn't get lost. When top-up transfers are implemented, you'll need to capture AccountInfo references before the handler body executes.
Would you like me to open an issue to track the light_finalize reactivation work mentioned in the TODO?
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/rentfree/program/parsing.rs` around lines 411 - 431, The
pre-init boolean returned by light_pre_init is being discarded; preserve it
(e.g., assign to a variable like __has_pre_init) so downstream logic can know
whether pre-init ran, and then re-enable the finalize path to call
ctx.accounts.light_finalize(ctx.remaining_accounts, ¶ms, __has_pre_init)
after the handler only when the handler result (__light_handler_result or
equivalent) is Ok; to avoid “moved ctx” issues, capture/cloned the AccountInfo
references from ctx.remaining_accounts (or otherwise extract the minimal data
needed) into a local structure before executing `#fn_block` so ctx can be safely
used for light_finalize, and ensure you reference params_ident consistently when
passing params to light_finalize.
Comment on lines
+99
to
106
| let hash_match_arms = pda_ctx_seeds.iter().map(|info| { | ||
| let variant_name = &info.variant_name; | ||
| let inner_type = qualify_type_with_crate(&info.inner_type); | ||
| let packed_variant_name = format_ident!("Packed{}", variant_name); | ||
| quote! { | ||
| RentFreeAccountVariant::#name { data, .. } => <#name as light_hasher::DataHasher>::hash::<H>(data), | ||
| RentFreeAccountVariant::#packed_name { .. } => unreachable!(), | ||
| RentFreeAccountVariant::#variant_name { data, .. } => <#inner_type as light_hasher::DataHasher>::hash::<H>(data), | ||
| RentFreeAccountVariant::#packed_variant_name { .. } => unreachable!(), | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider qualifying inner_type once per iteration.
qualify_type_with_crate(&info.inner_type) is called multiple times across different match arm generators (lines 101, 130, 140, 150, 160, 206, 229). While the function is idempotent, pre-qualifying once would be cleaner:
let account_variants = pda_ctx_seeds.iter().map(|info| {
let variant_name = &info.variant_name;
let inner_type = qualify_type_with_crate(&info.inner_type);
let packed_inner_type = make_packed_type(&info.inner_type)...;
// ... use inner_type and packed_inner_type throughout
});This is a minor efficiency improvement—not blocking.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/rentfree/program/variant_enum.rs` around lines 99 - 106,
Precompute and reuse the qualified inner type and packed inner type inside the
pda_ctx_seeds iterator instead of calling
qualify_type_with_crate(&info.inner_type) multiple times: inside the closure
that maps pda_ctx_seeds, bind let inner_type =
qualify_type_with_crate(&info.inner_type) and let packed_inner_type =
make_packed_type(&info.inner_type) (or construct Packed{Variant} type once),
then use inner_type and packed_inner_type in all generated match arms (e.g., in
the RentFreeAccountVariant::`#variant_name` and
RentFreeAccountVariant::`#packed_variant_name` arms) to avoid repeated
qualification calls.
Comment on lines
+22
to
+41
| pub fn qualify_type_with_crate(ty: &Type) -> Type { | ||
| if let Type::Path(type_path) = ty { | ||
| // Check if already qualified | ||
| if let Some(first_seg) = type_path.path.segments.first() { | ||
| let first_str = first_seg.ident.to_string(); | ||
| // Already qualified with crate, super, self, or starts with :: | ||
| if first_str == "crate" || first_str == "super" || first_str == "self" { | ||
| return ty.clone(); | ||
| } | ||
| } | ||
| // Check for absolute path (starts with ::) | ||
| if type_path.path.leading_colon.is_some() { | ||
| return ty.clone(); | ||
| } | ||
|
|
||
| // Prepend crate:: to the path | ||
| let mut qualified_path = type_path.clone(); | ||
| let crate_segment: syn::PathSegment = syn::parse_quote!(crate); | ||
| qualified_path.path.segments.insert(0, crate_segment); | ||
| Type::Path(qualified_path) |
Contributor
There was a problem hiding this comment.
Avoid forcing crate:: on already-qualified external paths.
Currently any non-absolute path gets crate:: prepended, which rewrites other_crate::Type into crate::other_crate::Type and breaks external type usage. Consider only qualifying single-segment paths so explicit multi-segment paths remain intact.
🔧 Proposed fix
pub fn qualify_type_with_crate(ty: &Type) -> Type {
if let Type::Path(type_path) = ty {
@@
- // Prepend crate:: to the path
- let mut qualified_path = type_path.clone();
- let crate_segment: syn::PathSegment = syn::parse_quote!(crate);
- qualified_path.path.segments.insert(0, crate_segment);
- Type::Path(qualified_path)
+ // Only qualify bare idents; keep explicit module paths as written.
+ if type_path.path.segments.len() == 1 {
+ let mut qualified_path = type_path.clone();
+ let crate_segment: syn::PathSegment = syn::parse_quote!(crate);
+ qualified_path.path.segments.insert(0, crate_segment);
+ Type::Path(qualified_path)
+ } else {
+ ty.clone()
+ }
} else {
ty.clone()
}
}🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/rentfree/shared_utils.rs` around lines 22 - 41, The
function qualify_type_with_crate currently prepends crate:: to any non-absolute
Type::Path, which corrupts external multi-segment paths like other_crate::Type;
modify qualify_type_with_crate to only prepend the crate:: segment when the path
is a single-segment (e.g., segments.len() == 1) and not already qualified (keep
the existing checks for leading_colon and first segment being crate/super/self),
leaving multi-segment paths untouched; locate the logic in
qualify_type_with_crate operating on Type::Path and add the segments length
check before inserting the crate segment.
Comment on lines
+321
to
331
| if let syn::GenericArgument::Type(inner_ty) = arg { | ||
| // Skip lifetime 'info by checking if this is a path type | ||
| if let Type::Path(inner_path) = inner_ty { | ||
| if let Some(inner_seg) = inner_path.path.segments.last() { | ||
| // Skip lifetime 'info TODO: add a helper that is generalized to strip lifetimes or check whether a crate already has this | ||
| if inner_seg.ident != "info" { | ||
| // Return the full type, preserving the path | ||
| return Some((false, inner_ty.clone())); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Heuristic for lifetime detection is fragile.
The check inner_seg.ident != "info" attempts to skip the 'info lifetime, but this works because 'info is parsed as a type path in some contexts. However, this would incorrectly skip a legitimate type named info (unlikely but possible).
A more robust approach would be to explicitly check for GenericArgument::Lifetime first and skip those, rather than relying on the identifier name:
for arg in &args.args {
// Skip lifetimes explicitly
if matches!(arg, syn::GenericArgument::Lifetime(_)) {
continue;
}
if let syn::GenericArgument::Type(inner_ty) = arg {
// ... rest of extraction
}
}The current TODO acknowledges this—consider addressing it in a follow-up.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/rentfree/traits/seed_extraction.rs` around lines 321 -
331, The current heuristic in the loop over args.args checks inner_seg.ident !=
"info" to skip the `'info` lifetime (in the block referencing inner_ty,
inner_path, inner_seg), which is fragile; change the loop to explicitly skip
GenericArgument::Lifetime by checking matches!(arg,
syn::GenericArgument::Lifetime(_)) (or equivalent) before handling
GenericArgument::Type, and then continue to the existing Type::Path / inner_seg
logic to return the type (preserving the existing return of Some((false,
inner_ty.clone()))) so lifetimes are reliably ignored instead of relying on the
"info" identifier check.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.