Permissioned Domains (XLS-80d) #5149

mtrippled · 2024-09-27T04:16:27Z

Spec: XRPLF/XRPL-Standards#228

High Level Overview of Change

Implements the object, transactions, and tests required by the spec:
XRPLF/XRPL-Standards#228

Context of Change

New feature. Follows existing patterns for adding a new ledger object and related transactions.

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

Requires an amendment.

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-09-27T04:37:21Z

Codecov Report

Attention: Patch coverage is 95.03546% with 7 lines in your changes missing coverage. Please review.

Project coverage is 76.2%. Comparing base (1fbf8da) to head (bb5d603).
Report is 8 commits behind head on develop.

Files with missing lines	Patch %	Lines
...c/xrpld/app/tx/detail/PermissionedDomainDelete.cpp	87.1%	4 Missing ⚠️
src/xrpld/app/tx/detail/PermissionedDomainSet.cpp	96.3%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #5149    +/-   ##
========================================
  Coverage     76.2%   76.2%            
========================================
  Files          760     764     +4     
  Lines        61568   61709   +141     
  Branches      8126    8126            
========================================
+ Hits         46909   47033   +124     
- Misses       14659   14676    +17

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
include/xrpl/protocol/Indexes.h	`100.0% <ø> (ø)`
include/xrpl/protocol/SField.h	`100.0% <ø> (ø)`
src/libxrpl/protocol/Feature.cpp	`94.6% <ø> (ø)`
src/libxrpl/protocol/Indexes.cpp	`97.7% <100.0%> (+0.1%)`	⬆️
src/libxrpl/protocol/LedgerFormats.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/protocol/SField.cpp	`77.5% <ø> (ø)`
src/libxrpl/protocol/TxFormats.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/InvariantCheck.cpp	`91.5% <100.0%> (+0.4%)`	⬆️
src/xrpld/app/tx/detail/InvariantCheck.h	`100.0% <ø> (ø)`
... and 7 more

... and 7 files with indirect coverage changes

oleks-rip

InnerObjectFormats.cpp should be updated with

add(sfCredential.jsonName.c_str(),
sfCredential.getCode(),
{
    {sfIssuer, soeREQUIRED},
    {sfCredentialType, soeREQUIRED},
});

to check sfAcceptedCredentials array during RPC parsing. This inner object already present in credentials PR(#5103) so I will not treat this as an issue

src/test/app/PermissionedDomains_test.cpp

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

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

src/test/app/PermissionedDomains_test.cpp

mtrippled · 2024-10-01T22:27:33Z

InnerObjectFormats.cpp should be updated with
add(sfCredential.jsonName.c_str(),
sfCredential.getCode(),
{
    {sfIssuer, soeREQUIRED},
    {sfCredentialType, soeREQUIRED},
});
to check sfAcceptedCredentials array during RPC parsing. This inner object already present in credentials PR(#5103) so I will not treat this as an issue

Credentials should be merged before this one. There likely will be merge conflicts from some of the new field names having different values, but they should be simple to resolve.

Also, test sorting of permissions.

src/test/jtx/impl/PermissionedDomains.cpp

failing the transaction. Conforms to Credentials spec.

src/test/jtx/impl/PermissionedDomains.cpp

dangell7

Partial Review

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

dangell7 · 2024-10-11T12:54:25Z

src/xrpld/rpc/handlers/AccountObjects.cpp

@@ -222,7 +222,8 @@ doAccountObjects(RPC::JsonContext& context)
            {jss::xchain_owned_claim_id, ltXCHAIN_OWNED_CLAIM_ID},
            {jss::xchain_owned_create_account_claim_id,
             ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID},
-            {jss::bridge, ltBRIDGE}};
+            {jss::bridge, ltBRIDGE},
+            {jss::permissioned_domain, ltPERMISSIONED_DOMAIN}};


There are other rpc functions that need to be added for account_objects I believe. LedgerEntry. Also you should write some tests to confirm the behavior.

You should also be able to query the ledger entry from the account/seq so add that as well. And then add tests.

