Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 28 commits into from
Oct 18, 2023
Merged

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Jul 31, 2023

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

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

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines 109 to 116
if (uri->empty())
{
sleDID->makeFieldAbsent(sfURI);
}
else
{
(*sleDID)[sfURI] = *uri;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/protocol/TER.h Outdated Show resolved Hide resolved
src/ripple/protocol/impl/SField.cpp Outdated Show resolved Hide resolved
@RichardAH
Copy link
Collaborator

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
Copy link
Collaborator Author

mvadari commented Aug 1, 2023

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 Outdated Show resolved Hide resolved
src/ripple/protocol/impl/TxFormats.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/test/jtx/DID.h Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Show resolved Hide resolved
src/ripple/protocol/SField.h Show resolved Hide resolved
@@ -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."),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@intelliot
Copy link
Collaborator

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

@mvadari
Copy link
Collaborator Author

mvadari commented Sep 19, 2023

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 Outdated Show resolved Hide resolved
src/test/app/AccountDelete_test.cpp Outdated Show resolved Hide resolved
src/test/rpc/LedgerRPC_test.cpp Outdated Show resolved Hide resolved
src/test/rpc/LedgerRPC_test.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Outdated Show resolved Hide resolved
src/ripple/app/tx/impl/DID.cpp Show resolved Hide resolved
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@intelliot
Copy link
Collaborator

intelliot commented Sep 26, 2023

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
Copy link
Collaborator

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

@sgramkumar
Copy link
Collaborator

sgramkumar commented Oct 11, 2023

QA tests 👍

@mvadari
Copy link
Collaborator Author

mvadari commented Oct 11, 2023

@intelliot this is good to go

@intelliot
Copy link
Collaborator

intelliot commented Oct 12, 2023

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


\sa keylet::did
*/
ltDID = 0x0049,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@intelliot intelliot changed the title XLS-40d: DID DID: Decentralized identifiers (DIDs) (XLS-40): Oct 12, 2023
@intelliot
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

intelliot pushed a commit to XRPLF/XRPL-Standards that referenced this pull request Oct 18, 2023
- `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 <mvadari@gmail.com>
@intelliot intelliot merged commit b421945 into XRPLF:develop Oct 18, 2023
16 checks passed
@intelliot intelliot mentioned this pull request Oct 23, 2023
1 task
@intelliot intelliot mentioned this pull request Oct 27, 2023
12 tasks
ckniffen pushed a commit to XRPLF/xrpl.js that referenced this pull request Nov 15, 2023
…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
@mvadari mvadari deleted the did branch February 15, 2024 18:09
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Amendment API Change Clio Reviewed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Will Need Documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.