Support PermissionDelegation Amendment XLS-74d XLS-75d #5354
Conversation
yinyiqian1
force-pushed
the
account_permission_new
branch
2 times, most recently
from
70a6362 to
5b262d1
Compare
yinyiqian1
changed the title
yinyiqian1
force-pushed
the
account_permission_new
branch
5 times, most recently
from
c8bc5f6 to
e8e74f1
Compare
| static_cast<std::uint32_t>( | ||
| TxFormats::getInstance().findTypeByName( | ||
| strValue) + | ||
| 1)); |
There was a problem hiding this comment.
I am not a fan of modifying the transaction type with a hard coded magic number.
What is the problem with '0'? I haven't seen anything yet that makes '0' special or illegal. If not, then just use the unmodified value.
If there is a problem with '0', then use a new (static) modifier function in Permissions. e.g. TypeToPermission(TxType type). All it needs to do is add 1, but this has the advantage of making it clear what's going on and why.
There was a problem hiding this comment.
@mvadari can we avoid adding 1? Since 0 is not used anyway
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5354 +/- ##
=========================================
+ Coverage 78.1% 78.2% +0.1%
=========================================
Files 790 795 +5
Lines 67911 68340 +429
Branches 8234 8254 +20
=========================================
+ Hits 53025 53429 +404
- Misses 14886 14911 +25
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
yinyiqian1
force-pushed
the
account_permission_new
branch
from
e8e74f1 to
b1f0387
Compare
yinyiqian1
force-pushed
the
account_permission_new
branch
from
b1f0387 to
4495956
Compare
yinyiqian1
force-pushed
the
account_permission_new
branch
from
4495956 to
49ca2b8
Compare
yinyiqian1
changed the title
|
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. |
| if (value == ttSIGNER_LIST_SET || value == ttREGULAR_KEY_SET || | ||
| value == ttACCOUNT_SET || value == ttDELEGATE_SET || | ||
| value == ttACCOUNT_DELETE) | ||
| return true; |
There was a problem hiding this comment.
Agreed (though difficult to make it happen without waiting for one of the two PRs to merge first)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
yinyiqian1
force-pushed
the
account_permission_new
branch
from
7e3e045 to
4e62a34
Compare
yinyiqian1
force-pushed
the
account_permission_new
branch
from
daa6f19 to
ce15383
Compare
|
@yinyiqian1 Deserialization of STUint32 to JSON is missing in STInteger.cpp. |
updated |
This is documented in our contributing guidelines. |
yinyiqian1
changed the title
| @@ -41,6 +41,9 @@ class Delegate_test : public beast::unit_test::suite | |||
| // can not set Delegate when feature disabled | |||
| env(delegate_set::delegateSet(gw, alice, {"Payment"}), | |||
| ter(temDISABLED)); | |||
|
|
|||
| // can not send delegating transaction when feture disabled | |||
There was a problem hiding this comment.
| // can not send delegating transaction when feture disabled | |
| // can not send delegating transaction when feature disabled |
| if (ctx_.tx.isFieldPresent(sfDelegate)) | ||
| { | ||
| // Delegated transactions are paid by the delegated account. | ||
| auto const delegate = ctx_.tx.getAccountID(sfDelegate); |
There was a problem hiding this comment.
might as well add a check
if (account_ == delegate)
Throw<std::logic_error>("Account is same as delegated account");
There was a problem hiding this comment.
this will be unreachable. Because in Transactor::checkPermission, sle can never be found when account=delegate.
There was a problem hiding this comment.
it's expected to be unreachable. altho not necessary, it'd be good to add defensive checks
There was a problem hiding this comment.
if added at or before checkPermission, can we leave it out here
There was a problem hiding this comment.
Never throw on user input, particularly transaction data, unless it's unavoidable. There are mechanisms already in place to check and handle invalid user data. In this case, the standard behavior is to return tefINTERNAL or tecINTERNAL if a condition that should be impossible is encountered. Since this is in the function that pays the fee, tefINTERNAL is more appropriate.
There was a problem hiding this comment.
if (account_ == delegate)
Also, if you haven't already, be sure to write a unit test covering this specific case.
There was a problem hiding this comment.
there's another comment discussing moving this check before check before result = T::checkPermission(ctx.view, ctx.tx);
So the code in applySteps.cpp invoke_preclaim would be:
result = T::checkFee(ctx, calculateBaseFee(ctx.view, ctx.tx));
if (result != tesSUCCESS)
return result;
if (ctx.tx.isFieldPresent(sfDelegate) && *ctx.tx[~sfDelegate] == ctx.tx[sfAccount])
return tefINTERNAL;
result = T::checkPermission(ctx.view, ctx.tx);
if (result != tesSUCCESS)
return result;
...
or maybe add it one level up in applySteps.cpp's preclaim or even earlier in preflight?
There was a problem hiding this comment.
Yeah, preclaim is supposed to be limited to checks that need the ledger. Since you're only looking at the transaction fields, the check should be in preflight. Specifically, preflight1, which would return a tem result indicating the transaction is malformed. Probably temBAD_SIGNER.
| @@ -190,6 +195,22 @@ Transactor::Transactor(ApplyContext& ctx) | |||
| { | |||
| } | |||
|
|
|||
| TER | |||
There was a problem hiding this comment.
can also add a defensive check in Transactor::Transactor(ApplyContext& ctx) to throw a logic error if delegate exists but sfDelegate and sfAccount are the same
There was a problem hiding this comment.
can we add this check before result = T::checkPermission(ctx.view, ctx.tx);?
If it is added before checkPermission, then I don't need to add it everywhere in checkPermission function
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
PermissionDelegationDelegateSettransaction is added so that a delegating account can give permission to delegated account to send transaction on his behalf.Ledger object
Delegateis created, its keylet is hashed from delegating and delegated account.optional common field
Delegateis added, to indicate that a transaction is a delegating transaction.The following transaction is a delegating transaction,
Delegateaccount is the sender of the transaction and will sign this transaction,Accountis the delegating account,Added a
checkPermissionfunction 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_objectsof 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
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)