I added a flag for account_objects to return type=permissioned_domain, and tested it. Also the ledger_entry call is already tested ok. Please specify what other RPC's you think are necessary. But i think what's there already is sufficient for people to find their PermissionedDomain objects.

There are 2 ledger entry calls. The index and { permissioned_domain: { account: account, seq: seq }} the other should be added and tested unless I'm just missing it.

You tested that the objects are empty, you then lower in the test would create the permissioned_domain and validate it. I'm not the main reviewer so if someone else is fine with you testing account_objects differently than the other objects then thats ok. (All the objects are not even tested there anyways so its prob not a big deal)

I've checked documentation, it seems we also need to extend the RPCs:

ledger_data- relies on chooseLedgerEntryType which is already modified, so we only need to add it to unit tests

ledger_entry - needs a custom section of code inside doLedgerEntry (and a unit test)

Whenever you add a new type of ledger entry, the following methods need to be extended:

account_objects - type field

ledger_data - type field

ledger_entry - new field for looking up the new entry type (typically this can accept either a string ledger entry ID or an object with nested sub-fields as appropriate for the type of entry)

I think the ledger_data and account_objects type filters use shared code as mentioned above.

I don't think there's anything else that always needs to be added/updated, but of course depending on what the amendment does there may always be more. In the case of Permissioned Domains I'm guessing nothing.

Also, I don't think adding a new type of transaction requires any similar work since AFAIK we don't have anything that filters on transaction type or retrieves specific types of transactions specifically.

include/xrpl/protocol/Indexes.h

src/libxrpl/protocol/LedgerFormats.cpp

dangell7 · 2024-10-11T13:08:09Z

src/test/jtx/PermissionedDomains.h

+// Get newly created domain from transaction metadata.
+uint256
+getNewDomain(std::shared_ptr<STObject const> const& meta);
+


nit: Can you check to make sure you're not duplicating any helper functions here? toBlob I have seen before and objectExists seems more agnostic than just for domains. Maybe we could move that and also pass in the ledger entry type? Owner Info is also very agnostic.

I put those things in their own namespace, for this particular project, for this particular set of methods & ledger object. If somebody else wants to use them for something else, they can modify, C&P, put somewhere else, etc.

src/test/jtx/impl/PermissionedDomains.cpp

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

Bronek · 2024-10-15T11:43:08Z

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

+        auto const reserve =
+            ctx_.view().fees().accountReserve((*ownerSle)[sfOwnerCount] + 1);
+        if (balance < reserve)
+            return tecINSUFFICIENT_RESERVE;


thanks for putting this check here 👍

Bronek · 2024-10-15T11:43:59Z

src/test/app/PermissionedDomains_test.cpp

