Fix: handle lamports properly in CommitFinalize#148
Merged
Conversation
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
3 times, most recently
from
124f94d to
17a016f
Compare
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
17a016f to
625998f
Compare
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/error.rs`:
- Around line 152-155: The error message on the InsufficientRent enum variant
uses incorrect verb agreement; update the #[error(...)] string for the
InsufficientRent variant so it reads "The account lamports are too small to make
the account rent-exempt" -> change "is" to "are" (i.e., "The account lamports
are too small to make the account rent-exempt") by editing the #[error(...)]
attribute above the InsufficientRent variant.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 118-144: The delegation record is set from the account balance
after settling, which leaves excess lamports when the account initially had more
than delegation_record; update the assignment so delegation_record.lamports is
explicitly set to the target args.commit_lamports (instead of
args.delegated_account.lamports()), i.e., change the final reconciliation to
assign delegation_record.lamports = args.commit_lamports so the commit reaches
the intended value after the Transfer/increment/decrement logic in this function
handling args.commit_lamports, delegation_record, and args.delegated_account.
In `@tests/test_commit_finalize.rs`:
- Around line 319-378: Add a failing boundary test to ensure commit_finalize
rejects underfunded accounts: in tests/test_commit_finalize.rs (near
test_commit_finalize_lamports_decrease) add a case that computes the minimum
rent (Rent::minimum_balance) for the delegation record / delegated account data
length and sets commit_lamports to a value strictly less than that, build the
same commit_finalize instruction (using
dlp::instruction_builder::commit_finalize and CommitFinalizeArgs), send the
transaction with Transaction::new_signed_with_payer and call
banks.process_transaction(tx).await and assert it returns an error (use
unwrap_err() or is_err()), and optionally assert that neither the delegated PDA
nor the delegation_record PDA were created/changed (get_account returns None or
unchanged lamports); this ensures the rent guard prevents underfunded commits.
- Around line 302-317: The test currently asserts the destination balances
(delegated_account and DelegationRecord) but doesn't ensure the extra lamports
didn't come from the validator fees vault; capture the vault balance before the
operation and assert it is unchanged after the commit by fetching the vault
account (e.g., validator_fees_vault or the constant/ID used for the fees vault),
calling banks.get_account(VAULT_ID).await.unwrap().unwrap(), storing its
lamports, and then after the existing assertions
assert_eq!(vault_account.lamports, original_vault_lamports) to pin the increase
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af3d8ac0-1a91-4da0-927f-318ae7cb22aa
📒 Files selected for processing (9)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_commit_finalize.rs (2)
268-317:⚠️ Potential issue | 🟠 MajorAdd the below-rent rejection case.
The new lamport tests still only cover successful grow/shrink flows. Please add a case where
CommitFinalizeArgs.lamportsisRent::default().minimum_balance(new_state.len()) - 1, assertprocess_transactionfails, and assert the delegated PDA plusDelegationRecord.lamportsstay unchanged so the rent guard cannot regress silently. Based on learnings, in Solana tests usingsolana_program_test, useRent::default()instead ofRent::get()because the rent sysvar is unavailable in the test context.Also applies to: 319-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commit_finalize.rs` around lines 268 - 317, Add a new negative test case in the same test module (e.g., alongside test_commit_finalize_lamports_increase) that builds a CommitFinalizeArgs where lamports = Rent::default().minimum_balance(new_state_len) - 1 (use Rent::default() not Rent::get()), call the instruction via Transaction::new_signed_with_payer and assert banks.process_transaction(tx).await.unwrap_err() (i.e., the transaction fails), then reload the DELEGATED_PDA_ID account and pdas.delegation_record account and assert both their lamports values remain unchanged (use DelegationRecord::try_from_bytes_with_discriminator to read delegation_record.lamports) to ensure the rent guard prevented regression.
302-317:⚠️ Potential issue | 🟠 MajorAssert that the grow path does not drain
validator_fees_vault.This only proves the destination balances. It still passes if the extra lamports are sourced from
validator_fees_vaultinstead of the validator, so please snapshot the vault beforecommit_finalizeand assert it is unchanged afterwards.Suggested test update
let (ix, pdas) = dlp::instruction_builder::commit_finalize( authority.pubkey(), DELEGATED_PDA_ID, &mut CommitFinalizeArgs { commit_id: 1, allow_undelegation: false.into(), data_is_diff: false.into(), lamports: commit_lamports, bumps: Default::default(), reserved_padding: Default::default(), }, &vec![1; 8], ); + let initial_fees_vault_lamports = banks + .get_account(pdas.validator_fees_vault) + .await + .unwrap() + .unwrap() + .lamports; let tx = Transaction::new_signed_with_payer( &[ix], Some(&authority.pubkey()), &[&authority], @@ let delegation_record = DelegationRecord::try_from_bytes_with_discriminator( &delegation_record_account.data, ) .unwrap(); assert_eq!(delegation_record.lamports, commit_lamports); + + let fees_vault = banks + .get_account(pdas.validator_fees_vault) + .await + .unwrap() + .unwrap(); + assert_eq!(fees_vault.lamports, initial_fees_vault_lamports); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commit_finalize.rs` around lines 302 - 317, Before invoking commit_finalize in this test, snapshot the lamports of the validator_fees_vault account by calling banks.get_account(validator_fees_vault).await.unwrap().unwrap().lamports and store it (e.g., pre_vault_lamports), then after the existing assertions (delegated_account and delegation_record checks) fetch the vault account again and assert its lamports equal pre_vault_lamports; reference the validator_fees_vault identifier and the commit_finalize test flow so the snapshot is taken immediately before the operation and the equality check is done immediately after.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 118-125: The top-up branch that creates and invokes
system::Transfer (using args.validator -> args.delegated_account with lamports =
commit_lamports - delegation_record.lamports) requires the funding account
(args.validator) to be [WRITE, SIGNER]; currently the validator is emitted
readonly causing CPIs to fail. Fix by ensuring the account metas passed into
this instruction (the account at index 0 / the validator account) are marked
writable and signer before the branch executes (or change the builder that
constructs the instruction metas so args.validator is emitted as writable), so
the system::Transfer::invoke() can succeed.
---
Duplicate comments:
In `@tests/test_commit_finalize.rs`:
- Around line 268-317: Add a new negative test case in the same test module
(e.g., alongside test_commit_finalize_lamports_increase) that builds a
CommitFinalizeArgs where lamports =
Rent::default().minimum_balance(new_state_len) - 1 (use Rent::default() not
Rent::get()), call the instruction via Transaction::new_signed_with_payer and
assert banks.process_transaction(tx).await.unwrap_err() (i.e., the transaction
fails), then reload the DELEGATED_PDA_ID account and pdas.delegation_record
account and assert both their lamports values remain unchanged (use
DelegationRecord::try_from_bytes_with_discriminator to read
delegation_record.lamports) to ensure the rent guard prevented regression.
- Around line 302-317: Before invoking commit_finalize in this test, snapshot
the lamports of the validator_fees_vault account by calling
banks.get_account(validator_fees_vault).await.unwrap().unwrap().lamports and
store it (e.g., pre_vault_lamports), then after the existing assertions
(delegated_account and delegation_record checks) fetch the vault account again
and assert its lamports equal pre_vault_lamports; reference the
validator_fees_vault identifier and the commit_finalize test flow so the
snapshot is taken immediately before the operation and the equality check is
done immediately after.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eccdb8b5-9a3a-47ce-9cf0-c0b911686d1e
📒 Files selected for processing (9)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_commit_finalize.rs`:
- Around line 102-103: Remove the leftover commented debug line
"//tokio::time::sleep(Duration::from_secs(10)).await;" from the
test_commit_finalize test in tests/test_commit_finalize.rs; simply delete that
commented sleep so the test file contains no commented-out debug code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9984f2a0-3247-4a1b-87b0-6e156d2f35b8
📒 Files selected for processing (10)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
b864cb4 to
1755f38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/instruction_builder/commit_finalize_from_buffer.rs (1)
78-91:⚠️ Potential issue | 🟡 MinorSize budget includes account not in instruction.
The
commit_finalize_from_buffer_size_budgetfunction includesprogram_config_pda(line 89) in the account size calculation, but this account is not part of the instruction's account list (lines 50-58 show only 7 accounts, none of which isprogram_config).This will over-estimate the data size budget, which is wasteful but not incorrect. However, it may indicate stale code from a previous version.
🔧 Proposed fix
pub fn commit_finalize_from_buffer_size_budget( delegated_account: AccountSizeClass, ) -> u32 { total_size_budget(&[ DLP_PROGRAM_DATA_SIZE_CLASS, AccountSizeClass::Tiny, // validator delegated_account, // delegated_account AccountSizeClass::Tiny, // delegation_record_pda AccountSizeClass::Tiny, // delegation_metadata_pda delegated_account, // data_buffer AccountSizeClass::Tiny, // validator_fees_vault_pda - AccountSizeClass::Tiny, // program_config_pda AccountSizeClass::Tiny, // system_program ]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instruction_builder/commit_finalize_from_buffer.rs` around lines 78 - 91, The size budget function commit_finalize_from_buffer_size_budget is including an AccountSizeClass::Tiny entry annotated as program_config_pda that is not actually part of the instruction's account list; remove that program_config_pda entry from the array passed to total_size_budget so the returned budget matches the real accounts (keep the other entries: DLP_PROGRAM_DATA_SIZE_CLASS, validator, delegated_account, delegation_record_pda, delegation_metadata_pda, data_buffer, validator_fees_vault_pda, and system_program). Also scan for any stale comments/annotations around program_config_pda in commit_finalize_from_buffer_size_budget and delete them so the function reflects the current instruction accounts.tests/test_commit_finalize.rs (1)
189-214: 🧹 Nitpick | 🔵 TrivialAdd a regression case where the base-chain account already has extra lamports.
The new fixture still forces
delegated_account.lamports == delegation_record.lamports, so this suite won’t catch a future refactor that incorrectly persistsargs.commit_lamportsinstead of the actual post-settlement base-chain balance. Let the helper take separate account and record lamports, then add one increase/decrease case withdelegated_account.lamports() > delegation_record.lamports.Based on learnings:
commit_lamportsis the ER-observed balance, whiledelegated_account.lamports()may already be higher on the base chain anddelegation_record.lamportsis intentionally updated to that actual post-settlement balance.Also applies to: 269-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commit_finalize.rs` around lines 189 - 214, The test helper setup_program_test_env_with_record_lamports currently forces the delegated account and delegation_record to have identical lamports; change it to accept separate "delegated_account_lamports" and "delegation_record_lamports" (keep the function name setup_program_test_env_with_record_lamports but update its parameters and usage) and initialize the ProgramTest added account (DELEGATED_PDA_ID) with delegated_account_lamports while populating the stored delegation record with delegation_record_lamports; then add a regression test case where delegated_account_lamports > delegation_record_lamports to ensure the code uses the actual base-chain balance (delegated_account.lamports()) rather than args.commit_lamports when persisting the final delegation_record.lamports (apply the same change pattern to all affected tests around the other ranges mentioned).src/processor/fast/internal/commit_finalize_internal.rs (1)
68-78:⚠️ Potential issue | 🟠 MajorDon’t require
validator_fees_vaultwritable unless the commit shrinks lamports.Only
std::cmp::Ordering::Lessmutates the vault, but this precheck now forces a write lock on every finalize. Because the vault PDA is shared per validator (src/pda.rs:163-169), that serializes otherwise unrelated commits for the same validator and rejects callers that still pass the legacy readonly meta even when the vault is never touched.🔧 Suggested change
require_initialized_pda_fast!( args.validator_fees_vault, &[ pda::VALIDATOR_FEES_VAULT_TAG, args.validator.address().as_ref(), &[args.bumps.validator_fees_vault], crate::fast::ID.as_ref(), PDA_MARKER ], - true + false ); ... std::cmp::Ordering::Less => { + require!(args.validator_fees_vault.is_writable(), ProgramError::Immutable); let amount = delegation_record.lamports - args.commit_lamports; args.delegated_account.lamports_decrement_by(amount)?; args.validator_fees_vault.lamports_increment_by(amount)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 68 - 78, The require_initialized_pda_fast! call currently forces the validator_fees_vault account to be writable unconditionally; change it to only require writable when the finalize actually decreases the vault lamports (i.e., when the operation is mutating). Detect that by comparing the before/after lamports (or using the existing delta/ordering value) and pass writable = true only when the comparison yields std::cmp::Ordering::Less (otherwise pass writable = false). Update the require_initialized_pda_fast! invocation that references args.validator_fees_vault and args.bumps.validator_fees_vault so the writable flag is conditional on the lamports-decrease check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/processor/fast/commit_finalize_from_buffer.rs`:
- Around line 20-26: The top doc comment listing accounts in
commit_finalize_from_buffer.rs incorrectly marks the validator only as
`[signer]`; update that first entry to `[signer, writable]` to match the
instruction builder and the same fix in commit_finalize.rs so the validator
account documentation is consistent with the actual account flags (update the
comment line that currently reads "0: `[signer]` the validator requesting the
commit" to include `[writable]`).
In `@src/processor/fast/commit_finalize.rs`:
- Around line 21-26: Update the doc comment in commit_finalize.rs to mark the
validator account as writable to match the instruction builder; change the first
entry from "`[signer]` the validator requesting the commit" to include
"`[writable]`" (e.g., "`[signer, writable]`" or similar) so it reflects
AccountMeta::new(validator, true) used when building the instruction and keeps
the comments consistent with the actual account metas.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 116-146: After the match that adjusts lamports (the block using
args.commit_lamports.cmp(&delegation_record.lamports)), add a single
rent-exemption check after the match to ensure the delegated account remains
rent-exempt regardless of branch; call Rent::get()? and use
try_minimum_balance(args.delegated_account.data_len()) to compute the minimum
and require_ge! that args.delegated_account.lamports() meets that minimum,
returning DlpError::InsufficientRent on failure — this uses the existing
args.delegated_account, Rent::get(), try_minimum_balance, and
DlpError::InsufficientRent symbols and should be placed immediately after the
match and before updating delegation_record.lamports.
---
Outside diff comments:
In `@src/instruction_builder/commit_finalize_from_buffer.rs`:
- Around line 78-91: The size budget function
commit_finalize_from_buffer_size_budget is including an AccountSizeClass::Tiny
entry annotated as program_config_pda that is not actually part of the
instruction's account list; remove that program_config_pda entry from the array
passed to total_size_budget so the returned budget matches the real accounts
(keep the other entries: DLP_PROGRAM_DATA_SIZE_CLASS, validator,
delegated_account, delegation_record_pda, delegation_metadata_pda, data_buffer,
validator_fees_vault_pda, and system_program). Also scan for any stale
comments/annotations around program_config_pda in
commit_finalize_from_buffer_size_budget and delete them so the function reflects
the current instruction accounts.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 68-78: The require_initialized_pda_fast! call currently forces the
validator_fees_vault account to be writable unconditionally; change it to only
require writable when the finalize actually decreases the vault lamports (i.e.,
when the operation is mutating). Detect that by comparing the before/after
lamports (or using the existing delta/ordering value) and pass writable = true
only when the comparison yields std::cmp::Ordering::Less (otherwise pass
writable = false). Update the require_initialized_pda_fast! invocation that
references args.validator_fees_vault and args.bumps.validator_fees_vault so the
writable flag is conditional on the lamports-decrease check.
In `@tests/test_commit_finalize.rs`:
- Around line 189-214: The test helper
setup_program_test_env_with_record_lamports currently forces the delegated
account and delegation_record to have identical lamports; change it to accept
separate "delegated_account_lamports" and "delegation_record_lamports" (keep the
function name setup_program_test_env_with_record_lamports but update its
parameters and usage) and initialize the ProgramTest added account
(DELEGATED_PDA_ID) with delegated_account_lamports while populating the stored
delegation record with delegation_record_lamports; then add a regression test
case where delegated_account_lamports > delegation_record_lamports to ensure the
code uses the actual base-chain balance (delegated_account.lamports()) rather
than args.commit_lamports when persisting the final delegation_record.lamports
(apply the same change pattern to all affected tests around the other ranges
mentioned).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7db01719-0a91-4593-a605-e574cefc2ea3
📒 Files selected for processing (10)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
1755f38 to
cc556eb
Compare
Contributor
|
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:
📝 WalkthroughWalkthroughAdded lamport-settlement to commit-finalize: new 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 |
Contributor
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)
tests/test_commit_finalize.rs (1)
188-239: 🧹 Nitpick | 🔵 TrivialSplit delegated-account lamports from record lamports in the fixture.
Line 212 and Line 238 both reuse
record_lamports, so the new suite only exercisesdelegated_account.lamports == delegation_record.lamports. That misses the supported case where the base-chain delegated account already holds extra lamports, which is the key reason the processor updates the record from the actual post-settlement account balance.Based on learnings
commit_lamportsis the ER-observed balance, whiledelegation_record.lamportsshould track the actual post-settlement base-chain balance.Also applies to: 268-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commit_finalize.rs` around lines 188 - 239, The test fixture setup_program_test_env_with_record_lamports incorrectly reuses record_lamports for both the delegated account's actual lamports and the delegation record's stored lamports; change the API and local variables so the delegated account balance and the delegation record's lamports are distinct (e.g., add a delegated_account_lamports parameter or a second local variable) and use delegated_account_lamports when calling program_test.add_account for DELEGATED_PDA_ID and use record_lamports (the delegation_record.lamports) when constructing delegation_record_data via get_delegation_record_data; apply the same split for other occurrences noted (around lines 268-466) to ensure commit_lamports (ER-observed balance) and delegation_record.lamports represent separate values.
♻️ Duplicate comments (3)
src/processor/fast/commit_finalize.rs (1)
21-25:⚠️ Potential issue | 🟡 MinorValidator account doc still misses writability.
Line 21 still says
[signer], but this account can fund a transfer in the lamport-increase branch and should be documented as writable for accuracy.📝 Proposed doc fix
-/// 0: `[signer]` the validator requesting the commit +/// 0: `[signer, writable]` the validator requesting the commitBased on learnings: In
delegation-program(src/instruction_builder/commit_finalize.rsandsrc/instruction_builder/commit_finalize_from_buffer.rs), validator account index 0 must be explicitly writable because finalize may transfer lamports from validator to delegated account.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor/fast/commit_finalize.rs` around lines 21 - 25, Update the account doc for the validator in the commit_finalize account list: change the first entry from `[signer]` to `[signer, writable]` so the validator (account index 0) is documented as writable; this ensures the comment in commit_finalize.rs correctly reflects that the validator may fund lamport transfers during finalize (matching the behavior in commit_finalize instruction builders).src/processor/fast/commit_finalize_from_buffer.rs (1)
20-26:⚠️ Potential issue | 🟡 MinorValidator doc entry is still missing
[writable].Line 20 documents validator as
[signer], but runtime behavior can debit validator lamports in settle-up flows, so docs should reflect writable.📝 Proposed doc fix
-/// 0: `[signer]` the validator requesting the commit +/// 0: `[signer, writable]` the validator requesting the commitBased on learnings: In
delegation-program(src/instruction_builder/commit_finalize.rsandsrc/instruction_builder/commit_finalize_from_buffer.rs), validator account index 0 must be explicitly writable because finalize may transfer lamports from validator to delegated account.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor/fast/commit_finalize_from_buffer.rs` around lines 20 - 26, The account docs for commit_finalize_from_buffer.rs incorrectly list the validator (account index 0) as just `[signer]` but it can be debited during finalize flows; update the account comment for the validator to `[signer, writable]` in the header block of commit_finalize_from_buffer.rs (the same spot documenting accounts 0–6) so the validator is explicitly writable — ensure the comment change mirrors the convention used in commit_finalize.rs/commit_finalize_from_buffer.rs and mentions account 0 (validator).src/processor/fast/internal/commit_finalize_internal.rs (1)
116-143:⚠️ Potential issue | 🟠 MajorMove the rent guard out of the
Lessarm.After Line 116 changes
data_len, the rent floor changes for every path. Lines 135-140 only enforceDlpError::InsufficientRentinOrdering::Less, so a resize that still leaves the account below the new minimum balance can succeed inOrdering::GreaterandOrdering::Equal.Suggested fix
match args.commit_lamports.cmp(&delegation_record.lamports) { std::cmp::Ordering::Greater => { require!(args.validator.is_writable(), ProgramError::Immutable); system::Transfer { from: args.validator, to: args.delegated_account, lamports: args.commit_lamports - delegation_record.lamports, } .invoke()?; } std::cmp::Ordering::Less => { let amount = delegation_record.lamports - args.commit_lamports; args.delegated_account.lamports_decrement_by(amount)?; args.validator_fees_vault.lamports_increment_by(amount)?; - - // require the account is still rent-exempted even after decrementing lamports - require_ge!( - args.delegated_account.lamports(), - Rent::get()? - .try_minimum_balance(args.delegated_account.data_len())?, - DlpError::InsufficientRent - ); } std::cmp::Ordering::Equal => {} } + + require_ge!( + args.delegated_account.lamports(), + Rent::get()? + .try_minimum_balance(args.delegated_account.data_len())?, + DlpError::InsufficientRent + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 116 - 143, The rent-exemption check currently lives only in the std::cmp::Ordering::Less branch (inside the match on args.commit_lamports.cmp(&delegation_record.lamports)), which misses cases where args.delegated_account.resize(...) changed the required minimum for the Greater or Equal branches; move the require_ge! rent guard out of that Less arm and run it once after the match (after args.delegated_account.resize(args.new_state.data_len())? completes and after any lamports transfers/decrements/increments have been applied) by comparing args.delegated_account.lamports() against Rent::get()?.try_minimum_balance(args.delegated_account.data_len())? and returning DlpError::InsufficientRent if below the floor, leaving the existing transfer (system::Transfer::invoke), lamports_decrement_by, and lamports_increment_by calls unchanged.
🤖 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 `@tests/test_commit_finalize.rs`:
- Around line 188-239: The test fixture
setup_program_test_env_with_record_lamports incorrectly reuses record_lamports
for both the delegated account's actual lamports and the delegation record's
stored lamports; change the API and local variables so the delegated account
balance and the delegation record's lamports are distinct (e.g., add a
delegated_account_lamports parameter or a second local variable) and use
delegated_account_lamports when calling program_test.add_account for
DELEGATED_PDA_ID and use record_lamports (the delegation_record.lamports) when
constructing delegation_record_data via get_delegation_record_data; apply the
same split for other occurrences noted (around lines 268-466) to ensure
commit_lamports (ER-observed balance) and delegation_record.lamports represent
separate values.
---
Duplicate comments:
In `@src/processor/fast/commit_finalize_from_buffer.rs`:
- Around line 20-26: The account docs for commit_finalize_from_buffer.rs
incorrectly list the validator (account index 0) as just `[signer]` but it can
be debited during finalize flows; update the account comment for the validator
to `[signer, writable]` in the header block of commit_finalize_from_buffer.rs
(the same spot documenting accounts 0–6) so the validator is explicitly writable
— ensure the comment change mirrors the convention used in
commit_finalize.rs/commit_finalize_from_buffer.rs and mentions account 0
(validator).
In `@src/processor/fast/commit_finalize.rs`:
- Around line 21-25: Update the account doc for the validator in the
commit_finalize account list: change the first entry from `[signer]` to
`[signer, writable]` so the validator (account index 0) is documented as
writable; this ensures the comment in commit_finalize.rs correctly reflects that
the validator may fund lamport transfers during finalize (matching the behavior
in commit_finalize instruction builders).
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 116-143: The rent-exemption check currently lives only in the
std::cmp::Ordering::Less branch (inside the match on
args.commit_lamports.cmp(&delegation_record.lamports)), which misses cases where
args.delegated_account.resize(...) changed the required minimum for the Greater
or Equal branches; move the require_ge! rent guard out of that Less arm and run
it once after the match (after
args.delegated_account.resize(args.new_state.data_len())? completes and after
any lamports transfers/decrements/increments have been applied) by comparing
args.delegated_account.lamports() against
Rent::get()?.try_minimum_balance(args.delegated_account.data_len())? and
returning DlpError::InsufficientRent if below the floor, leaving the existing
transfer (system::Transfer::invoke), lamports_decrement_by, and
lamports_increment_by calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6da4fe80-7623-4d84-b2cb-0159e3bf25da
📒 Files selected for processing (10)
dlp-api/src/error.rsdlp-api/src/instruction_builder/commit_finalize.rsdlp-api/src/instruction_builder/commit_finalize_from_buffer.rsdlp-api/src/requires.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_commit_finalize.rs`:
- Around line 321-322: Replace the hardcoded literal 1_000 in the assertion with
a computed expected debit derived from the test inputs: compute expected_debit =
commit_lamports - initial_lamports and assert that before_validator_lamports -
after_validator_lamports is greater than expected_debit (use assert_gt! or
equivalent). Update the assertion that currently references
before_validator_lamports and after_validator_lamports to compare against this
expected_debit so the test stays correct if the fixture values change.
🪄 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: 89b77128-0df9-4a3e-95d0-4b2c78374d15
📒 Files selected for processing (2)
src/lib.rstests/test_commit_finalize.rs
Contributor
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/internal/commit_finalize_internal.rs (1)
68-78:⚠️ Potential issue | 🟠 MajorDefer
validator_fees_vaultwritability to theLessbranch.Line 77 makes
validator_fees_vaultmandatory writable before you know whether that branch will run, but onlyOrdering::Lessmutates it. That now rejectsGreater/Equalcalls with a readonly vault and needlessly widens the write set on a per-validator PDA.🔧 Suggested fix
require_initialized_pda_fast!( args.validator_fees_vault, &[ pda::VALIDATOR_FEES_VAULT_TAG, args.validator.address().as_ref(), &[args.bumps.validator_fees_vault], crate::fast::ID.as_ref(), PDA_MARKER ], - true + false ); @@ std::cmp::Ordering::Less => { + require!( + args.validator_fees_vault.is_writable(), + ProgramError::Immutable + ); let amount = delegation_record.lamports - args.commit_lamports; args.delegated_account.lamports_decrement_by(amount)?; args.validator_fees_vault.lamports_increment_by(amount)?;Also applies to: 132-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 68 - 78, The require_initialized_pda_fast invocation currently marks args.validator_fees_vault as writable unconditionally; change it so the initial check uses writable=false (or omits the true flag) and only call require_initialized_pda_fast with writable=true inside the Ordering::Less branch where validator_fees_vault is actually mutated (also apply the same change for the other occurrence around lines 132-136). Locate the require_initialized_pda_fast calls referencing args.validator_fees_vault and move or duplicate the writable=true invocation into the Less branch so Greater/Equal paths do not require a writable PDA.
🤖 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/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 68-78: The require_initialized_pda_fast invocation currently marks
args.validator_fees_vault as writable unconditionally; change it so the initial
check uses writable=false (or omits the true flag) and only call
require_initialized_pda_fast with writable=true inside the Ordering::Less branch
where validator_fees_vault is actually mutated (also apply the same change for
the other occurrence around lines 132-136). Locate the
require_initialized_pda_fast calls referencing args.validator_fees_vault and
move or duplicate the writable=true invocation into the Less branch so
Greater/Equal paths do not require a writable PDA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: db5762fe-92f0-4844-872f-fad3fa3fddda
📒 Files selected for processing (1)
src/processor/fast/internal/commit_finalize_internal.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/processor/fast/finalize.rs`:
- Around line 169-170: The change mistakenly overwrites
delegation_record.lamports with the ER snapshot commit_record.lamports; instead
ensure delegation_record.lamports reflects the actual post-settlement base-chain
balance (the value after calling settle_lamports_balance) rather than the commit
snapshot. Locate the update around delegation_record and commit_record.lamports
in finalize.rs (after settle_lamports_balance/commit_lamports) and replace the
assignment so delegation_record.lamports is set to the real post-settlement
lamports value (e.g., the on-chain/delegated account balance returned or read
after settlement) rather than commit_record.lamports.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 143-154: The code incorrectly persists the ER-observed balance by
assigning delegation_record.lamports = args.commit_lamports; instead you must
persist the actual post-settlement base-chain balance from the delegated
account: set delegation_record.lamports to args.delegated_account.lamports()
(using the correct numeric conversion/typing if needed) so future
reconciliations start from the real on-chain balance; leave the existing
rent-exemption check (check_minimum_balance and require_ge! comparing
args.delegated_account.lamports() to Rent::get()... ) unchanged.
🪄 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: f96ea5d3-35cf-4e3b-831e-5e190233813a
📒 Files selected for processing (3)
src/processor/fast/finalize.rssrc/processor/fast/internal/commit_finalize_internal.rstests/test_lamports_settlement.rs
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
f3e5e5c to
cc8242a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_lamports_settlement.rs`:
- Around line 154-162: Add a brief comment in the test near the second
CommitFinalizeArgs explaining that after the first call to commit_finalize the
delegation_record.lamports is updated to initial_lamports + 100, so the second
call intentionally passes lamports: initial_lamports (i.e., commit_lamports <
delegation_record.lamports) to demonstrate the edge case where the committed
amount is lower than the on-chain delegated balance and therefore does not
change the stored balance (no-op / settlement logic not triggered); reference
the involved symbols CommitFinalizeArgs, commit_finalize, and
delegation_record.lamports so readers can quickly connect the comment to the
code paths being tested.
- Around line 71-183: Add assertions that read and verify the on-chain
delegation record and validator fees vault after each commit: after each
commit_finalize (calls to dlp_api::instruction_builder::commit_finalize) fetch
the PDA returned by
delegation_record_pda_from_delegated_account(&delegated.pubkey()) and assert its
delegation_record.lamports equals delegated_account.lamports; also fetch the
vault via validator_fees_vault_pda_from_validator(&validator.pubkey()) and
assert its lamports remain unchanged for the increase path. Perform these checks
both after the first commit (commit_id = 1) and after the second commit
(commit_id = 2).
🪄 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: 1ec552b2-c9fb-4f0b-ba5f-0b42ddb5fc53
📒 Files selected for processing (1)
tests/test_lamports_settlement.rs
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
cc8242a to
f4e9c2b
Compare
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
6fa2376 to
09e0102
Compare
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.
#131 did not settle lamports. This PR does it properly.
It also fixes this 2-years old bug in
finalize.rs:Also, added two tests for both
CommitFinalizeandCommit+Finalizethat send 4 commit instructions, with different combinations of lamports changes, to ensure that lamports-settling is correct.CodeRabbit comment on #131:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores