[Work in Progress] add AccountPermission #5164

yinyiqian1 · 2024-10-25T19:01:42Z

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

This PR includes the following changes:

Added AccountPermissionSet transaction.
to enable an account(delegating account) delegate some permissions to another account(delegated account), so that the delegated account can send transactions on behalf of the delegating account.
Added Account_Permission ledger object
The AccountPermissionSet transaction will create AccountPermission ledger object, with keylet(delegating account, delegated account)
Added Onbehalf of common field.
Permission has two types:
a. transaction level permission
b. granular permission which will be part of a transaction

more details:

introduced account and isDelegated in PreflightContext, PreclaimContext and ApplyContext. The account is the account which is the transaction being operated on.
transactor's member account_ is updated to the account which is the transaction being operated on.
a set of Granular Permissions gpSet is added to ApplyContext. It contains the granular permissions enabled for that transaction. But if the transaction is fully delegated, then even there are granular permissions related to that transaction, the gpSet will be cleared up. To be more specific:
a. isDelegated=true && gpSet not empty: only granular permissions delegated to the transaction
b. isDelegated=true && gpSet is empty: the transaction is fully delegated
only the granular permissions listed in Permissions.cpp are supported, so the unauthorized granular operations and the other parts under these transactions are prohibited under granular permission mode.

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)

codecov · 2024-10-25T19:40:24Z

Codecov Report

Attention: Patch coverage is 47.22892% with 219 lines in your changes missing coverage. Please review.

Project coverage is 77.7%. Comparing base (838978b) to head (7a7fa0c).
Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/AccountPermissionSet.cpp	0.0%	56 Missing ⚠️
src/libxrpl/protocol/Permissions.cpp	0.0%	34 Missing ⚠️
src/xrpld/app/tx/detail/Transactor.cpp	13.9%	31 Missing ⚠️
src/xrpld/rpc/handlers/LedgerEntry.cpp	5.0%	19 Missing ⚠️
src/xrpld/app/tx/detail/SetTrust.cpp	26.3%	14 Missing ⚠️
src/libxrpl/protocol/STParsedJSON.cpp	23.5%	13 Missing ⚠️
src/xrpld/app/tx/detail/SetAccount.cpp	50.0%	13 Missing ⚠️
src/xrpld/app/tx/detail/Payment.cpp	33.3%	12 Missing ⚠️
src/libxrpl/protocol/Indexes.cpp	0.0%	5 Missing ⚠️
src/xrpld/app/tx/detail/DeleteAccount.cpp	28.6%	5 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5164     +/-   ##
=========================================
- Coverage     77.9%   77.7%   -0.2%     
=========================================
  Files          782     787      +5     
  Lines        66621   66927    +306     
  Branches      8161    8270    +109     
=========================================
+ Hits         51902   51975     +73     
- Misses       14719   14952    +233

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
include/xrpl/protocol/Indexes.h	`100.0% <ø> (ø)`
include/xrpl/protocol/detail/ledger_entries.macro	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/InnerObjectFormats.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/TxFormats.cpp	`100.0% <ø> (ø)`
src/xrpld/app/tx/applySteps.h	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/AMMBid.cpp	`90.4% <100.0%> (ø)`
src/xrpld/app/tx/detail/AMMClawback.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/AMMCreate.cpp	`90.5% <100.0%> (ø)`
... and 50 more

... and 14 files with indirect coverage changes

shawnxie999 · 2024-10-29T13:57:01Z

include/xrpl/protocol/Permissions.h

+    gpMPTokenIssuanceUnlock = 65548,
+};
+
+class Permission


should probably delete copy and copy assignment constructors

does this still needs to be done?

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

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

gregtatcam · 2024-11-19T13:50:50Z

PR description should be added with a link to technical specifications if applicable.

yinyiqian1 · 2024-11-19T14:57:39Z

PR description should be added with a link to technical specifications if applicable.

just added

yinyiqian1 · 2024-11-19T15:49:11Z

FYI: unit test failures are transient, which have passed locally. "ERR:Env Env::close() failed: no response from server"

gregtatcam

I took a pass and left a few comments.
My big concern is how sfAccount is obfuscated by sfOnBehalfOf. sfAccount pretty much should never be used in the transactors with some exceptions. It's easy to miss these kind of errors where sfAccount is used instead of the account included in the context.

gregtatcam · 2024-11-21T15:28:38Z

include/xrpl/protocol/detail/ledger_entries.macro

+LEDGER_ENTRY(ltACCOUNT_PERMISSION, 0x0082, AccountPermission, ({
+    {sfAccount,              soeREQUIRED},
+    {sfAuthorize,            soeREQUIRED},
+    {sfPermissions,          soeREQUIRED},


Does it make sense to make this optional since it could be empty?

Since being empty means deleting all the permissions, can we keep it as required? So that the user won't delete the permissions by accident when they mistakenly leave out the Permissions field. Giving an empty field is more clear that they want to delete all the permissions.

This is an on-ledger object, not the transaction. It's empty if there is no permissions so it could be optional. Can save a bit of space this way, right?

yes. make sense.

gregtatcam · 2024-11-21T15:29:44Z

include/xrpl/protocol/detail/transactions.macro

+/** This transaction type delegates authorized account specified permissions */
+TRANSACTION(ttACCOUNT_PERMISSION_SET, 61, AccountPermissionSet, ({
+    {sfAuthorize, soeREQUIRED},
+    {sfPermissions, soeREQUIRED},


Does it make sense to make it optional since it could be empty?

gregtatcam · 2024-11-21T15:36:46Z

include/xrpl/protocol/Permissions.h

+ */
+
+enum GranularPermissionType : std::uint32_t {
+    gpTrustlineAuthorize = 65537,


Is gp prefix necessary? Can we just have TrustlineAuthorize, etc?

gp is to indicate it is a granular permission set's member. I'm ok with either way.

just deleted gp

gregtatcam · 2024-11-21T15:46:13Z

src/libxrpl/protocol/Indexes.cpp

@@ -77,6 +77,7 @@ enum class LedgerNameSpace : std::uint16_t {
    MPTOKEN_ISSUANCE = '~',
    MPTOKEN = 't',
    CREDENTIAL = 'D',
+    ACCOUNT_PERMISSION = 'P',


This value is already used by DEPOSIT_PREAUTH_CREDENTIALS

gregtatcam · 2024-11-21T16:47:59Z

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

+    {
+        auto permissionObj = dynamic_cast<STObject const*>(&permission);
+
+        if (!permissionObj || (permissionObj->getFName() != sfPermission))


Is this needed? If the array is invalid it should fail in the array parsing. And if the name is not Permission then it should fail also before it gets to the transactor. Should have a unit-test fo this.

gregtatcam · 2024-11-21T20:21:44Z

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

@@ -197,7 +197,7 @@ preclaimHelper<MPTIssue>(
 TER
 Clawback::preclaim(PreclaimContext const& ctx)
 {
-    AccountID const issuer = ctx.tx[sfAccount];
+    AccountID const issuer = ctx.account;


Should be const&. Please check everywhere where an alias variable is used for ctx.account. It should be AccountID const&.

gregtatcam · 2024-11-21T20:29:18Z

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

+    if (ctx_.isDelegated && !ctx_.gpSet.empty())
+    {
+        // if gpSet is not empty, granular delegation is happening.
+        if (bLock && ctx_.gpSet.find(gpMPTokenIssuanceLock) == ctx_.gpSet.end())


Can simplify here and below.

if (bLock && !ctx.gpSet.contains(gpMPTokenIssuanceLock))

gregtatcam · 2024-11-21T20:33:26Z

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

+        auto const amountIssue = dstAmount.issue();
+        if (isXRP(amountIssue))
+            return tecNO_AUTH;
+        if (amountIssue.account == account_ &&


Can simplify here and below:

if (amountIssue.account == account_ && ctx_.gpSet.contains(gpPaymentMint))

gregtatcam · 2024-11-21T20:34:48Z

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

+        if (bSetNoRipple || bClearNoRipple || bQualityIn || bQualityOut)
+            return terNO_AUTH;
+        if (bSetAuth &&
+            ctx_.gpSet.find(gpTrustlineAuthorize) == ctx_.gpSet.end())


Can simplify here and below:

if (bSetAuth && !ctx_.gpSet.contains(gpTrustlineAuthorize))

gregtatcam · 2024-11-21T20:49:24Z

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

@@ -458,6 +458,7 @@ LedgerEntryTypesMatch::visitEntry(
        switch (after->getType())
        {
            case ltACCOUNT_ROOT:
+            case ltACCOUNT_PERMISSION:


I can't leave a comment on unchanged lines so commenting here for ValidClawback::finalize:924, which is this line:

AccountID const issuer = tx.getAccountID(sfAccount);

The issuer in this case could be either sfAccount or sfOnBehalfOf, right?

This is missed out. I'll also check all the other similar places which is missed out.

@gregtatcam
I also need to make change to AcceptedLedgerTx.cpp, LedgerToJson.cpp, and NetworkOPs.cpp, because they also check the account id when the transaction type is ttOFFER_CREATE. I'm working on this as well

gregtatcam · 2024-12-20T20:17:10Z

include/xrpl/protocol/STTxWr.h

+    {
+    }
+
+    STTx


Should be STTx const&

gregtatcam · 2024-12-20T20:21:40Z

include/xrpl/protocol/STTxWr.h

+    }
+
+    template <class T>
+    typename std::enable_if<


Should use require instead here and below.

gregtatcam · 2024-12-20T21:01:09Z

include/xrpl/protocol/Permissions.h

+    std::unordered_map<GranularPermissionType, TxType> granularTxTypeMap;
+
+public:
+    static Permission const&


They should be also static. There should be only one instance of those maps.

gregtatcam · 2024-12-20T21:03:57Z

include/xrpl/protocol/STTxWr.h

+    {
+        return tx_;
+    }
+


I think it also makes sense to add implicit conversion operator to STTx const&. Then don't need to call tx.getTx() in many cases. Can just pass tx.

And perhaps change getTx() to something else, maybe getSTTx? tx.getTx() doesn't look right.

gregtatcam

The unit-tests are missing for AccountPermissionSet, all delegated transactions, and all granular permissions.

gregtatcam · 2024-12-23T12:50:29Z

include/xrpl/protocol/STTxWr.h

+// delegated by another account by checking if the sfOnBehalfOf field is
+// present. If it is present, we need to return the sfOnBehalfOf field as the
+// account when calling getAccountID(sfAccount) and tx[sfAccount].
+class STTxWr


How about renaming to STTxDelegated?

gregtatcam · 2024-12-23T12:59:10Z

src/libxrpl/protocol/Permissions.cpp

+//==============================================================================
+
+#include <xrpl/protocol/Permissions.h>
+#include <xrpl/protocol/SField.h>


This and next include are not used.

gregtatcam · 2024-12-23T13:07:17Z

src/xrpld/app/ledger/detail/LedgerToJson.cpp

@@ -206,7 +206,9 @@ fillJsonTx(
    if ((fill.options & LedgerFill::ownerFunds) &&
        txn->getTxnType() == ttOFFER_CREATE)
    {
-        auto const account = txn->getAccountID(sfAccount);
+        auto const& account = txn->isFieldPresent(sfOnBehalfOf)


What do you about adding getDelegatingAccountID() to STTx?

can we give it a more precise name like getDelegatingOrOriginalAccountID() or getEffectiveAccountID()?

getEffectiveAccountID() sounds good.

gregtatcam · 2024-12-23T13:16:47Z

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

-    auto pfresult = preflight(app, view.rules(), tx, flags, j);
+    bool const isDelegated = view.rules().enabled(featureAccountPermission) &&
+        tx.isFieldPresent(sfOnBehalfOf);
+    STTxWr txWr(tx, isDelegated);


What do you think if STTxWr is hidden even further? I.e. it can be created on a fly in *Context and *Result types and those type will have STTxWr const data member instead of STTWr const&. STTxWr is a lightweight class and it's not a problem instantiating a few instances. preflight, preclaim, *Context, and *Result signatures don't change then.

gregtatcam · 2024-12-23T13:21:09Z

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

+AccountPermissionSet::preclaim(PreclaimContext const& ctx)
+{
+    auto const account = ctx.view.read(keylet::account(ctx.tx[sfAccount]));
+    if (!account)


Should use exists instead and simplify:

if (!ctx.view.exists(keylet::account(ctx.tx[sfAccount]))

gregtatcam · 2024-12-23T14:18:30Z

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

@@ -126,6 +130,10 @@ preflight1(PreflightContext const& ctx)
        ctx.tx.isFieldPresent(sfAccountTxnID))
        return temINVALID;

+    if (!ctx.rules.enabled(featureAccountPermission) &&


This is already checked at the beginning of the function.

gregtatcam · 2024-12-23T14:24:53Z

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

+    STTx const& tx,
+    std::unordered_set<GranularPermissionType>& permissions)
+{
+    if (!view.read(keylet::account(tx[sfOnBehalfOf])))


Should be exists instead.

Should we check if sfOnBehalfOf is included in tx? checkPermissions is so far only called in delegated but still might make sense to check.

gregtatcam · 2024-12-23T14:28:27Z

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

+        keylet::accountPermission(tx[sfOnBehalfOf], tx[sfAccount]);
+    auto const sle = view.read(accountPermissionKey);
+    if (!sle)
+        return temMALFORMED;  // todo: change error code


Change the error code?

how about tecNO_PERMISSION because the permission does not exist?

gregtatcam · 2024-12-23T14:42:29Z

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

@@ -247,6 +300,21 @@ TER
 Transactor::payFee()
 {
    auto const feePaid = ctx_.tx[sfFee].xrp();
+    if (ctx_.tx.isDelegated())


Why is this needed? We always need the account sender to update the balance. I.e. it's always sfAccount. And if it's not delegated then account_ is sfAccount.

I removed the check here. But I still need to add
if (!ctx_.tx.isDelegated()) mSourceBalance -= feePaid;
to keep mSourceBalance value precise.

gregtatcam · 2024-12-23T14:45:59Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

@@ -834,6 +869,7 @@ doLedgerEntry(RPC::JsonContext& context)
        {jss::index, parseIndex, ltANY},
        {jss::account_root, parseAccountRoot, ltACCOUNT_ROOT},
        // TODO: add amendments
+        {jss::amm, parseAccountPermission, ltACCOUNT_PERMISSION},


This is not amm, is it?

forgot to change when copying :(

yinyiqian1 marked this pull request as draft October 25, 2024 19:01

yinyiqian1 force-pushed the account_permissions branch from 81fcb3f to 34c5065 Compare October 25, 2024 19:20

yinyiqian1 force-pushed the account_permissions branch from 34c5065 to 2e9b7a6 Compare October 25, 2024 19:47

yinyiqian1 requested review from gregtatcam and shawnxie999 October 25, 2024 20:02

shawnxie999 reviewed Oct 29, 2024

View reviewed changes

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

shawnxie999 reviewed Oct 29, 2024

View reviewed changes

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

shawnxie999 reviewed Oct 29, 2024

View reviewed changes

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

shawnxie999 reviewed Oct 29, 2024

View reviewed changes

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

yinyiqian1 force-pushed the account_permissions branch 3 times, most recently from 6101542 to bd7150e Compare November 5, 2024 21:50

yinyiqian1 force-pushed the account_permissions branch 2 times, most recently from 04cf7ad to 5c1d6a3 Compare November 18, 2024 16:24

Add AccountPermissions

6bbe2ca

yinyiqian1 force-pushed the account_permissions branch from 5c1d6a3 to 6bbe2ca Compare November 18, 2024 19:44

shawnxie999 reviewed Nov 18, 2024

View reviewed changes

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

update granular permissions

372cc5c

gregtatcam reviewed Nov 21, 2024

View reviewed changes

yinyiqian1 force-pushed the account_permissions branch from 87bae6a to b41a4cb Compare December 5, 2024 17:17

yinyiqian1 added 2 commits December 5, 2024 18:22

Add STTx Wrapper

9310eae

Merge remote-tracking branch 'origin/develop' into accountpermission

5f5b265

yinyiqian1 force-pushed the account_permissions branch from dc2671a to 5f5b265 Compare December 5, 2024 23:43

Modify STTxWr

15506e5

gregtatcam reviewed Dec 20, 2024

View reviewed changes

gregtatcam reviewed Dec 23, 2024

View reviewed changes

+                  {
+                  }
+                  STTx

[Work in Progress] add AccountPermission #5164

Are you sure you want to change the base?

[Work in Progress] add AccountPermission #5164

Conversation

yinyiqian1 commented Oct 25, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam commented Nov 19, 2024 • edited Loading

yinyiqian1 commented Nov 19, 2024

yinyiqian1 commented Nov 19, 2024

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yinyiqian1 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yinyiqian1 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yinyiqian1 commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 25, 2024 •

edited

Loading

gregtatcam commented Nov 19, 2024 •

edited

Loading

yinyiqian1 Dec 2, 2024 •

edited

Loading

yinyiqian1 Jan 6, 2025 •

edited

Loading