Merged
Conversation
Contributor
📝 WalkthroughWalkthroughPropagates direct CreateAccountsProof detection and proof-access expressions through Light PDAs macros; instruction-params extraction now returns an explicit Changes
Sequence Diagram(s)(omitted — changes are internal parsing/codegen updates that do not introduce a new multi-component runtime flow suitable for a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
ananas-block
force-pushed
the
jorrit/more-macro-fixes
branch
from
e463bb0 to
9e37eae
Compare
SwenSchaeferjohann
approved these changes
Jan 20, 2026
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/macros/src/light_pdas/accounts/light_account.rs (1)
732-741: Add coverage for the direct proof-arg default path.
The tests only exercisedirect_proof_arg = None. Add at least one test that passesSome(proof_ident)to validate the new defaulting behavior foraddress_tree_info/output_tree.
🤖 Fix all issues with AI agents
In `@sdk-libs/macros/src/light_pdas/accounts/parse.rs`:
- Around line 248-251: Find and validate the direct-proof argument ordering:
after computing direct_proof_arg via find_direct_proof_arg(&instruction_args),
ensure that when direct_proof_arg.is_some() the referenced argument is the first
entry in instruction_args (otherwise get_proof_access will emit a `#proof_ident`
that isn't in the generated light_pre_init signature). If it is not first, emit
a clear compile-time error from parse.rs instructing the caller to make
CreateAccountsProof the first instruction arg (or update the generation logic),
or alternatively adjust the signature-generation code to include that arg;
reference find_direct_proof_arg, get_proof_access and the light_pre_init
signature when adding this validation/error.
In `@sdk-libs/macros/src/light_pdas/program/instructions.rs`:
- Line 536: The `use crate::light_pdas::program::parsing::ExtractResult;` import
is placed inside a for loop; move this `use` statement to the surrounding
function or module scope just above the `for` loop so the import is resolved
once rather than on every iteration, locating the import near the function that
contains the loop (referencing the `ExtractResult` type and the loop that
iterates over program instructions) to keep the code idiomatic and efficient.
In `@sdk-libs/macros/src/light_pdas/program/parsing.rs`:
- Around line 352-367: Add a Debug derive to the ExtractResult enum so macro
debugging can print extraction outcomes; update the enum declaration for
ExtractResult (which defines variants Success, MultipleParams, and None) to
include #[derive(Debug)] immediately above it.
- Around line 407-411: The current heuristic that filters parameter names using
!name.contains("signer") && !name.contains("bump") can false-negative legitimate
params (e.g., signer_count, bump_amount); update the filter in the params
collection logic (the code that pushes into params_candidates using pat_ident
and name) to use stricter checks such as exact matches and/or suffix/prefix
patterns instead of contains (for example check name == "signer" || name ==
"bump" or name.ends_with("_signer") || name.ends_with("_bump")), so only true
signer-like or bump-like identifiers are excluded while keeping other
identifiers like signer_count or bump_amount.
- Around line 749-768: Add a boundary test that verifies
extract_context_and_params returns ExtractResult::Success when there is exactly
one instruction parameter: create a new #[test] similar to the existing
multiple-params test but with a function signature pub fn handler(ctx:
Context<MyAccounts>, amount: u64) -> Result<()> { ... } and assert that
extract_context_and_params(&fn_item) matches ExtractResult::Success { .. };
reference the extract_context_and_params function and the ExtractResult::Success
variant when adding the test so it covers the single-parameter case.
Comment on lines
+248
to
+251
| // Check if CreateAccountsProof is passed as a direct instruction argument | ||
| // (compute this early so we can use it for field parsing defaults) | ||
| let direct_proof_arg = find_direct_proof_arg(&instruction_args); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard against direct-proof arg not being first (compile error risk).
get_proof_access() will emit #proof_ident even if that argument isn’t in the generated light_pre_init signature (only the first arg is passed). That causes an out-of-scope identifier when CreateAccountsProof isn’t the first instruction arg. Please enforce ordering (or adjust the generated signature).
🛠️ Suggested validation
let direct_proof_arg = find_direct_proof_arg(&instruction_args);
+ if let (Some(proof_ident), Some(args)) = (&direct_proof_arg, &instruction_args) {
+ if args.first().map(|a| &a.name) != Some(proof_ident) {
+ return Err(Error::new_spanned(
+ proof_ident,
+ "CreateAccountsProof must be the first #[instruction] argument when passed directly; otherwise wrap it inside params.create_accounts_proof.",
+ ));
+ }
+ }📝 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
| // Check if CreateAccountsProof is passed as a direct instruction argument | |
| // (compute this early so we can use it for field parsing defaults) | |
| let direct_proof_arg = find_direct_proof_arg(&instruction_args); | |
| // Check if CreateAccountsProof is passed as a direct instruction argument | |
| // (compute this early so we can use it for field parsing defaults) | |
| let direct_proof_arg = find_direct_proof_arg(&instruction_args); | |
| if let (Some(proof_ident), Some(args)) = (&direct_proof_arg, &instruction_args) { | |
| if args.first().map(|a| &a.name) != Some(proof_ident) { | |
| return Err(Error::new_spanned( | |
| proof_ident, | |
| "CreateAccountsProof must be the first #[instruction] argument when passed directly; otherwise wrap it inside params.create_accounts_proof.", | |
| )); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/accounts/parse.rs` around lines 248 - 251,
Find and validate the direct-proof argument ordering: after computing
direct_proof_arg via find_direct_proof_arg(&instruction_args), ensure that when
direct_proof_arg.is_some() the referenced argument is the first entry in
instruction_args (otherwise get_proof_access will emit a `#proof_ident` that isn't
in the generated light_pre_init signature). If it is not first, emit a clear
compile-time error from parse.rs instructing the caller to make
CreateAccountsProof the first instruction arg (or update the generation logic),
or alternatively adjust the signature-generation code to include that arg;
reference find_direct_proof_arg, get_proof_access and the light_pre_init
signature when adding this validation/error.
| if rentfree_struct_names.contains(&context_type) { | ||
| // Wrap the function with pre_init/finalize logic | ||
| *fn_item = wrap_function_with_light(fn_item, ¶ms_ident, &ctx_name); | ||
| use crate::light_pdas::program::parsing::ExtractResult; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move the import outside the loop for efficiency.
The use statement is inside the for loop, causing it to be processed on every iteration. While Rust's name resolution handles this correctly, it's cleaner and more idiomatic to place imports at the block scope where they're needed—here, that means before the loop.
♻️ Suggested refactor
// Auto-wrap instruction handlers that use rentfree Accounts structs
if let Some((_, ref mut items)) = module.content {
+ use crate::light_pdas::program::parsing::ExtractResult;
for item in items.iter_mut() {
if let Item::Fn(fn_item) = item {
// Check if this function uses a rentfree Accounts struct
- use crate::light_pdas::program::parsing::ExtractResult;
match extract_context_and_params(fn_item) {🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/program/instructions.rs` at line 536, The `use
crate::light_pdas::program::parsing::ExtractResult;` import is placed inside a
for loop; move this `use` statement to the surrounding function or module scope
just above the `for` loop so the import is resolved once rather than on every
iteration, locating the import near the function that contains the loop
(referencing the `ExtractResult` type and the loop that iterates over program
instructions) to keep the code idiomatic and efficient.
Comment on lines
+352
to
+367
| /// Result from extracting context and params from a function signature. | ||
| pub enum ExtractResult { | ||
| /// Successfully extracted context type, params ident, and context ident | ||
| Success { | ||
| context_type: String, | ||
| params_ident: Ident, | ||
| ctx_ident: Ident, | ||
| }, | ||
| /// Multiple params arguments detected (format-2 case) - caller decides if this is an error | ||
| MultipleParams { | ||
| context_type: String, | ||
| param_names: Vec<String>, | ||
| }, | ||
| /// No valid context/params combination found | ||
| None, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-designed result enum for extraction outcomes.
The three-variant enum cleanly models all possible outcomes:
Successwith full extraction dataMultipleParamsfor detecting format-2 cases that need consolidationNonefor functions that don't match the expected signature
Consider adding #[derive(Debug)] to aid macro debugging, though this is optional for internal code.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/program/parsing.rs` around lines 352 - 367,
Add a Debug derive to the ExtractResult enum so macro debugging can print
extraction outcomes; update the enum declaration for ExtractResult (which
defines variants Success, MultipleParams, and None) to include #[derive(Debug)]
immediately above it.
Comment on lines
407
to
411
| // Track potential params argument (not the context param, not signer-like names) | ||
| let name = pat_ident.ident.to_string(); | ||
| if !name.contains("signer") && !name.contains("bump") { | ||
| // Prefer "params" but accept others | ||
| if name == "params" || params_ident.is_none() { | ||
| params_ident = Some(pat_ident.ident.clone()); | ||
| } | ||
| params_candidates.push(pat_ident.ident.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Filtering heuristic may have false negatives for edge-case parameter names.
The filter !name.contains("signer") && !name.contains("bump") would incorrectly exclude legitimate data parameters like signer_count, bump_amount, or token_signer_info. These substring matches might be overly broad.
Consider using exact matches or prefix/suffix patterns if this causes issues in practice:
// More precise filtering
let is_excluded = name == "signer" || name == "bump"
|| name.ends_with("_signer") || name.ends_with("_bump");That said, for typical Anchor patterns this works fine—flagging for awareness rather than immediate change.
🤖 Prompt for AI Agents
In `@sdk-libs/macros/src/light_pdas/program/parsing.rs` around lines 407 - 411,
The current heuristic that filters parameter names using
!name.contains("signer") && !name.contains("bump") can false-negative legitimate
params (e.g., signer_count, bump_amount); update the filter in the params
collection logic (the code that pushes into params_candidates using pat_ident
and name) to use stricter checks such as exact matches and/or suffix/prefix
patterns instead of contains (for example check name == "signer" || name ==
"bump" or name.ends_with("_signer") || name.ends_with("_bump")), so only true
signer-like or bump-like identifiers are excluded while keeping other
identifiers like signer_count or bump_amount.
ea9a6b1 to
3ba85c0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sdk-libs/macros/src/light_pdas/accounts/light_account.rs`:
- Around line 740-741: Add a unit test that calls parse_light_account_attr with
the third parameter set to Some(ident) instead of &None to exercise the branch
where defaults should pick the direct proof identifier; specifically construct a
test input (field and ident) that relies on defaulting behavior, call
parse_light_account_attr(&field, &ident, &Some(ident.clone())), assert
result.is_ok(), and then assert the returned structure uses the provided direct
proof identifier in the expected fields (e.g., the direct_proof identifier slot
and any derived default names) to ensure the Some(...) path is covered and
behaves like the existing &None cases.
In `@sdk-libs/macros/src/light_pdas/accounts/parse.rs`:
- Around line 213-218: find_direct_proof_arg currently returns the first
matching CreateAccountsProof arg which makes selection ambiguous if multiple
matches exist; change it to detect multiple matches using
is_create_accounts_proof_type (e.g., collect all args where
is_create_accounts_proof_type(&arg.ty) is true) and, if more than one is found,
return/emit a compile error (use syn::Error::new_spanned or produce a
compile_error! token stream) instead of silently picking the first; update the
function signature and callers (e.g., any code expecting Option<Ident>) to
propagate the Result/syn::Error so the proc-macro fails with a clear message
when multiple CreateAccountsProof args are present.
♻️ Duplicate comments (3)
sdk-libs/macros/src/light_pdas/program/instructions.rs (1)
630-630: Move the import outside the loop for efficiency.The
usestatement is still inside theforloop. While functionally correct, it's more idiomatic to place it at the block scope before the loop begins.♻️ Suggested refactor
// Auto-wrap instruction handlers that use rentfree Accounts structs if let Some((_, ref mut items)) = module.content { + use crate::light_pdas::program::parsing::ExtractResult; for item in items.iter_mut() { if let Item::Fn(fn_item) = item { // Check if this function uses a rentfree Accounts struct - use crate::light_pdas::program::parsing::ExtractResult; match extract_context_and_params(fn_item) {sdk-libs/macros/src/light_pdas/accounts/parse.rs (1)
248-251: Validate directCreateAccountsProofargument ordering to avoid out-of-scope identifiers.
direct_proof_argis captured, but if that argument isn’t the first#[instruction]arg, the generated signature can omit it while codegen still emits#proof_ident, leading to a compile error. Enforce ordering or adjust the generated signature accordingly.🛠️ Suggested validation
let direct_proof_arg = find_direct_proof_arg(&instruction_args); + if let (Some(proof_ident), Some(args)) = (&direct_proof_arg, &instruction_args) { + if args.first().map(|a| &a.name) != Some(proof_ident) { + return Err(Error::new_spanned( + proof_ident, + "CreateAccountsProof must be the first #[instruction] argument when passed directly; otherwise wrap it inside params.create_accounts_proof.", + )); + } + }sdk-libs/macros/src/light_pdas/program/parsing.rs (1)
409-412: Tighten the params filter to avoid false negatives.
contains("signer")/contains("bump")can exclude legitimate params likesigner_countorbump_amount. Prefer exact or suffix/prefix checks.♻️ Proposed refinement
- if !name.contains("signer") && !name.contains("bump") { + let is_excluded = name == "signer" + || name == "bump" + || name.ends_with("_signer") + || name.ends_with("_bump"); + if !is_excluded { params_candidates.push(pat_ident.ident.clone()); }
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Bug Fixes
New Behavior
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.