`DID`: Decentralized identifiers (DIDs) (XLS-40): #4636

mvadari · 2023-07-31T18:05:31Z

High Level Overview of Change

Introduces:

New ledger object: DID
Two new transactions:
- DIDSet, which creates and updates the DID object
- DIDDelete, which deletes the DID object

Context of Change

XLS-40d specification

Earlier discussions/versions:

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)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

Added tests for the new features.

Longest suite times:
   68.2s ripple.app.AMM
   58.4s ripple.app.TheoreticalQuality
   51.6s ripple.app.XChain
   47.9s ripple.tx.NFToken4
   46.2s ripple.tx.NFToken3
   45.9s ripple.tx.NFToken1
   45.7s ripple.tx.NFToken2
   43.5s ripple.tx.NFToken0
   41.8s ripple.tx.NFTokenDir
   39.3s ripple.app.AMMExtended
1119.3s, 227 suites, 1840 cases, 672141 tests total, 0 failures

 % ./rippled --unittest=ripple.app.DID                               
ripple.app.DID Enabled
ripple.app.DID Account reserve
ripple.app.DID Invalid Set
ripple.app.DID Invalid Delete
ripple.app.DID Modify DID with Set
Longest suite times:
    1.0s ripple.app.DID
1.1s, 1 suite, 5 cases, 83 tests total, 0 failures

nbougalis

Left some comments. Biggest issue, IMO, is the lack of any invariant checks. You should consider what types of checks make sense to code up.

src/ripple/app/tx/impl/DID.cpp

nbougalis · 2023-08-01T03:55:05Z

src/ripple/app/tx/impl/DID.cpp

+            if (uri->empty())
+            {
+                sleDID->makeFieldAbsent(sfURI);
+            }
+            else
+            {
+                (*sleDID)[sfURI] = *uri;
+            }


You should be able to just reduce this if/else with:

(*sleDID)[~sfURI] = ctx_.tx[~sfURI];

Notice that you don't check if one or both the URI and DATA fields are being set to the same value that they already hold; so it's possible for someone to "update" the object while making no change. You may want to elide the call to update in that case.

I don't think that reduction works.
What I want is: do nothing if the field doesn't exist in the transaction, delete the field if the field is an empty string, and replace it in any other case.
What my understanding of that line of code is: delete the field if it doesn't exist, and replace it in any other case.

You're right, given what you're trying to do my proposed construct won't work. It's fine as it, but I wouldn't have personally structured this as you have; I'd have included two flags that, if set, indicate the type of value you're setting/clearing. So, if sfFlags & flagURI was set, then the URI field is replaced with the URI field from the transaction.

This was the method that @seelabs and @aanchal4 thought was most user-intuitive.

src/ripple/app/tx/impl/DID.cpp

src/ripple/protocol/TER.h

src/ripple/protocol/impl/SField.cpp

RichardAH · 2023-08-01T07:25:10Z

Is the DID object just a first class NFT that's set and unset by the user themselves on their own account? Am I missing something? Should someone else be able to modify this?

mvadari · 2023-08-01T14:37:25Z

Is the DID object just a first class NFT that's set and unset by the user themselves on their own account? Am I missing something? Should someone else be able to modify this?

I think you might be referring to an old version of the spec - it was updated (and changed pretty heavily) about a month ago.

src/ripple/protocol/impl/LedgerFormats.cpp

src/ripple/protocol/impl/TxFormats.cpp

src/ripple/app/tx/impl/DID.cpp

src/test/jtx/DID.h

src/ripple/app/tx/impl/DID.cpp

src/ripple/protocol/SField.h

ckeshava · 2023-08-23T19:17:42Z

src/ripple/protocol/impl/TER.cpp

@@ -92,6 +92,7 @@ transResults()
        MAKE_ERROR(tecINSUFFICIENT_FUNDS,            "Not enough funds available to complete requested transaction."),
        MAKE_ERROR(tecOBJECT_NOT_FOUND,              "A requested object could not be located."),
        MAKE_ERROR(tecINSUFFICIENT_PAYMENT,          "The payment is not sufficient."),
+        MAKE_ERROR(tecEMPTY_DID,                     "The DID object cannot be made to be empty."),


Suggestion: I think it would be useful to say "Atleast URI or Data field must be non-empty in a DID object"

src/test/rpc/LedgerRPC_test.cpp

intelliot · 2023-09-07T18:19:00Z

How should this PR be moved forward? Should it be considered for the next release?

mvadari · 2023-09-19T17:58:46Z

How should this PR be moved forward? Should it be considered for the next release?

@intelliot the spec isn't finalized yet and there isn't much time between the final spec release and the 2.0 release. I'd like it to go into 2.0, but it may get pushed to the next release.

