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

Allow "diddocContent" in ATTRIB on older networks. #77

Merged

Conversation

peacekeeper
Copy link
Member

Fixes #71.

I know this adds some complexity, but I believe there are advantages in allowing "diddocContent" in an ATTRIB for those older networks that don't support "diddocContent" in a NYM yet.

  • The main advantage is that full DID document support (e.g. custom verification methods) can already be achieved on all existing HL Indy networks, some of which may take months or years to upgrade to a newer version.
  • If there is a "diddocContent" in a NYM, then nothing changes, i.e. only a single transaction is required.

Signed-off-by: Markus Sabadello <markus@danubetech.com>
@peacekeeper peacekeeper force-pushed the peacekeeper-diddoccontent-attrib branch from 3b4088c to 6f45ce9 Compare March 7, 2023 21:25
@dbluhm
Copy link
Member

dbluhm commented Mar 8, 2023

I think it would be beneficial for some clarity on how implementations should behave in the presence of both an attrib diddocContent and diddocContent in a nym. Additionally, I think it would be beneficial to describe when implementations should check for an attrib diddocContent if they choose to support it; i.e. "resolve the nym and if no diddocContent is found, check for an attrib for diddocContent." Simply stating that implementations "MAY" modify the algorithm to retrieve from an attrib instead of from the nym feels like we're opening the door for compatibility issues down the line, causing more friction for deployment of did:indy.

Otherwise, I think this is likely a prudent way to handle slower than ideal rollout of did:indy.

Thoughts @swcurran?

@swcurran
Copy link
Member

swcurran commented Mar 9, 2023

I’m happy to add this and promote that this should be added to all resolver implementations.

It would be useful to have an example in of an ATTRIB based DID Doc Content entry — e.g. the JSON for a given DIDDoc and the ATTRIB raw value. What do you think?

Agree with @dbluhm that there should be a definition of what happens if both a DID with diddocContent and an ATTRIB with the same are found. My $0.02CDN is to say that if diddocContent is found in the DID, no check for a corresponding ATTRIB should be done. I believe that is the same wording we have for the endpoint today.

Thanks @peacekeeper !

@peacekeeper
Copy link
Member Author

My $0.02CDN is to say that if diddocContent is found in the DID, no check for a corresponding ATTRIB should be done.

Yes this was my idea as well, to be consistent with what we are saying about endpoint.

I agree with @dbluhm that some more details should be added to this PR, i'll work on that.

@peacekeeper peacekeeper force-pushed the peacekeeper-diddoccontent-attrib branch from c8445db to 83956c2 Compare March 20, 2023 15:26
@peacekeeper
Copy link
Member Author

@dbluhm @swcurran I updated this PR to move this new content into its own section. I tried to explain better how clients should behave, based on your feedback, which was how I intended it to be anyway. I also added an example.

Maybe you could re-review? Let me know if anything is still unclear or could be improved.

@peacekeeper peacekeeper force-pushed the peacekeeper-diddoccontent-attrib branch from 83956c2 to b7cd063 Compare March 20, 2023 15:31

::: example Example `raw` value of an "ATTRIB `diddocContent`"
```json
{
Copy link
Member

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 example had to be like this — e.g. that the JSON had to be stringified. That’s what I meant about an example. Same as this example ATTRIB on TestNet — https://indyscan.io/tx/SOVRIN_STAGINGNET/domain/339997

"{ \"diddocContent\": { \"@context\": [ \"https://www.w3.org/ns/did/v1\", \"https://identity.foundation/didcomm-messaging/service-endpoint/v1\" ], \"service\": [{ \"id\": \"did:indy:sovrin:123456#did-communication\", \"type\": \"did-communication\", \"priority\": 0, \"serviceEndpoint\": \"https://example.com\", \"recipientKeys\": [\"#verkey\"], \"routingKeys\": [] }] } }"

@swcurran
Copy link
Member

Looks good — although see my comment above. The submission is missing DCO — please fix that. DCO - Developer Certificate of Origin - https://github.com/apps/dco. See “Details” link above, beside failed test.

Signed-off-by: Markus Sabadello <markus@danubetech.com>
@peacekeeper peacekeeper force-pushed the peacekeeper-diddoccontent-attrib branch from b7cd063 to ade379e Compare April 15, 2023 19:46
Signed-off-by: Markus Sabadello <markus@danubetech.com>
@peacekeeper
Copy link
Member Author

@swcurran I agree with you regarding the example, I just added commit 27c27ee to improve that.

I also added the DCO to the commits.

Sorry it took a while to do this.

@peacekeeper
Copy link
Member Author

Could we merge this, or are there any concerns?

@swcurran swcurran merged commit 2c59904 into hyperledger:main Jun 6, 2023
@swcurran
Copy link
Member

swcurran commented Jun 6, 2023

Sorry for the delay — just lost sight of the issue.

@peacekeeper peacekeeper deleted the peacekeeper-diddoccontent-attrib branch June 11, 2023 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow "diddocContent" in ATTRIB
3 participants