+        BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 1);
+
+        /*
+                env(did::setValid(alice), ter(tecINSUFFICIENT_RESERVE));


is there any purpose to having this commented-out section here ?

vvysokikh1 · 2024-10-15T16:25:02Z

src/test/app/PermissionedDomains_test.cpp

+        env.close();
+
+        // Pay alice almost enough to make the reserve.
+        env(pay(env.master, alice, incReserve + drops(19)));


Please avoid using fee 10 directly, this will break with any fee other than 10.

In this case, this could be written as

auto const baseFee = env.current()->fees().base.drops(); ... env(pay(env.master, alice, incReserve + 2 * baseFee - 1));

vvysokikh1 · 2024-10-15T16:25:16Z

src/test/app/PermissionedDomains_test.cpp

+
+        // Pay alice almost enough to make the reserve.
+        env(pay(env.master, alice, incReserve + drops(19)));
+        BEAST_EXPECT(env.balance(alice) == acctReserve + incReserve + drops(9));


vvysokikh1 · 2024-10-15T16:25:24Z

src/test/app/PermissionedDomains_test.cpp

+        BEAST_EXPECT(pd::ownerInfo(alice, env)["OwnerCount"].asUInt() == 0);
+
+        // Pay alice enough to make the reserve.
+        env(pay(env.master, alice, drops(11)));


please also fix for commented section if it's not going to be removed.

Permissioned Domains: XRPLF/XRPL-Standards#228

9754309

mtrippled force-pushed the permissioned-domains branch from 4ead1c5 to 9754309 Compare September 27, 2024 04:44

mtrippled requested review from Bronek and oleks-rip September 27, 2024 04:55

Bronek changed the title ~~Permissioned Domains~~ Permissioned Domains (XLS-80d) Sep 27, 2024

oleks-rip reviewed Sep 27, 2024

View reviewed changes

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

oleks-rip reviewed Sep 27, 2024

View reviewed changes

src/test/app/PermissionedDomains_test.cpp Show resolved Hide resolved

mtrippled force-pushed the permissioned-domains branch from 4e947c6 to d638a48 Compare October 1, 2024 22:23

Review fixes from Olek.

61e785b

mtrippled force-pushed the permissioned-domains branch from d638a48 to 61e785b Compare October 1, 2024 22:35

Test for 2 types of bad DomainIDs in PermissionedDomainSet.

70dfad9

Also, test sorting of permissions.

mtrippled force-pushed the permissioned-domains branch from 674b858 to 70dfad9 Compare October 2, 2024 01:28

oleks-rip reviewed Oct 7, 2024

View reviewed changes

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

Silently remove duplicate credentials instead of

e08bef8

failing the transaction. Conforms to Credentials spec.

mtrippled force-pushed the permissioned-domains branch from 49d1890 to e08bef8 Compare October 8, 2024 00:09

oleks-rip reviewed Oct 8, 2024

View reviewed changes

src/test/jtx/impl/PermissionedDomains.cpp Show resolved Hide resolved

oleks-rip approved these changes Oct 8, 2024

View reviewed changes

mvadari mentioned this pull request Oct 9, 2024

XLS-80d: Permissioned Domains XRPLF/XRPL-Standards#234

Open

dangell7 suggested changes Oct 11, 2024

View reviewed changes

Bronek reviewed Oct 11, 2024

View reviewed changes

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

mtrippled force-pushed the permissioned-domains branch from 7587f0e to edd696b Compare October 14, 2024 21:53

Review fixes from Denis.

7070c56

mtrippled force-pushed the permissioned-domains branch from 1f2af22 to 7070c56 Compare October 15, 2024 01:19

Check for reserve only when creating a new object.

bb5d603

Bronek reviewed Oct 15, 2024

View reviewed changes

vvysokikh1 suggested changes Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissioned Domains (XLS-80d) #5149

Permissioned Domains (XLS-80d) #5149

mtrippled commented Sep 27, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

oleks-rip left a comment

mtrippled commented Oct 1, 2024

dangell7 left a comment

dangell7 Oct 11, 2024

dangell7 Oct 11, 2024

mtrippled Oct 15, 2024 •

edited

Loading

dangell7 Oct 15, 2024 •

edited

Loading

Bronek Oct 15, 2024

mDuo13 Oct 15, 2024 •

edited

Loading

dangell7 Oct 11, 2024

mtrippled Oct 15, 2024

Bronek Oct 15, 2024

Bronek Oct 15, 2024

vvysokikh1 Oct 15, 2024

vvysokikh1 Oct 15, 2024

vvysokikh1 Oct 15, 2024

vvysokikh1 Oct 15, 2024

Permissioned Domains (XLS-80d) #5149

Are you sure you want to change the base?

Permissioned Domains (XLS-80d) #5149

Conversation

mtrippled commented Sep 27, 2024

High Level Overview of Change

Context of Change

Type of Change

API Impact

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

oleks-rip left a comment

Choose a reason for hiding this comment

mtrippled commented Oct 1, 2024

dangell7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrippled Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

dangell7 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mDuo13 Oct 15, 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

codecov bot commented Sep 27, 2024 •

edited

Loading

mtrippled Oct 15, 2024 •

edited

Loading

dangell7 Oct 15, 2024 •

edited

Loading

mDuo13 Oct 15, 2024 •

edited

Loading