src/ripple/protocol/SField.h

src/test/app/AccountDelete_test.cpp

src/test/rpc/LedgerRPC_test.cpp

src/ripple/app/tx/impl/DID.cpp

gregtatcam

👍 LGTM

intelliot · 2023-09-26T18:20:34Z

note: let's wait for updated spec (at least) before merging this

updates needed at: XRPLF/XRPL-Standards#100

should also wait for QA testing to complete (~ next week)

intelliot · 2023-10-09T16:11:47Z

(note: checking with @nbougalis regarding whether he still has change requests)

sgramkumar · 2023-10-11T03:05:02Z

QA tests 👍

mvadari · 2023-10-11T20:21:16Z

@intelliot this is good to go

intelliot · 2023-10-12T15:20:26Z

note: @dangell7 has reviewed: "Looks good to me."

dangell7 · 2023-10-12T16:28:14Z

src/ripple/protocol/LedgerFormats.h

+
+       \sa keylet::did
+    */
+    ltDID = 0x0049,


nit: This clashes with a LedgerFormat we use in Xahau.

Just to close the loop: this was discussed offline and Xahau will take care of this on their end

intelliot · 2023-10-12T17:02:49Z

Suggested commit message:

`DID`: Decentralized identifiers (DIDs) (XLS-40): (#4636)

Implement native support for W3C DIDs.

Add a new ledger object: `DID`.

Add two new transactions:
1. `DIDSet`: create or update the `DID` object.
2. `DIDDelete`: delete the `DID` object.

This meets the requirements specified in the DID v1.0 specification
currently recommended by the W3C Credentials Community Group.

The DID format for the XRP Ledger conforms to W3C DID standards. The
objects can be created and owned by any XRPL account holder. The
transactions can be integrated by any service, wallet, or application.

intelliot · 2023-10-13T21:26:07Z

note: @sappenin would like the spec to be merged prior to this implementation

Spec: XLS-40: DID Specification XRPL-Standards#136

seelabs

👍

gregtatcam

👍 LGTM

- `ledger_entry` is the recommended read method, and it is more efficient than the `account_objects` method. - Note that both methods support `DID` object retrieval. Context: - Prior discussion: #100 - Implementation: XRPLF/rippled#4636 --------- Co-authored-by: Mayukha Vadari <[email protected]>

…om rippled code (#2491) Add support for XLS-40 and adds a script to automatically generate transaction models from rippled source code. ### Context of Change XRPLF/XRPL-Standards#136 XRPLF/rippled#4636

Implement native support for W3C DIDs. Add a new ledger object: `DID`. Add two new transactions: 1. `DIDSet`: create or update the `DID` object. 2. `DIDDelete`: delete the `DID` object. This meets the requirements specified in the DID v1.0 specification currently recommended by the W3C Credentials Community Group. The DID format for the XRP Ledger conforms to W3C DID standards. The objects can be created and owned by any XRPL account holder. The transactions can be integrated by any service, wallet, or application.

seelabs requested review from seelabs and gregtatcam July 31, 2023 18:11

initial DID implementation

cef1f64

mvadari force-pushed the did branch from f3487a4 to cef1f64 Compare July 31, 2023 21:53

nbougalis suggested changes Aug 1, 2023

View reviewed changes

cjcobb23 reviewed Aug 1, 2023

View reviewed changes

src/ripple/protocol/impl/LedgerFormats.cpp Outdated Show resolved Hide resolved

src/ripple/protocol/impl/TxFormats.cpp Outdated Show resolved Hide resolved

mvadari added 3 commits August 17, 2023 17:57

add RPC tests

2f4c285

respond to comments, handle max blob length

99b53be

add AccountDelete test

5916f5f

ckeshava reviewed Aug 23, 2023

View reviewed changes

src/test/rpc/LedgerRPC_test.cpp Show resolved Hide resolved

ckeshava approved these changes Sep 8, 2023

View reviewed changes

intelliot added the Needs Discussion label Sep 8, 2023

mvadari added 5 commits September 15, 2023 16:38

update to latest version

7ee38f9

merge develop

27ad50c

merge develop

ed9e915

Merge branch 'develop' into did

10b6ff7

Merge branch 'develop' into did

7fc2015

gregtatcam requested changes Sep 21, 2023

View reviewed changes

mvadari added 2 commits September 26, 2023 13:29

respond to comments

298077f

add another simplification lambda

04066ae

mvadari requested a review from gregtatcam September 26, 2023 17:37

gregtatcam approved these changes Sep 26, 2023

View reviewed changes

intelliot added API Change and removed Needs Discussion labels Sep 26, 2023

nbougalis approved these changes Oct 11, 2023

View reviewed changes

cindyyan317 mentioned this pull request Oct 11, 2023

Support XLS-40 (DID) in clio XRPLF/clio#918

Closed

Merge branch 'develop' into did

7eb733f

intelliot mentioned this pull request Oct 11, 2023

XLS-40: DID Specification XRPLF/XRPL-Standards#136

Merged

intelliot requested a review from dangell7 October 12, 2023 15:19

dangell7 approved these changes Oct 12, 2023

View reviewed changes

intelliot changed the title ~~XLS-40d: DID~~ DID: Decentralized identifiers (DIDs) (XLS-40): Oct 12, 2023

Merge branch 'develop' into did

d65b77f

Merge branch 'develop' into did

9885bf5

intelliot added the Clio Reviewed label Oct 17, 2023

Update to latest spec

ea560af

seelabs approved these changes Oct 17, 2023

View reviewed changes

gregtatcam approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'develop' into did

7c2768d

intelliot merged commit b421945 into XRPLF:develop Oct 18, 2023
16 checks passed

mvadari mentioned this pull request Oct 23, 2023

feat: add support for XLS-40 + add script to auto-generate models from rippled code XRPLF/xrpl-py#652

Merged

2 tasks

intelliot mentioned this pull request Oct 23, 2023

Proposed 2.0.0-b4 #4782

Merged

1 task

mvadari mentioned this pull request Oct 23, 2023

feat: add support for XLS-40d + add script to auto-generate models from rippled code XRPLF/xrpl.js#2491

Merged

2 tasks

intelliot mentioned this pull request Oct 27, 2023

PriceOracle: Price Oracle (XLS-47d) #4789

Merged

12 tasks

cindyyan317 mentioned this pull request Nov 2, 2023

Implement DID XRPLF/clio#967

Merged

mvadari deleted the did branch February 15, 2024 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`DID`: Decentralized identifiers (DIDs) (XLS-40): #4636

`DID`: Decentralized identifiers (DIDs) (XLS-40): #4636

mvadari commented Jul 31, 2023 •

edited by intelliot

Loading

nbougalis left a comment

nbougalis Aug 1, 2023

mvadari Aug 1, 2023

nbougalis Aug 1, 2023

mvadari Aug 1, 2023

RichardAH commented Aug 1, 2023

mvadari commented Aug 1, 2023

ckeshava Aug 23, 2023

intelliot commented Sep 7, 2023

mvadari commented Sep 19, 2023

gregtatcam left a comment

intelliot commented Sep 26, 2023 •

edited

Loading

intelliot commented Oct 9, 2023

sgramkumar commented Oct 11, 2023 •

edited

Loading

mvadari commented Oct 11, 2023

intelliot commented Oct 12, 2023 •

edited

Loading

dangell7 Oct 12, 2023

mvadari Oct 17, 2023

intelliot commented Oct 12, 2023

intelliot commented Oct 13, 2023

seelabs left a comment

gregtatcam left a comment

DID: Decentralized identifiers (DIDs) (XLS-40): #4636

DID: Decentralized identifiers (DIDs) (XLS-40): #4636

Conversation

mvadari commented Jul 31, 2023 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Aug 1, 2023

Choose a reason for hiding this comment

mvadari Aug 1, 2023

Choose a reason for hiding this comment

nbougalis Aug 1, 2023

Choose a reason for hiding this comment

mvadari Aug 1, 2023

Choose a reason for hiding this comment

RichardAH commented Aug 1, 2023

mvadari commented Aug 1, 2023

ckeshava Aug 23, 2023

Choose a reason for hiding this comment

intelliot commented Sep 7, 2023

mvadari commented Sep 19, 2023

gregtatcam left a comment

Choose a reason for hiding this comment

intelliot commented Sep 26, 2023 • edited Loading

intelliot commented Oct 9, 2023

sgramkumar commented Oct 11, 2023 • edited Loading

mvadari commented Oct 11, 2023

intelliot commented Oct 12, 2023 • edited Loading

dangell7 Oct 12, 2023

Choose a reason for hiding this comment

mvadari Oct 17, 2023

Choose a reason for hiding this comment

intelliot commented Oct 12, 2023

intelliot commented Oct 13, 2023

seelabs left a comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

`DID`: Decentralized identifiers (DIDs) (XLS-40): #4636

`DID`: Decentralized identifiers (DIDs) (XLS-40): #4636

mvadari commented Jul 31, 2023 •

edited by intelliot

Loading

intelliot commented Sep 26, 2023 •

edited

Loading

sgramkumar commented Oct 11, 2023 •

edited

Loading

intelliot commented Oct 12, 2023 •

edited

Loading