feat: add commits fees#134
Conversation
snawaz
left a comment
There was a problem hiding this comment.
Looks good to me!
Posted one question and couple of nits.
snawaz
left a comment
There was a problem hiding this comment.
The simplification looks awesome. I found a subtle bug though, once that is fixed, we can merge it.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor/fast/utils/pda.rs (1)
87-116: Well-structured fee distribution logic.The refactored function cleanly handles:
- Proportional fee extraction from available lamports
- Protocol/validator fee splitting (10%/90%)
- Remainder reimbursement to rent payer
One minor robustness consideration on line 101: while the current domain constraints make overflow impossible (fees are capped by PDA rents which are tiny fractions of u64::MAX), using a
u128intermediate as suggested in past reviews would add defense-in-depth:♻️ Optional: Use u128 intermediate for overflow safety
- let protocol_fee = fee_taken * PROTOCOL_FEES_PERCENTAGE as u64 / 100; + let protocol_fee = ((fee_taken as u128 * PROTOCOL_FEES_PERCENTAGE as u128) / 100) as u64;This matches the pattern suggested in past reviews and guards against any future changes that might increase fee magnitudes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/consts.rssrc/processor/fast/undelegate.rssrc/processor/fast/utils/pda.rstests/fixtures/accounts.rstests/test_commit_fees_on_undelegation.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:45:25.802Z
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.
Applied to files:
src/processor/fast/undelegate.rs
🧬 Code graph analysis (2)
tests/fixtures/accounts.rs (1)
src/state/delegation_metadata.rs (1)
seeds(35-35)
src/processor/fast/undelegate.rs (1)
src/processor/fast/utils/pda.rs (1)
close_pda_with_fees(87-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (10)
src/consts.rs (1)
10-14: LGTM!The new fee constants are well-documented and appropriately typed. The values (0.0001 SOL per commit, 0.0003 SOL per session) are reasonable for micropayments on Solana.
tests/fixtures/accounts.rs (1)
107-138: LGTM!Good refactoring approach - the existing
create_delegation_metadata_datanow delegates to the new parameterized function, maintaining backward compatibility while enabling nonce-aware test scenarios.tests/test_commit_fees_on_undelegation.rs (2)
22-69: LGTM!The test correctly validates the fee calculation logic:
- Sets nonce to 101, implying 100 commits (101 - 1 = 100)
- Calculates expected fees using the same formula as the implementation
- Verifies the protocol fee vault receives 10% of total fees
The fee calculation at line 40-41 correctly caps fees at available lamports, matching the
min()logic inprocess_delegation_cleanup.
71-165: LGTM!Comprehensive test setup that properly initializes all required accounts for the undelegation flow. The high nonce (101) effectively tests the commit fee calculation path.
src/processor/fast/undelegate.rs (5)
14-14: LGTM!Correctly imports the new fee constants for use in the cleanup logic.
134-134: LGTM!Extracts the nonce before dropping the borrow, enabling its use in the cleanup logic.
165-172: LGTM!Correctly passes
delegation_last_update_nonceto the cleanup function for the empty-data fast path.
221-228: LGTM!Correctly passes
delegation_last_update_nonceto the cleanup function for the CPI path.
336-367: LGTM!The fee calculation logic is correct:
saturating_sub(1)safely handles zero nonce (first commit not charged, as clarified in past reviews)checked_mulprotects against overflow in commit fee calculationmin()ensures fees never exceed available lamports from the PDAs- Shared mutable
fee_remainingcorrectly distributes fees across both PDA closuressrc/processor/fast/utils/pda.rs (1)
10-10: LGTM!Correctly imports the protocol fee percentage constant for fee routing calculations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
Add and apply commits fees
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.