Skip to content

fix: Bidirection Object Storage for delegation#6681

Open
Kassaking7 wants to merge 5 commits intoXRPLF:developfrom
Kassaking7:DEFI-568
Open

fix: Bidirection Object Storage for delegation#6681
Kassaking7 wants to merge 5 commits intoXRPLF:developfrom
Kassaking7:DEFI-568

Conversation

@Kassaking7
Copy link
Copy Markdown
Collaborator

@Kassaking7 Kassaking7 commented Mar 27, 2026

High Level Overview of Change

When account A (the delegatee) is deleted, any Delegate ledger objects in other accounts' owner directories that reference A via sfAuthorize were not cleaned up, leaving dangling objects on-ledger.

This PR fixes the issue by storing the Delegate object in both the delegating account's (B) and the authorized account's (A) owner directories, mirroring the pattern used by EscrowCreate. When either party deletes their account, AccountDelete walks their owner directory, finds the ltDELEGATE entry, and deleteDelegate cleans up both sides.

Context of Change

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)

I conducted a local test using standalone mode. The core commands I had:

% curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{
    "method": "account_objects",
    "params": [{"account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM"}]
  }' | python3 -m json.tool
{
    "result": {
        "account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM",
        "account_objects": [
            {
                "Account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM",
                "Authorize": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua",
                "DestinationNode": "0",
                "Flags": 0,
                "LedgerEntryType": "Delegate",
                "OwnerNode": "0",
                "Permissions": [
                    {
                        "Permission": {
                            "PermissionValue": "Payment"
                        }
                    }
                ],
                "PreviousTxnID": "ED79B04396CA6D34C74483B8FC5FD14BAFB3F9A8321609A14E2352EB9362FDA6",
                "PreviousTxnLgrSeq": 6,
                "index": "07349519B9ED9599FF6C84C206735741C5C24F4C456AB30EFAAC69C42FC45D77"
            }
        ],
        "ledger_current_index": 7,
        "status": "success",
        "validated": false
    }
}


%curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{
    "method": "account_objects",
    "params": [{"account": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua"}]
  }' | python3 -m json.tool
{
    "result": {
        "account": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua",
        "account_objects": [
            {
                "Account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM",
                "Authorize": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua",
                "DestinationNode": "0",
                "Flags": 0,
                "LedgerEntryType": "Delegate",
                "OwnerNode": "0",
                "Permissions": [
                    {
                        "Permission": {
                            "PermissionValue": "Payment"
                        }
                    }
                ],
                "PreviousTxnID": "ED79B04396CA6D34C74483B8FC5FD14BAFB3F9A8321609A14E2352EB9362FDA6",
                "PreviousTxnLgrSeq": 6,
                "index": "07349519B9ED9599FF6C84C206735741C5C24F4C456AB30EFAAC69C42FC45D77"
            }
        ],
        "ledger_current_index": 7,
        "status": "success",
        "validated": false
    }
}

%for i in $(seq 1 256); do
  curl -s -X POST http://127.0.0.1:5005 \
    -H 'Content-Type: application/json' \
    -d '{"method":"ledger_accept","params":[{}]}' > /dev/null
done
echo "done"
done

%curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{
    "method": "submit",
    "params": [{
      "secret": "ssT5qnHJRAFZ6aRK1By3AfkD64Jqn",
      "tx_json": {
        "TransactionType": "AccountDelete",
        "Account": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua",
        "Destination": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM",
        "Fee": "2000000",
        "Sequence": 5
      }
    }]
  }' | python3 -m json.tool | grep engine_result
        "engine_result": "tesSUCCESS",
        "engine_result_code": 0,
        "engine_result_message": "The transaction was applied. Only final in a validated ledger.",

% curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{"method":"ledger_accept","params":[{}]}' | python3 -m json.tool | grep ledger_current
        "ledger_current_index": 264,

% curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{
    "method": "account_objects",
    "params": [{"account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM"}]
  }' | python3 -m json.tool
{
    "result": {
        "account": "rGkPsFGDkKprjswvPjtqHVt4VwvRRbz7nM",
        "account_objects": [],
        "ledger_current_index": 264,
        "status": "success",
        "validated": false
    }
}
% curl -s -X POST http://127.0.0.1:5005 \
  -H 'Content-Type: application/json' \
  -d '{
    "method": "account_info",
    "params": [{"account": "rK27ECzvntUQPQRnpQfT4ByMKpYdPYktua"}]
  }' | python3 -m json.tool | grep -E "error|Sequence"
        "error": "actNotFound",
        "error_code": 19,
        "error_message": "Account not found.",
        "status": "error",

We can see that now we don't have delegate object for delegator after delegatee's account got deleted.

Comment thread src/libxrpl/tx/transactors/delegate/DelegateSet.cpp Outdated
Comment thread src/libxrpl/tx/transactors/delegate/DelegateSet.cpp Outdated
Comment thread src/test/app/Delegate_test.cpp
@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

⚠️ This PR contains unsigned commits. To get your PR merged, please sign them. ⚠️

If only the most recent commit is unsigned, you can run:

  1. Amend the commit: git commit --amend --no-edit -n -S
  2. Overwrite the commit: git push --force-with-lease

If multiple commits are unsigned, you can run:

  1. Go into interactive rebase mode: git rebase --interactive HEAD~<NUM_OF_COMMITS>,
    where NUM_OF_COMMITS is the number of most recent commits that will be available
    to edit.
  2. Change "pick" to "edit" for the commits you need to sign, and then save and exit.
  3. For each commit, run: git commit --amend --no-edit -n -S
  4. Continue the rebase: git rebase --continue
  5. Overwrite the commit(s): git push --force-with-lease

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.
See use 1Password to sign your commits.

@github-actions
Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

Comment thread src/test/app/Delegate_test.cpp Outdated
@Kassaking7 Kassaking7 force-pushed the DEFI-568 branch 2 times, most recently from 98f7c62 to ac6b4dd Compare April 6, 2026 16:55
Copy link
Copy Markdown
Collaborator

@yinyiqian1 yinyiqian1 left a comment

Choose a reason for hiding this comment

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

LGTM

@yinyiqian1 yinyiqian1 added the Needs additional review PR requires at least one more code review approval before it can be merged label Apr 6, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.6%. Comparing base (653a383) to head (13770c1).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6681   +/-   ##
=======================================
  Coverage     81.6%   81.6%           
=======================================
  Files         1010    1010           
  Lines        75992   76009   +17     
  Branches      7601    7599    -2     
=======================================
+ Hits         62013   62032   +19     
+ Misses       13979   13977    -2     
Files with missing lines Coverage Δ
include/xrpl/protocol/detail/ledger_entries.macro 100.0% <ø> (ø)
...de/xrpl/protocol_autogen/ledger_entries/Delegate.h 100.0% <100.0%> (ø)
include/xrpl/tx/transactors/delegate/DelegateSet.h 100.0% <ø> (ø)
...c/libxrpl/tx/transactors/account/AccountDelete.cpp 96.3% <100.0%> (ø)
...rc/libxrpl/tx/transactors/delegate/DelegateSet.cpp 100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

@Kassaking7 Kassaking7 requested a review from shawnxie999 April 10, 2026 15:14
Comment thread src/test/app/Delegate_test.cpp Outdated
Copy link
Copy Markdown
Collaborator

@PeterChen13579 PeterChen13579 left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to implement the suggestion I have, if you want. I'm okay either way

@Kassaking7
Copy link
Copy Markdown
Collaborator Author

Kassaking7 commented Apr 13, 2026

Hi @bthomee , this PR is ready to merge. Thanks!

@Kassaking7 Kassaking7 added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Needs additional review PR requires at least one more code review approval before it can be merged labels Apr 13, 2026
@ximinez ximinez requested a review from Copilot April 13, 2026 22:53
Copy link
Copy Markdown
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Unless I'm massively missing something, these changes need to be amendment gated.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes cleanup of ltDELEGATE ledger entries when either the delegator or authorized (delegatee) account is deleted by making Delegate objects discoverable from both parties’ owner directories (mirroring the sfOwnerNode/sfDestinationNode pattern used by other bi-directional objects like Escrow).

Changes:

  • Store ltDELEGATE in both the delegator’s and authorized account’s owner directories and persist the authorized-side directory page in sfDestinationNode.
  • Update DelegateSet::deleteDelegate to remove the directory entry from both sides (and keep owner-count accounting on the delegator only).
  • Extend protocol-autogen schema/wrappers and add/expand unit tests to validate the new optional field and AccountDelete behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/libxrpl/tx/transactors/delegate/DelegateSet.cpp Inserts Delegate into both owner dirs and deletes from both using sfDestinationNode.
include/xrpl/tx/transactors/delegate/DelegateSet.h Updates deleteDelegate signature to no longer require an explicit account parameter.
src/libxrpl/tx/transactors/account/AccountDelete.cpp Uses updated deleteDelegate signature when sweeping ltDELEGATE from owner dirs.
include/xrpl/protocol/detail/ledger_entries.macro Adds sfDestinationNode as an optional field of ltDELEGATE.
include/xrpl/protocol_autogen/ledger_entries/Delegate.h Adds wrapper getter/has-check and builder setter for sfDestinationNode.
src/tests/libxrpl/protocol_autogen/ledger_entries/DelegateTests.cpp Adds coverage for sfDestinationNode round-trip and absence when unset.
src/test/app/Delegate_test.cpp Adds AccountDelete scenarios validating cleanup from both directories and reserve recovery.
Comments suppressed due to low confidence (1)

src/libxrpl/tx/transactors/delegate/DelegateSet.cpp:78

  • When updating an existing Delegate (sle already present) you only update sfPermissions. If this ledger entry was created by older code it may lack sfDestinationNode and may not be present in the authorized account’s owner directory, so deleting the authorized account would still leave a dangling Delegate in the delegator’s directory. Consider backfilling here: if sfDestinationNode is absent, dirInsert into keylet::ownerDir(authAccount), set sfDestinationNode, and update the SLE so the bidirectional cleanup works for pre-existing entries too.
        sle->setFieldArray(sfPermissions, permissions);
        ctx_.view().update(sle);
        return tesSUCCESS;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/xrpl/protocol/detail/ledger_entries.macro
Comment thread src/libxrpl/tx/transactors/account/AccountDelete.cpp Outdated
@yinyiqian1
Copy link
Copy Markdown
Collaborator

Unless I'm massively missing something, these changes need to be amendment gated.

This change is in DelegateSet transaction, its amendment PermissionDelegationV1_1, is not enabled yet.

@ximinez
Copy link
Copy Markdown
Collaborator

ximinez commented Apr 14, 2026

Unless I'm massively missing something, these changes need to be amendment gated.

This change is in DelegateSet transaction, its amendment PermissionDelegationV1_1, is not enabled yet.

That's what I missed. Thanks!

LMK when the Copilot comments are addressed, and I'll merge.

@ximinez ximinez dismissed their stale review April 14, 2026 01:27

Amendment not needed

@Kassaking7 Kassaking7 requested a review from yinyiqian1 April 16, 2026 16:54
@Kassaking7
Copy link
Copy Markdown
Collaborator Author

Unless I'm massively missing something, these changes need to be amendment gated.

This change is in DelegateSet transaction, its amendment PermissionDelegationV1_1, is not enabled yet.

That's what I missed. Thanks!

LMK when the Copilot comments are addressed, and I'll merge.

All comments addressed. Ready to merge. Thanks!

@bthomee bthomee added Needs additional review PR requires at least one more code review approval before it can be merged and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Apr 17, 2026
@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

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

Labels

Needs additional review PR requires at least one more code review approval before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants