rippled icon indicating copy to clipboard operation
rippled copied to clipboard

[Work in Progress] add AccountPermission

Open yinyiqian1 opened this issue 1 year ago • 4 comments

spec: https://github.com/XRPLF/XRPL-Standards/discussions/217 https://github.com/XRPLF/XRPL-Standards/discussions/218

This PR includes the following changes:

  1. 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.
  2. Added Account_Permission ledger object The AccountPermissionSet transaction will create AccountPermission ledger object, with keylet(delegating account, delegated account)
  3. Added Onbehalf of common field.
  4. Permission has two types: a. transaction level permission b. granular permission which will be part of a transaction

more details:

  1. introduced account and isDelegated in PreflightContext, PreclaimContext and ApplyContext. The account is the account which is the transaction being operated on.
  2. transactor's member account_ is updated to the account which is the transaction being operated on.
  3. 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
  4. 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)
  • [x] 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)

yinyiqian1 avatar Oct 25 '24 19:10 yinyiqian1

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

gregtatcam avatar Nov 19 '24 13:11 gregtatcam

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

just added

yinyiqian1 avatar Nov 19 '24 14:11 yinyiqian1

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

yinyiqian1 avatar Nov 19 '24 15:11 yinyiqian1

Codecov Report

Attention: Patch coverage is 99.62617% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (2406b28) to head (08fa591). Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
src/libxrpl/protocol/Permissions.cpp 97.2% 1 Missing :warning:
src/xrpld/app/ledger/detail/LedgerToJson.cpp 0.0% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5164     +/-   ##
=========================================
+ Coverage     78.1%   78.2%   +0.2%     
=========================================
  Files          790     794      +4     
  Lines        67908   68348    +440     
  Branches      8227    8223      -4     
=========================================
+ Hits         53028   53477    +449     
+ Misses       14880   14871      -9     
Files with missing lines Coverage Δ
include/xrpl/protocol/Feature.h 100.0% <ø> (ø)
include/xrpl/protocol/Indexes.h 100.0% <ø> (ø)
include/xrpl/protocol/STTx.h 100.0% <ø> (ø)
include/xrpl/protocol/STTxDelegated.h 100.0% <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/Indexes.cpp 98.1% <100.0%> (+<0.1%) :arrow_up:
src/libxrpl/protocol/InnerObjectFormats.cpp 100.0% <100.0%> (ø)
src/libxrpl/protocol/STParsedJSON.cpp 70.6% <100.0%> (+1.4%) :arrow_up:
src/libxrpl/protocol/STTx.cpp 89.9% <100.0%> (+0.4%) :arrow_up:
... and 42 more

... and 8 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 19 '25 17:02 codecov[bot]

Just a heads up I believe this PR would have some conflicts with the Batch feature, specifically with the account sequences.

shawnxie999 avatar Mar 06 '25 17:03 shawnxie999

Added sfDelegateSequence which are optional
to avoid double insertion, when a delegating transaction happens, we have to increment both of the delegating account's sequence and the delegated account's sequence. So sfDelegateSequence is used to track the delegating account's sequence.

Added sfDelegateTicketSequence which are optional
we allow a delegated account to create ticket on behalf of the delegating account of he has the permission. In this case, the delegated account should be able to use the ticket he created on behalf of the delegating account. So sfDelegateTicketSequence is introduced.

Why is this PR not just an extension of "Regular Key Type" functionality? Why not just verify the signature of the delegated account and then process everything as per usual? Or if you want to verify both signatures add sfDelegateSigners and verify the signatures.

dangell7 avatar Mar 13 '25 08:03 dangell7