-
Notifications
You must be signed in to change notification settings - Fork 48
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
docs: ADR-024: Decentralized identifiers resolver #1289
Conversation
👀 Docs preview available here: https://65bcae4e5638ab5cfa22ce0a--desmos-docs.netlify.app |
a5bc7b6
to
e46817e
Compare
e46817e
to
f7d7c9f
Compare
// Documentation: https://www.w3.org/TR/did-core/#also-known-as | ||
string also_known_as = 4; | ||
|
||
// Keys of verification methods for verifying digital signature, must have at least one associated to Desmos account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must have at least one associated to Desmos account.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed at least one to be Desmos message signer, then we can verify if the public key matches message signer, if yes then we can allow the sender to modify the DID document. Otherwise, no one can update the DID document.
} | ||
|
||
// MsgCreateDidDoc represents the message to be used to create a new DID document. | ||
message MsgCreateDidDoc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that inside this message there are some fields that contain some data that can ideally be faked by the user. For example I have the following questions:
- How can we make sure that the data provided by the user is correct (e.g. the verification methods are all valid)?
- Should we allow the user to set the ID? Wouldn't it be better to generate this one instead?
- Should we allow the user to set the Controller ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we make sure that the data provided by the user is correct (e.g. the verification methods are all valid)?
We can only check the public keys is the valid format or not. On the other hand, if user provide the non-owned public key to these fields, then user can not provide the proper signature to verifier, it means that we can recognize that user does not have that DID document.
Should we allow the user to set the ID? Wouldn't it be better to generate this one instead?
Yeah, you are right. Generating on chain would be better for this case. I will update the docs.
Should we allow the user to set the Controller ID?
Controller is a spec that allowing other DID to update the document. It gives more flexibility to DID document control, like group control. To have the flexibility, I think we can have it.
https://www.w3.org/TR/did-core/#the-relationship-between-did-controllers-and-did-subjects
|
||
The store keys of DID document inside KVStore will be as follows: | ||
|
||
* 0x1 | {id} | {version_id} -> ProtocolBuffer(DidDoc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that the only DID that we will use is going to be the one related to a profile, so all these data could be auto-filled by us when a user wants to query a profile as a DID, directly inside the query response.
I don't believe we would want other methods to get a DID of a post or something else. So why do we need to store DIDs on the chain, and we cannot simlpy generate them on-the-fly when a user requests one using a particular query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that having a resolver translating to DID from Desmos address? By this way, we will lose a lot of information and flexibility defined by w3c, like service
, alsoKnownAs
and the possibility other keys to control DID since no one can modify DID document to authorize other keys.
For example, the auto generated DID document will be like:
{
"id": "did:desmos:<desmos-address>"
"verificationMethods": [{ "id": "did:desmos:<desmos-address>#user-key", "type": "Secp256k1", "publicKeyMultibase": "<base58-encoded-key>" }]
"assertionMethod": ["did:desmos:<desmos-address>#user-key"],
"authentication": ["did:desmos:<desmos-address>#user-key"],
"capabilityInvocation": ["did:desmos:<desmos-address>#user-key"]
"keyAgreement": ["did:desmos:<desmos-address>#user-key"]
}
NOTE: did:desmos:<desmos-address>#user-key
is the public key of Desmos account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all the things that you have listed?
- Do we really need the user to allow specifying a
service
oralsoKnownAs
? - What would it mean for a Desmos account, which is strictly tied to a Desmos private key, that other keys can control the DID? Since the DID is representing the Desmos account/profile, and it's not technically possible to have other keys controlling such profile, what would it mean if a user set other keys to control such DID? Wouldn't it imply that the profile is controled by someone external and thus does not represent the identity anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the user to allow specifying a service or alsoKnownAs?
They are optional information defined by users for their custom application usage. For example, on the service field side, user can set a bank service where this DID document especially use. The application can read this service field to quickly recognize this DID document is for bank service. There is also a resolution query method which is did:desmos:<id>#<service-name>
: https://www.w3.org/TR/did-core/#fragment
On the other hand, alsoKnownAs
is also used in application situation, to define the DID document related on other external ID but no guaranteed. For instance, setting paul_chen_tw
on X on this field, requiring another process to link ID on X and my DID document. So, I think alsoKnownAs field is not necessary and can be removed.
What would it mean for a Desmos account, which is strictly tied to a Desmos private key, that other keys can control the DID? Since the DID is representing the Desmos account/profile, and it's not technically possible to have other keys controlling such profile, what would it mean if a user set other keys to control such DID? Wouldn't it imply that the profile is controled by someone external and thus does not represent the identity anymore?
DID designed by W3C allows users to authorize other public keys to control their DID document by adding the key into capabilityInvocation field. The DID identity is owned by the public key owner, so user should add the keys they really owns. So I don't think DID document for general purpose can be seen as a Desmos profile. https://www.w3.org/TR/did-core/#capability-invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am misunderstanding something. I thought what we will build is a general DID document registry like cheqd. After this discussion, I think your decision is resolving Desmos profile as DID document rather than building a general registry for Desmos core. Do I understand correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly that was my idea. I don't think that having a generic DID registry might be good for Desmos. But resolving Desmos profiles as DIDs it might be indeed. I would like to ask @kwunyeung his thoughts though as I might be the one misinterpreting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RiccardoM what you have mentioned is the same as what I originally thought of implementing DID. This feature is to provide a web standard compatible representation of Desmos Profile. We can generate all data of the profile as the DID structure which can be easily adapted by any applications that are going to make use of the DID w3c standardiaztion.
Even for fields like service
and alsoKnownAs
should be generated from what data has been stored on chain with the chain-link
or app-link
. E.g., we may see
did:ens:forbole.eth
did:ethr:0x1234123123123123123123
twitter:forbole
mailto:[email protected]
The applications that supports those DIDs should support those resolvers from their ends.
It's still early but in the long run, the service
field could be useful. E.g., a university is providing a service for providing educational qualification proofs to an identity and the service is registered on Desmos. If that specific DID is linked with the Desmos Profile, then in the Desmos Profile DID, we can have a service that is pointing to that DID with the qualification verification service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwunyeung After the discussion, I now understand that our DID will tie to the user profile in Desmos, in the other word, the starting point is Desmos profile.
I agree with we can resolve chain-link
and application-link
in alsoKnownAs
field. However, service
may not be useful since it is a hint by user that tells where the DID can be used. In our case, our DID should be use for all the apps inside Desmos ecosystem.
@RiccardoM Updated ADR to DID resolver from DID registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Clear and effective 💯 We can accept it and start with the implementation.
docs/architecture/adr-024-decentralized-identifiers-resolver.md
Outdated
Show resolved
Hide resolved
Warning Rate Limit Exceeded@RiccardoM has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 44 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update introduces the integration of Decentralized Identifiers (DIDs) into Desmos, enhancing identity management by allowing Desmos Profiles to be resolved into DID documents. It marks a significant step towards leveraging decentralized digital identity within the platform, aligning with W3C specifications. Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (1)
- docs/architecture/adr-024-decentralized-identifiers-resolver.md (1 hunks)
Additional comments: 1
docs/architecture/adr-024-decentralized-identifiers-resolver.md (1)
- 56-62: The protobuf definition for the
Query
service is clear and follows best practices. However, ensure that the URL in theoption (google.http.get)
annotation correctly reflects the API's versioning and path.
docs/architecture/adr-024-decentralized-identifiers-resolver.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-024-decentralized-identifiers-resolver.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-024-decentralized-identifiers-resolver.md
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Closes: #XXXX
This PR writes the documentation of how to integrate DIDs registry to Desmos.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
DidDoc
query method for resolving Desmos Profiles into DID documents.