feat: Impl RequestUndelegation and UndelegateWithRollbackAfterTimeout#183
feat: Impl RequestUndelegation and UndelegateWithRollbackAfterTimeout#183snawaz wants to merge 12 commits into
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo new instruction handlers are added to the DLP delegation program: 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: 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/test_request_undelegation.rs`:
- Around line 170-224: The test suite currently only validates the happy-path
case for the undelegate_with_request instruction with optional accounts. Add a
new test function that creates a malformed undelegate_with_request instruction
with exactly one optional account instead of the required zero or two, then
assert that processing the transaction returns an InvalidInstructionData error.
This regression test ensures the strict optional-account validation logic in
src/processor/fast/undelegate.rs (lines 101-111) continues to work correctly.
🪄 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: f31d3f2e-84de-4ecc-99a0-022139c3c9bc
📒 Files selected for processing (16)
dlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/instruction_builder/request_undelegation.rsdlp-api/src/instruction_builder/undelegate.rsdlp-api/src/pda.rsdlp-api/src/requires.rsdlp-api/src/state/mod.rsdlp-api/src/state/undelegation_request.rsdlp-api/src/state/utils/discriminator.rssrc/lib.rssrc/processor/fast/mod.rssrc/processor/fast/request_undelegation.rssrc/processor/fast/undelegate.rstests/fixtures/accounts.rstests/test_request_undelegation.rs
snawaz
force-pushed
the
snawaz/request-undelegation
branch
from
a2ffa59 to
4d45bec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/carry_over_requested_undelegation.rs`:
- Around line 252-259: The code checks if commit_state_account and
commit_record_account are uninitialized and returns early without verifying
these are the expected PDA addresses, allowing an attacker to pass arbitrary
empty accounts. Before the uninitialized check on commit_state_account and
commit_record_account, add validation to ensure both accounts are the correct
expected PDA addresses for commit-state and commit-record respectively. Only
after confirming the accounts match the expected PDAs should the code proceed to
check if they are uninitialized and return Ok(()).
- Around line 61-65: The `require_owned_pda` call on `delegated_account`
validates ownership but does not verify the account is writable before it is
later mutated. Add a writability validation check immediately after the
ownership validation to ensure `delegated_account` has the writable flag set
before proceeding with any mutations or unsafe owner changes. This guard should
reject readonly account metadata early in the function.
In `@tests/test_carry_over_requested_undelegation.rs`:
- Around line 162-168: The test currently only checks that the transaction fails
with assert!(res.is_err()) and verifies the request PDA exists, which is
insufficient. Strengthen this test by asserting the specific error variant or
message related to "request not expired" instead of just checking is_err, and
add additional assertions to verify that the delegation record metadata and
delegated account state remain unchanged after the failed transaction to ensure
no partial mutations occurred.
In `@tests/test_request_undelegation.rs`:
- Around line 159-160: The test at the process_transaction call is using a
generic is_err() assertion that will pass for any transaction error, not just
the specific timeout validation error being tested. Replace this broad check
with a specific assertion that extracts the error result and verifies it matches
the exact custom program error for "timeout below minimum" validation. This will
ensure the test fails only for the intended error condition and not for
unrelated transaction failures.
🪄 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: c60d13ee-2ecb-450c-a11f-c695b6f5eb80
📒 Files selected for processing (14)
dlp-api/src/consts.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/carry_over_requested_undelegation.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/instruction_builder/request_undelegation.rsdlp-api/src/state/undelegation_request.rssrc/lib.rssrc/processor/fast/carry_over_requested_undelegation.rssrc/processor/fast/mod.rssrc/processor/fast/request_undelegation.rstests/fixtures/accounts.rstests/test_carry_over_requested_undelegation.rstests/test_request_undelegation.rs
There was a problem hiding this comment.
Actionable comments posted: 1
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)
103-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the shared undelegation-request seed helper.
Line 103 manually reconstructs the PDA seeds even though
dlp-api/src/pda.rsdefinesundelegation_request_seeds_from_delegated_account!; using the shared helper keeps processor and client/builder PDA contracts from drifting.🤖 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 103 - 130, The undelegation request PDA seeds are being rebuilt manually in request_undelegation.rs instead of using the shared helper, which risks drifting from the client-side PDA contract. Update the undelegation flow to use undelegation_request_seeds_from_delegated_account! from the shared pda module wherever request_seeds is constructed, including the require_uninitialized_pda and create_pda paths, so the processor and builder stay aligned.
🤖 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/carry_over_requested_undelegation.rs`:
- Around line 97-127: The timeout flow in carry_over_requested_undelegation
currently validates the delegation owner and rent payer but does not bind the
loaded UndelegationRequest to the active delegation instance. Update the logic
in the carry_over_requested_undelegation path to compare
request.delegation_nonce_at_request against the current delegation state from
DelegationMetadata (or its equivalent current nonce/slot snapshot) before
allowing teardown, and return an error when they differ. Add a regression test
around the carry_over_requested_undelegation handler covering the
re-delegated/stale request case to ensure an expired request cannot affect a
newer delegation.
---
Outside diff comments:
In `@src/processor/fast/request_undelegation.rs`:
- Around line 103-130: The undelegation request PDA seeds are being rebuilt
manually in request_undelegation.rs instead of using the shared helper, which
risks drifting from the client-side PDA contract. Update the undelegation flow
to use undelegation_request_seeds_from_delegated_account! from the shared pda
module wherever request_seeds is constructed, including the
require_uninitialized_pda and create_pda paths, so the processor and builder
stay aligned.
🪄 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: ce808865-72db-46be-a7a0-3aeefacb34be
📒 Files selected for processing (16)
dlp-api/src/consts.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/carry_over_requested_undelegation.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/instruction_builder/request_undelegation.rsdlp-api/src/state/undelegation_request.rssrc/lib.rssrc/processor/fast/carry_over_requested_undelegation.rssrc/processor/fast/mod.rssrc/processor/fast/request_undelegation.rstests/fixtures/accounts.rstests/test_call_handler_v2.rstests/test_carry_over_requested_undelegation.rstests/test_lamports_settlement.rstests/test_request_undelegation.rs
💤 Files with no reviewable changes (1)
- tests/test_lamports_settlement.rs
snawaz
force-pushed
the
snawaz/request-undelegation
branch
from
d2555d9 to
3cd51cc
Compare
snawaz
force-pushed
the
snawaz/request-undelegation
branch
from
803cc35 to
2f367f4
Compare
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)
dlp-api/src/error.rs (1)
158-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the existing custom error code for
InvalidPendingCommitState.Line 182 moves an existing
DlpErrorvariant to52by inserting the new request errors ahead of it. That is a wire-level breaking change: Solana surfaces these asInstructionError::Custom(code), and the provided tests already compareDlpError as u32. Please keepInvalidPendingCommitStateon its previous discriminant and assign the new undelegation-request errors after it instead.🤖 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 `@dlp-api/src/error.rs` around lines 158 - 183, Preserve the existing custom error code for InvalidPendingCommitState in DlpError and do not change its discriminant when adding the new undelegation-request variants. Move the new request-related errors so they are assigned after InvalidPendingCommitState, keeping the existing variant’s numeric value stable for InstructionError::Custom and tests that cast DlpError as u32. Locate the enum in error.rs by the InvalidPendingCommitState variant and adjust the surrounding discriminants accordingly.
🤖 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 `@dlp-api/src/error.rs`:
- Around line 158-183: Preserve the existing custom error code for
InvalidPendingCommitState in DlpError and do not change its discriminant when
adding the new undelegation-request variants. Move the new request-related
errors so they are assigned after InvalidPendingCommitState, keeping the
existing variant’s numeric value stable for InstructionError::Custom and tests
that cast DlpError as u32. Locate the enum in error.rs by the
InvalidPendingCommitState variant and adjust the surrounding discriminants
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e917a490-9e60-4cad-b1c3-f990e2d9bb36
📒 Files selected for processing (12)
.github/workflows/run-tests.ymldlp-api/src/consts.rsdlp-api/src/discriminator.rsdlp-api/src/error.rsdlp-api/src/instruction_builder/mod.rsdlp-api/src/instruction_builder/undelegate_after_request_timeout.rsdlp-api/src/state/undelegation_request.rssrc/lib.rssrc/processor/fast/mod.rssrc/processor/fast/undelegate_after_request_timeout.rstests/test_request_undelegation.rstests/test_undelegate_after_request_timeout.rs
snawaz
changed the title
…meout and enforce program-authorization, removing permissionless
snawaz
changed the title
snawaz
changed the title
snawaz
force-pushed
the
snawaz/request-undelegation
branch
from
ccbdd76 to
28bc3c1
Compare
GabrielePicco
left a comment
GabrielePicco
left a comment
There was a problem hiding this comment.
Overall LGTM, but left some questions and comments
|
|
||
| let request_accounts = match optional_accounts.len() { | ||
| 0 => None, | ||
| 2 => { |
There was a problem hiding this comment.
When an active request exists but the validator uses the existing 12-account undelegate path, this branch lets undelegation succeed without closing the request PDA. After that the delegated account is no longer DLP-owned and the delegation PDAs are closed, so neither re-request nor timeout rollback can validate and close the request, leaving request rent stranded and scanners with a stale pending request.
| serde = { version = "1.0.228", default-features = false, features = ["derive"] } | ||
|
|
||
| solana-pubkey-compat = { package = "solana-pubkey", version = "2.4", features = ["borsh", "bytemuck", "curve25519"] } | ||
| wheels = { git = "https://github.com/magicblock-labs/magicblock-wheels.git", rev = "5b83b56" } |
There was a problem hiding this comment.
We need to publish wheels otherwise the API crate won't be publishable anymore
| solana --version | ||
| solana-keygen new --silent --no-bip39-passphrase | ||
|
|
||
| - name: log disk space |
|
|
||
| #[variable_offset_layout(buffer_offset = 0)] | ||
| pub struct RequestUndelegationArgs { | ||
| pub timeout_slots: Option<u16>, // number of slots as timeout |
There was a problem hiding this comment.
this shouldn't be configurable as it's otherwise an attack vector, should be removed and defined as a protocol constant
|
|
||
| let timeout_slots = RequestUndelegationArgs::decode(data)? | ||
| .timeout_slots() | ||
| .unwrap_or(DEFAULT_UNDELEGATION_REQUEST_TIMEOUT_SLOTS as u16) |
There was a problem hiding this comment.
This should not be configurable trough args
| DlpError::UndelegationRequestTimeoutTooShort | ||
| ); | ||
|
|
||
| if is_uninitialized_account(undelegation_request_account) { |
There was a problem hiding this comment.
We should return Ok in the else branch if we want this to be idempotent, or otherwise fail the transaction.
If there is already a pending request, it should either be honored or closed. Updating it adds unnecessary extra complexity, in my opinion, and potentially introduces an attack vector where someone can keep updating it, bumping the nonce and/or the timeout slot.
| /// request/re-request path for the delegated account. The timed-out rollback is | ||
| /// then restricted to the request rent payer so unrelated callers cannot race to | ||
| /// execute it. For non-empty accounts, the owner program must also accept the |
There was a problem hiding this comment.
Q: "The timed-out rollback is then restricted to the request rent payer so unrelated callers cannot race to execute it"
Racing is a desirable property to undelegate after the timeout, what's the advantage of this restriction?
2 participants
RequestUndelegationto be invoked by owner program through CPIUndelegateWithRollbackAfterTimeoutto be invoked by anyone once timeout is expired.Rollbackin it.CHECKPOINTcomment.Undelegateinstruction to optionally take additional accounts to handle request-undelegation.