feat: auto-undelegate owner-program requests during finalize#184
feat: auto-undelegate owner-program requests during finalize#184snawaz wants to merge 7 commits into
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis PR adds auto-undelegation support to Suggested reviewers
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/test_lamports_settlement.rs (1)
1043-1057: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread the actual rent reimbursement account through these finalize helpers.
Line 1056 and Line 1284 still hardcode
args.authority.pubkey()into the newrent_reimbursementslot. In the same file, the directcommit_finalizecalls passpayer.pubkey()as reimbursement, and the undelegation path validates that this account matchesdelegation_metadata.rent_payer. These helpers will build the wrong finalize instruction for any path that exercises auto-undelegation or reimbursement validation.Suggested fix
struct FinalizeNewStateArgs<'a> { banks: &'a mut BanksClient, authority: &'a Keypair, blockhash: Hash, delegated_account: Pubkey, owner_program: Pubkey, + rent_reimbursement: Pubkey, } async fn finalize_new_state(args: FinalizeNewStateArgs<'_>) { let ix = dlp_api::instruction_builder::finalize( args.authority.pubkey(), args.delegated_account, args.owner_program, - args.authority.pubkey(), + args.rent_reimbursement, ); // ... } struct FinalizeWithFeePayerArgs<'a> { banks: &'a mut BanksClient, authority: &'a Keypair, fee_payer: &'a Keypair, label: &'a str, delegated_account: Pubkey, owner_program: Pubkey, + rent_reimbursement: Pubkey, } async fn finalize_with_fee_payer(args: FinalizeWithFeePayerArgs<'_>) { let ix = dlp_api::instruction_builder::finalize( args.authority.pubkey(), args.delegated_account, args.owner_program, - args.authority.pubkey(), + args.rent_reimbursement, ); // ... }Also applies to: 1270-1286
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lamports_settlement.rs` around lines 1043 - 1057, The finalize helper paths are still hardcoding the reimbursement account to the authority key, which breaks auto-undelegation and rent_payer validation. Update the finalize instruction construction in finalize_new_state and the related helper used by commit_finalize so the actual rent reimbursement account is threaded through as an argument, and use that value instead of args.authority.pubkey() when populating the rent_reimbursement slot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/processor/fast/internal/undelegate_internal.rs`:
- Around line 461-467: Use checked arithmetic for the cleanup fee calculation in
undelegate_internal::undelegate_internal. `commit_fee` is already guarded with
`checked_mul`, but the later `commit_fee + SESSION_FEE_LAMPORTS` and the
combined lamport total can still overflow before the `min(...)` logic, so
replace those additions with checked/saturating handling that returns
`DlpError::Overflow` on failure. Keep the fix localized around
`total_fee_requested` and `total_lamports` so the fee/reimbursement split
remains correct.
In `@src/processor/fast/undelegate.rs`:
- Around line 100-105: The idempotent fast-path in undelegate_account is too
broad because it returns Ok(()) whenever delegated_account.owner() matches
owner_program.address(), which can incorrectly treat a still-delegated account
owned by crate::fast::ID as already undelegated. Update the check in
undelegate_account so it only short-circuits after confirming the current owner
is not the DLP program, keeping the existing validation path for accounts still
owned by crate::fast::ID.
---
Outside diff comments:
In `@tests/test_lamports_settlement.rs`:
- Around line 1043-1057: The finalize helper paths are still hardcoding the
reimbursement account to the authority key, which breaks auto-undelegation and
rent_payer validation. Update the finalize instruction construction in
finalize_new_state and the related helper used by commit_finalize so the actual
rent reimbursement account is threaded through as an argument, and use that
value instead of args.authority.pubkey() when populating the rent_reimbursement
slot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b565f45f-962f-4564-a6a7-9825ec228013
📒 Files selected for processing (23)
dlp-api/src/instruction_builder/commit_finalize.rsdlp-api/src/instruction_builder/commit_finalize_from_buffer.rsdlp-api/src/instruction_builder/finalize.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/commit_state.rssrc/processor/fast/finalize.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/internal/mod.rssrc/processor/fast/internal/undelegate_internal.rssrc/processor/fast/request_undelegation.rssrc/processor/fast/undelegate.rssrc/processor/fast/undelegate_confined_account.rstests/test_call_handler.rstests/test_call_handler_v2.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rstests/test_commit_undelegate_zero_lamports_system_owned.rstests/test_finalize.rstests/test_lamports_settlement.rstests/test_request_undelegation.rstests/test_undelegate.rstests/test_undelegate_with_rollback_after_timeout.rs
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)
src/processor/fast/request_undelegation.rs (1)
93-105: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBackfill
undelegation_requesterwhen reusing an existing request.Lines 158-198 only revalidate the existing PDA; they never mirror the creation-path write from Lines 137-142. That leaves preexisting requests with
DelegationMetadata.undelegation_requester == None, andprocess_auto_undelegation_if_requested()insrc/processor/fast/internal/undelegate_internal.rsexits early unless the requester isOwnerProgram, so finalize will never auto-undelegate those already-open requests even after a retry. Lines 100-105 also pin retries toDelegationMetadata::rent_payer, so some legacy requests cannot use this path to repair themselves. Please persistOwnerProgramhere too, or add an explicit migration/compatibility path for existing requests.Also applies to: 158-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/processor/fast/request_undelegation.rs` around lines 93 - 105, Backfill the missing undelegation requester when reusing an existing request: the retry path in request_undelegation should mirror the creation-path write to DelegationMetadata.undelegation_requester instead of only revalidating the PDA. Update the existing-request branch in request_undelegation to persist OwnerProgram on the loaded DelegationMetadata (the same symbol used in the create flow), or add a compatibility/migration branch for legacy records so process_auto_undelegation_if_requested can proceed for preexisting requests. Ensure the rent_payer revalidation stays intact while allowing old requests to be repaired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/processor/fast/request_undelegation.rs`:
- Around line 93-105: Backfill the missing undelegation requester when reusing
an existing request: the retry path in request_undelegation should mirror the
creation-path write to DelegationMetadata.undelegation_requester instead of only
revalidating the PDA. Update the existing-request branch in request_undelegation
to persist OwnerProgram on the loaded DelegationMetadata (the same symbol used
in the create flow), or add a compatibility/migration branch for legacy records
so process_auto_undelegation_if_requested can proceed for preexisting requests.
Ensure the rent_payer revalidation stays intact while allowing old requests to
be repaired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6f63787-378f-4bc7-8494-c10b60c65e18
📒 Files selected for processing (3)
dlp-api/src/state/undelegation_request.rssrc/processor/fast/internal/undelegate_internal.rssrc/processor/fast/request_undelegation.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/tests/test-delegation.ts`:
- Line 368: The delegation threshold assertion and its failure message are out
of sync: the test currently uses the new 55000 limit but the adjacent
expect/assert message still mentions 18500. Update the message to match the
actual threshold, or better, extract the limit into a shared constant and reuse
it in both the assertion and the message in test-delegation.ts so the check
stays consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74a19563-70ef-498d-aed0-6e872f149b08
📒 Files selected for processing (2)
src/processor/fast/undelegate.rstests/integration/tests/test-delegation.ts
snawaz
force-pushed
the
snawaz/auto-undelegate
branch
from
7b7c5b0 to
756805e
Compare
snawaz
force-pushed
the
snawaz/auto-undelegate
branch
from
756805e to
72af1c7
Compare
|
discarded because the idea of undelegation in finalize step is discarded. |
1 participant
added owner-program requested undelegation with auto-undelegation during finalize/commit-finalize using
DelegationMetadata::undelegation_requesterto trigger the flow and close the request/delegation PDAs.enforced (and documented) the shared rent payer invariant for owner-program requests so validators can fetch one account only (metadata) instead of two accounts, derive the request PDA, and avoid fetching
UndelegationRequest(which wont exist for 99.999% cases) during every finalize.Summary by CodeRabbit
New Features
Bug Fixes
Tests