fix: ignore compute budget parsing#17
Conversation
WalkthroughModified transaction processor to replace compute budget instruction processing with default compute budget limits in the fee-payer validation function, eliminating associated error counter increments during this validation step. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 (2)
src/transaction_processor.rs (2)
32-32: 🧹 Nitpick | 🔵 TrivialUnused import in production code.
process_compute_budget_instructionsis imported but no longer used in production code after the change at line 639. It's only referenced by test code. Consider either:
- Moving this import inside the
#[cfg(test)]module, or- Using
#[cfg(test)]conditional compilation for this import♻️ Suggested refactor
- solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions,Then add inside the
#[cfg(test)] mod testsblock:use solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/transaction_processor.rs` at line 32, The import solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions is unused in production and should be conditionally compiled for tests; either move that use into the existing #[cfg(test)] mod tests block (add use solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions; inside the tests module) or wrap the top-level import with #[cfg(test)] so it is only compiled for tests, ensuring no unused-import warning in production while keeping the symbol available to test code.
2581-2610:⚠️ Potential issue | 🔴 CriticalTest will fail due to removed compute budget parsing.
This test expects
validate_transaction_nonce_and_fee_payerto reject transactions with duplicateset_compute_unit_limitinstructions by returningTransactionError::DuplicateInstruction(1u8)and incrementingerror_counters.invalid_compute_budget.With the change at line 639 replacing
process_compute_budget_instructions()withComputeBudgetLimits::default(), compute budget instructions are no longer validated during fee payer validation, so this test will fail.Either:
- Remove/update this test if the behavior change is intentional, or
- Reconsider the change at line 639 if this validation is still needed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/transaction_processor.rs` around lines 2581 - 2610, The test test_validate_transaction_fee_payer_invalid_compute_budget now fails because validate_transaction_nonce_and_fee_payer no longer parses compute-budget instructions (the call to process_compute_budget_instructions was replaced with ComputeBudgetLimits::default()), so duplicate set_compute_unit_limit instructions are not detected and error_counters.invalid_compute_budget isn't incremented; to fix, either restore compute-budget parsing/validation inside validate_transaction_nonce_and_fee_payer by calling process_compute_budget_instructions (or equivalent logic) so DuplicateInstruction(1u8) is returned and error_counters.invalid_compute_budget is incremented, or if the behavioral change is intentional, update/remove the test test_validate_transaction_fee_payer_invalid_compute_budget to reflect the new behavior (and any expectations around TransactionError::DuplicateInstruction and error_counters.invalid_compute_budget).
🤖 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 `@src/transaction_processor.rs`:
- Line 32: The import
solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions
is unused in production and should be conditionally compiled for tests; either
move that use into the existing #[cfg(test)] mod tests block (add use
solana_compute_budget_instruction::instructions_processor::process_compute_budget_instructions;
inside the tests module) or wrap the top-level import with #[cfg(test)] so it is
only compiled for tests, ensuring no unused-import warning in production while
keeping the symbol available to test code.
- Around line 2581-2610: The test
test_validate_transaction_fee_payer_invalid_compute_budget now fails because
validate_transaction_nonce_and_fee_payer no longer parses compute-budget
instructions (the call to process_compute_budget_instructions was replaced with
ComputeBudgetLimits::default()), so duplicate set_compute_unit_limit
instructions are not detected and error_counters.invalid_compute_budget isn't
incremented; to fix, either restore compute-budget parsing/validation inside
validate_transaction_nonce_and_fee_payer by calling
process_compute_budget_instructions (or equivalent logic) so
DuplicateInstruction(1u8) is returned and error_counters.invalid_compute_budget
is incremented, or if the behavioral change is intentional, update/remove the
test test_validate_transaction_fee_payer_invalid_compute_budget to reflect the
new behavior (and any expectations around TransactionError::DuplicateInstruction
and error_counters.invalid_compute_budget).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ae7080e-7aaf-4176-aa53-6b30def68188
📒 Files selected for processing (1)
src/transaction_processor.rs
Summary by CodeRabbit