Skip to content

feat: Add invariant checks and invariant check tests for LoanAccept#7822

Open
a1q123456 wants to merge 1 commit into
a1q123456/split-loan-set-and-loan-accept-tempfrom
a1q123456/split-loan-set-and-loan-accept
Open

feat: Add invariant checks and invariant check tests for LoanAccept#7822
a1q123456 wants to merge 1 commit into
a1q123456/split-loan-set-and-loan-accept-tempfrom
a1q123456/split-loan-set-and-loan-accept

Conversation

@a1q123456

@a1q123456 a1q123456 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR adds invariant checks and invariant check tests for LoanAccept with all related fields, flags, and a LoanAccept transactor that does nothing.

Context of Change

This PR defines what invariant checks should be performed before the actual development starts to ensure everything will work properly.

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@a1q123456 a1q123456 changed the title A1q123456/split loan set and loan accept feat: Split LoanSet and LoanAccept Jul 17, 2026
@a1q123456 a1q123456 changed the title feat: Split LoanSet and LoanAccept feat: Add invariant checks and invariant check tests for LoanAccept Jul 17, 2026

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it

Review by Claude Sonnet 4.6 · Prompt: V15

@a1q123456
a1q123456 changed the base branch from a1q123456/split-loan-set-and-loan-accept-implementation to a1q123456/split-loan-set-and-loan-accept-temp July 17, 2026 00:07
// fee to the broker owner).
auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId);
if (!maybeVaultDeltaAssets)
if (!maybeVaultDeltaAssets && !isPendingLoan)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Severity: HIGH

When isPendingLoan is true and the vault balance is unchanged, maybeVaultDeltaAssets is nullopt but the guard no longer returns early. The subsequent dereference on line 1140 (*maybeVaultDeltaAssets) and line 1149 (maybeVaultDeltaAssets->delta) is undefined behavior—crashing the node or potentially corrupting memory. Compare with ttLOAN_DELETE at line 1560 which correctly uses value_or().
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Follow the safe-access pattern already used at lines 1227-1229 and 1559-1561 in this file: after the existing early-return guard, add a value_or() call to safely unwrap maybeVaultDeltaAssets, and then use the unwrapped value instead of direct dereferences.

Specifically:

  1. After the closing brace of the guard block (after line 1137), insert:
auto const vaultDelta = maybeVaultDeltaAssets.value_or(
    DeltaInfo{.delta = kZero, .scale = scale(afterVault.assetsTotal, vaultAsset)});
  1. On line 1140, change computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()) to computeVaultMinScale(vaultDelta, view.rules()).
  2. On line 1149, change maybeVaultDeltaAssets->delta to vaultDelta.delta.

This mirrors exactly how ttLOAN_DELETE (line 1560) and ttLOAN_IMPAIR/ttLOAN_UNIMPAIR (line 1228) handle the same optional safely.

// finalise a loan whose StartDate is still in the future.
if (lendingV11Enabled && txType == ttLOAN_ACCEPT)
{
if (after->getAccountID(sfBorrower) != tx.getAccountID(sfAccount))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: LOW

The Borrower authorization check reads from after (post-modification state) instead of before (pre-modification state). A buggy or malicious transactor that overwrites sfBorrower to match the submitter during execution would bypass this invariant, defeating its purpose as a last-line-of-defense authorization check.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Change after->getAccountID(sfBorrower) to before->getAccountID(sfBorrower) on line 132. The invariant check should read the sfBorrower field from the pre-modification state (before) rather than the post-modification state (after). This ensures that even if a buggy transactor overwrites sfBorrower during execution to match the submitter's account, the invariant will still catch the unauthorized access by comparing against the original, unmodified value. The before variable is already available in scope (the code is inside the if (before) block starting at line 77).

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
if (after->getAccountID(sfBorrower) != tx.getAccountID(sfAccount))
if (before->getAccountID(sfBorrower) != tx.getAccountID(sfAccount))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant