Support PermissionDelegation Amendment XLS-74d XLS-75d #5354

yinyiqian1 · 2025-03-19T06:19:33Z

There's a change in the spec, so please refer the new spec:
The PR for spec change: XRPLF/XRPL-Standards#257

Original spec:
XRPLF/XRPL-Standards#217
XRPLF/XRPL-Standards#218

This amendment is called PermissionDelegation

DelegateSet transaction is added so that a delegating account can give permission to delegated account to send transaction on his behalf.

{
    TransactionType: "DelegateSet",
    Account: "rDelegating......",
    Authorize: "rDelegated......",
    Permissions: [{Permission: {PermissionValue: "Payment"}}],
}

Ledger object Delegate is created, its keylet is hashed from delegating and delegated account.
optional common field Delegate is added, to indicate that a transaction is a delegating transaction.
The following transaction is a delegating transaction, Delegate account is the sender of the transaction and will sign this transaction, Account is the delegating account,

{
    Transaction: "Payment",
    Account: "rDelegating......",
    Amount: "1000000000",
    Destination: "r......",
    Delegate: "rDelegated......"
}

Added a checkPermission function to check if the delegated account is authorized to send the transaction. And it can be extended in each sub-transactor to check the granular permissions.
the delegated account pays the fee
can query account_objects of the delegating account to see the permissions he owns.
A delegate transaction can be multi-signed by the delegated account's signer list

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

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)

ximinez

Preliminary partial review. I know you're still working on this, but a few things jumped out at me, and I wanted to bring them to your attention before you go down a problematic path.

src/xrpld/app/tx/detail/Transactor.cpp

src/libxrpl/protocol/STParsedJSON.cpp

src/libxrpl/protocol/Permissions.cpp

src/libxrpl/protocol/STParsedJSON.cpp

src/libxrpl/protocol/Permissions.cpp

include/xrpl/protocol/Permissions.h

src/libxrpl/protocol/Permissions.cpp

src/xrpld/rpc/handlers/LedgerEntry.cpp

codecov · 2025-03-19T16:18:14Z

Codecov Report

Attention: Patch coverage is 99.77578% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (c4308b2) to head (647ae72).
Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/libxrpl/protocol/Permissions.cpp	97.7%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5354     +/-   ##
=========================================
+ Coverage     78.1%   78.2%   +0.1%     
=========================================
  Files          795     800      +5     
  Lines        68598   68969    +371     
  Branches      8278    8280      +2     
=========================================
+ Hits         53593   53961    +368     
- Misses       15005   15008      +3

Files with missing lines	Coverage Δ
include/xrpl/protocol/Indexes.h	`100.0% <ø> (ø)`
include/xrpl/protocol/detail/ledger_entries.macro	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/permissions.macro	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/Indexes.cpp	`98.1% <100.0%> (+<0.1%)`	⬆️
src/libxrpl/protocol/InnerObjectFormats.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/STInteger.cpp	`86.4% <100.0%> (+1.9%)`	⬆️
src/libxrpl/protocol/STParsedJSON.cpp	`70.0% <100.0%> (+0.8%)`	⬆️
src/libxrpl/protocol/TxFormats.cpp	`100.0% <ø> (ø)`
src/xrpld/app/misc/detail/DelegateUtils.cpp	`100.0% <100.0%> (ø)`
... and 18 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp

src/xrpld/app/tx/detail/SetTrust.cpp

include/xrpl/protocol/detail/permissions.macro

src/xrpld/rpc/handlers/LedgerEntry.cpp

shawnxie999 · 2025-03-20T17:08:22Z

Also, I propose that we shouldn't need to squash commits. Commits makes it easier for the reviewers to keep track of the changes, and they will all be squashed in the end anyways. Also, the audit relies on a particular commit hash, so it's best to keep the commit history as authentic as possible.

include/xrpl/protocol/Protocol.h

include/xrpl/protocol/detail/sfields.macro

include/xrpl/protocol/detail/permissions.macro

src/xrpld/app/tx/detail/DelegateSet.cpp

src/xrpld/app/tx/detail/SetAccount.cpp

shawnxie999 · 2025-03-20T18:47:54Z

src/libxrpl/protocol/Permissions.cpp

+    if (value == ttSIGNER_LIST_SET || value == ttREGULAR_KEY_SET ||
+        value == ttACCOUNT_SET || value == ttDELEGATE_SET ||
+        value == ttACCOUNT_DELETE)
+        return true;


I think this list should be updated with batch txn as well when the times comes, since I don't know whether it makes sense to delegate someone else to do a batch. what do you think @mvadari @dangell7

Agreed (though difficult to make it happen without waiting for one of the two PRs to merge first)

I do agree that it should be prohibited but we should wait till batch is merged. Also if ed's comment was implemented it would need to be added in the transactor instead. I like that approach tbh.

@dangell7 ed agrees not to force overridding the checkPermission function. Since it's checked on the DelegateSet level and all transactions need to be authorized by the delegating account through DelegateSet transaction before a delegated account can send delegating transactions

I got some suggestion from the security audit that I should use whitelist for the supported transactions, which is safer. I added enum delegatable {enabled, disabled} and put it in the transactions.macro. These safety sensitive transactions are delegatable::disabled. And the system-generated transaction types are disabled as well. For newly added transactions in the future, developers can choose to disable delegation through transactions.macro

src/xrpld/app/tx/detail/SetTrust.cpp

src/test/app/Delegate_test.cpp

src/xrpld/app/tx/detail/Transactor.cpp

src/test/jtx/impl/delegate.cpp

src/xrpld/app/tx/detail/DelegateSet.cpp

src/xrpld/app/misc/detail/DelegateUtils.cpp

src/xrpld/app/tx/detail/SetTrust.cpp

gregtatcam · 2025-04-10T13:52:04Z

I got compile error when building with unity on. Non unity build compiles fine.

Could you also please fix a couple of unused variable warning as well?

One is in protocol/Permissions.cpp:76. Just add [[maybe_unused]]. Second is app/tx/detail/InvariantCheck.cpp. Can add (void)enforce; for this one.

/Users/user/Work/Projects/rippled-acct-prms/src/test/app/SetRegularKey_test.cpp:64:27: error: reference to 'disabled' is ambiguous
64 | env(regkey(alice, disabled));
| ^
/Users/user/Work/Projects/rippled-acct-prms/build/modules/xrpl.libxrpl.protocol/xrpl/protocol/Permissions.h:50:29: note: candidate found by name lookup is 'ripple::disabled'
50 | enum delegatable { enabled, disabled };
| ^
/Users/user/Work/Projects/rippled-acct-prms/src/test/jtx/tags.h:50:25: note: candidate found by name lookup is 'ripple::test::jtx::disabled'
50 | static disabled_t const disabled;
| ^
In file included from /Users/user/Work/Projects/rippled-acct-prms/build/CMakeFiles/rippled.dir/Unity/unity_21_cxx.cxx:37:
/Users/user/Work/Projects/rippled-acct-prms/src/test/app/SetRegularKey_test.cpp:99:27: error: reference to 'disabled' is ambiguous
99 | env(regkey(alice, disabled));
| ^
/Users/user/Work/Projects/rippled-acct-prms/build/modules/xrpl.libxrpl.protocol/xrpl/protocol/Permissions.h:50:29: note: candidate found by name lookup is 'ripple::disabled'
50 | enum delegatable { enabled, disabled };
| ^
/Users/user/Work/Projects/rippled-acct-prms/src/test/jtx/tags.h:50:25: note: candidate found by name lookup is 'ripple::test::jtx::disabled'
50 | static disabled_t const disabled;
| ^
In file included from /Users/user/Work/Projects/rippled-acct-prms/build/CMakeFiles/rippled.dir/Unity/unity_21_cxx.cxx:37:
/Users/user/Work/Projects/rippled-acct-prms/src/test/app/SetRegularKey_test.cpp:129:27: error: reference to 'disabled' is ambiguous
129 | env(regkey(alice, disabled), ter(tecNO_ALTERNATIVE_KEY));
| ^
/Users/user/Work/Projects/rippled-acct-prms/build/modules/xrpl.libxrpl.protocol/xrpl/protocol/Permissions.h:50:29: note: candidate found by name lookup is 'ripple::disabled'
50 | enum delegatable { enabled, disabled };
| ^
/Users/user/Work/Projects/rippled-acct-prms/src/test/jtx/tags.h:50:25: note: candidate found by name lookup is 'ripple::test::jtx::disabled'
50 | static disabled_t const disabled;
| ^
In file included from /Users/user/Work/Projects/rippled-acct-prms/build/CMakeFiles/rippled.dir/Unity/unity_21_cxx.cxx:37:
/Users/user/Work/Projects/rippled-acct-prms/src/test/app/SetRegularKey_test.cpp:131:27: error: reference to 'disabled' is ambiguous
131 | env(regkey(alice, disabled));
| ^
/Users/user/Work/Projects/rippled-acct-prms/build/modules/xrpl.libxrpl.protocol/xrpl/protocol/Permissions.h:50:29: note: candidate found by name lookup is 'ripple::disabled'
50 | enum delegatable { enabled, disabled };
| ^
/Users/user/Work/Projects/rippled-acct-prms/src/test/jtx/tags.h:50:25: note: candidate found by name lookup is 'ripple::test::jtx::disabled'
50 | static disabled_t const disabled;
| ^
In file included from /Users/user/Work/Projects/rippled-acct-prms/build/CMakeFiles/rippled.dir/Unity/unity_21_cxx.cxx:37:
/Users/user/Work/Projects/rippled-acct-prms/src/test/app/SetRegularKey_test.cpp:243:27: error: reference to 'disabled' is ambiguous
243 | env(regkey(alice, disabled), sig(alie), ticket::use(--ticketSeq));
| ^
/Users/user/Work/Projects/rippled-acct-prms/build/modules/xrpl.libxrpl.protocol/xrpl/protocol/Permissions.h:50:29: note: candidate found by name lookup is 'ripple::disabled'
50 | enum delegatable { enabled, disabled };
| ^
/Users/user/Work/Projects/rippled-acct-prms/src/test/jtx/tags.h:50:25: note: candidate found by name lookup is 'ripple::test::jtx::disabled'
50 | static disabled_t const disabled;

yinyiqian1 · 2025-04-10T15:42:35Z

@gregtatcam

I got compile error when building with unity on. Non unity build compiles fine.

Could you also please fix a couple of unused variable warning as well?

One is in protocol/Permissions.cpp:76. Just add [[maybe_unused]]. Second is app/tx/detail/InvariantCheck.cpp. Can add (void)enforce; for this one.

updated

…_new

gregtatcam

👍 LGTM

I have a few minor comments for your consideration,

include/xrpl/protocol/jss.h

include/xrpl/protocol/Permissions.h

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp

src/xrpld/app/misc/DelegateUtils.h

yinyiqian1 force-pushed the account_permission_new branch 2 times, most recently from 70a6362 to 5b262d1 Compare March 19, 2025 06:37

yinyiqian1 changed the title ~~Support AccountPermission~~ Support AccountPermission XLS-74d XLS-75d Mar 19, 2025

yinyiqian1 force-pushed the account_permission_new branch 5 times, most recently from c8bc5f6 to e8e74f1 Compare March 19, 2025 15:57

ximinez requested changes Mar 19, 2025

View reviewed changes

src/libxrpl/protocol/Permissions.cpp Outdated Show resolved Hide resolved

mvadari reviewed Mar 19, 2025

View reviewed changes

include/xrpl/protocol/Permissions.h Outdated Show resolved Hide resolved

include/xrpl/protocol/Permissions.h Outdated Show resolved Hide resolved

src/libxrpl/protocol/Permissions.cpp Outdated Show resolved Hide resolved

src/xrpld/rpc/handlers/LedgerEntry.cpp Outdated Show resolved Hide resolved

yinyiqian1 mentioned this pull request Mar 19, 2025

Support AccountPermission XLS-74d and XLS-75d #5164

Closed

13 tasks

yinyiqian1 force-pushed the account_permission_new branch from e8e74f1 to b1f0387 Compare March 20, 2025 14:51

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

yinyiqian1 force-pushed the account_permission_new branch from b1f0387 to 4495956 Compare March 20, 2025 15:41

Support AccountPermission

49ca2b8

yinyiqian1 force-pushed the account_permission_new branch from 4495956 to 49ca2b8 Compare March 20, 2025 16:38

yinyiqian1 changed the title ~~Support AccountPermission XLS-74d XLS-75d~~ Support DelegateSet Amendment XLS-74d XLS-75d Mar 20, 2025

yinyiqian1 marked this pull request as ready for review March 20, 2025 16:50

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

src/xrpld/rpc/handlers/LedgerEntry.cpp Outdated Show resolved Hide resolved

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

include/xrpl/protocol/Protocol.h Outdated Show resolved Hide resolved

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

include/xrpl/protocol/detail/sfields.macro Outdated Show resolved Hide resolved

mvadari reviewed Mar 20, 2025

View reviewed changes

include/xrpl/protocol/detail/permissions.macro Outdated Show resolved Hide resolved

mvadari reviewed Mar 20, 2025

View reviewed changes

src/xrpld/app/tx/detail/DelegateSet.cpp Outdated Show resolved Hide resolved

yinyiqian1 requested a review from gregtatcam March 20, 2025 18:20

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

src/xrpld/app/tx/detail/SetAccount.cpp Outdated Show resolved Hide resolved

shawnxie999 reviewed Mar 20, 2025

View reviewed changes

src/xrpld/app/tx/detail/SetTrust.cpp Outdated Show resolved Hide resolved

shawnxie999 reviewed Mar 21, 2025

View reviewed changes

yinyiqian1 added 2 commits March 24, 2025 11:28

resolve comments and code coverage

5624385

resolve comment and typo

45999cb

shawnxie999 reviewed Mar 24, 2025

View reviewed changes

src/test/app/Delegate_test.cpp Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/Transactor.cpp Show resolved Hide resolved

PeterChen13579 mentioned this pull request Mar 24, 2025

Support Account permissions in Clio XRPLF/clio#1975

Open

shawnxie999 reviewed Mar 24, 2025

View reviewed changes

src/xrpld/app/tx/detail/Transactor.cpp Show resolved Hide resolved

yinyiqian1 added 2 commits March 25, 2025 11:32

resolve comments

7b20a39

remove redundant code

f7f7478

shawnxie999 reviewed Mar 25, 2025

View reviewed changes

src/xrpld/app/tx/detail/Transactor.cpp Outdated Show resolved Hide resolved

yinyiqian1 added 2 commits March 26, 2025 12:41

resolve comment

0d5e148

Merge branch 'develop' into account_permission_new

cb2d2cd

shawnxie999 approved these changes Mar 26, 2025

View reviewed changes

dangell7 reviewed Mar 26, 2025

View reviewed changes

src/test/jtx/impl/delegate.cpp Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/DelegateSet.cpp Outdated Show resolved Hide resolved

dangell7 reviewed Mar 26, 2025

View reviewed changes

src/xrpld/app/misc/detail/DelegateUtils.cpp Show resolved Hide resolved

dangell7 reviewed Mar 26, 2025

View reviewed changes

src/xrpld/app/tx/detail/SetTrust.cpp Show resolved Hide resolved

whitelist delegatable tx and resolve comments

8f48a4e

yinyiqian1 requested review from gregtatcam and ximinez April 3, 2025 19:31

resolve unity build namimg conflicts

c39d061

yinyiqian1 added 4 commits April 10, 2025 12:38

Merge remote-tracking branch 'origin/develop' into account_permission…

237d09f

…_new

Resolve unused warning for Release build

6f4ae53

void(enforce) to avoid warning

d8373e9

fix (void)enforce

3d245c8

gregtatcam approved these changes Apr 10, 2025

View reviewed changes

include/xrpl/protocol/jss.h Outdated Show resolved Hide resolved

include/xrpl/protocol/Permissions.h Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/MPTokenIssuanceSet.cpp Show resolved Hide resolved

src/xrpld/app/misc/DelegateUtils.h Show resolved Hide resolved

yinyiqian1 added 2 commits April 11, 2025 13:43

resolve comments

647ae72

Merge branch 'develop' into account_permission_new

5210035

kennyzlei mentioned this pull request Apr 22, 2025

Support PermissionDelegation (aka Account Permissions) XRPLF/clio#2015

Open

kennyzlei added the Clio Reviewed label Apr 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PermissionDelegation Amendment XLS-74d XLS-75d #5354

Support PermissionDelegation Amendment XLS-74d XLS-75d #5354

yinyiqian1 commented Mar 19, 2025 •

edited

Loading

ximinez left a comment •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

shawnxie999 commented Mar 20, 2025

shawnxie999 Mar 20, 2025

mvadari Mar 20, 2025

dangell7 Mar 20, 2025

yinyiqian1 Mar 24, 2025

yinyiqian1 Apr 3, 2025

gregtatcam commented Apr 10, 2025

yinyiqian1 commented Apr 10, 2025 •

edited

Loading

gregtatcam left a comment

Support PermissionDelegation Amendment XLS-74d XLS-75d #5354

Are you sure you want to change the base?

Support PermissionDelegation Amendment XLS-74d XLS-75d #5354

Conversation

yinyiqian1 commented Mar 19, 2025 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

ximinez left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 19, 2025 • edited Loading

Codecov Report

shawnxie999 commented Mar 20, 2025

shawnxie999 Mar 20, 2025

Choose a reason for hiding this comment

mvadari Mar 20, 2025

Choose a reason for hiding this comment

dangell7 Mar 20, 2025

Choose a reason for hiding this comment

yinyiqian1 Mar 24, 2025

Choose a reason for hiding this comment

yinyiqian1 Apr 3, 2025

Choose a reason for hiding this comment

gregtatcam commented Apr 10, 2025

yinyiqian1 commented Apr 10, 2025 • edited Loading

gregtatcam left a comment

Choose a reason for hiding this comment

yinyiqian1 commented Mar 19, 2025 •

edited

Loading

ximinez left a comment •

edited

Loading

codecov bot commented Mar 19, 2025 •

edited

Loading

yinyiqian1 commented Apr 10, 2025 •

edited

Loading