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

Endpoints ATTRIB json is spec divergent #1928

Closed
dbluhm opened this issue Sep 6, 2022 · 3 comments
Closed

Endpoints ATTRIB json is spec divergent #1928

dbluhm opened this issue Sep 6, 2022 · 3 comments
Assignees

Comments

@dbluhm
Copy link
Contributor

dbluhm commented Sep 6, 2022

This is a quick issue to document some discussion that took place on discord. I'll provide a summary and a copy-paste of the conversation so we can track the issue here.

Recently, I opened PR #1863 (addressing #1313) that contained some misunderstandings on my part of what the endpoint attirb should look like.

Prior to this PR, ACA-Py expected to receive endpoint attrib data that looked like this:

{
  "endpoint": "https://example.com",
  "profile": "https://example.com/profile",
  "linked_domains": "https://example.com/linked-domains",
}

I'm not sure where the profile and linked_domains convention is defined but these were being included as separate service endpoints in the constructed DID document. According to the new rules for did:sov, the endpoint attrib should look like:

{
  "endpoint": "https://example.com",
  "types": ["did-communication", ...],
  "routingKeys": [...],
}

I made a misguided attempt to combine the two, which resulted in this structure:

{
  "endpoint": {
    "endpoint": "https://example.com",
    "types": ["did-communication", ...],
    "routingKeys": [...],
  },
  "profile": "https://example.com/profile",
  "linked_domains": "https://example.com/linked-domains",
}

The resolution of a structure looking like the above was merged in with #1863 and then a follow up PR from @cjhowland #1899 added the registration (again, mislead by my misunderstanding of the endpoint attrib structure). This lead to some errors in both the VON network browser and Indy Scan on ledgers where these changes were tested out, alerting us to the fact that this endpoint data doesn't look right.

We have a couple of questions to answer:

  • How should profile and linked_domains fit into the new endpoint attrib json?
  • How are other agents handling this?
@dbluhm dbluhm self-assigned this Sep 6, 2022
@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 6, 2022

Quick transcription of the thread:

Christian Bormann

09/01/2022
Can we change the implementation to already go the did:indy way - bascically expecting full-blown service descriptions in ATTRIB as described here https://hyperledger.github.io/indy-did-method/#the-endpoint-attrib ? That way we would fix the current issue and already implement some of the client changes necessary with did:indy.
By introducing routingKeys we kind of need a separation of the different services imho (because routingKeys would need to belong to specific services)

I guess it is time to revisit https://github.com/hyperledger/aries-rfcs/blob/main/features/0067-didcomm-diddoc-conventions/README.md and perhaps generate/test some real-world examples like i tried in hyperledger/indy-did-method#55 ?

Daniel Bluhm

09/01/2022
Unless I'm misreading, I think the Indy DID Method spec doesn't explicitly recommend any changes to usage of the attrib and basically just describes the old did:sov convention:

...an ATTRIB ledger object with a raw value of contain the JSON for a name-value pair of endpoint and a URL endpoint, often an IP address.

But I think just using a full did doc service array would be the most complete solution. I think that would more or less represent the endpoint attrib "end game" since it can fully express whatever you want to put in a service endpoint. Hopefully meaning we won't have to revisit the convention again later lol.

Christian Bormann

09/01/2022
Hmm I was under the impression the ATTRIB could contain a service array (as diddocContent), but I guess i misunderstood/remembered that part? How exactly would this look like?
The DIDDoc produced by the NYM and “endpoint” ATTRIB would be created using the DIDDoc Assembly Rules and using the diddocContent from the ATTRIB instead of the NYM item.

Imho: Go with a full did doc service array and revisit https://github.com/hyperledger/aries-rfcs/blob/main/features/0067-didcomm-diddoc-conventions/README.md to figure out if this fits our current needs (for the aries style didcomm endpoint) and is interoperable with the current state of did-core / didcommv2 specs.

Daniel Bluhm

09/01/2022

How I read it was that an attrib of {"endpoint": {"endpoint": "https://example.com/my-agent"}} should be considered equivalent to

"diddocContent" : {
"@context" : [ "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/my-agent",
"recipientKeys": [ "#verkey" ],
"routingKeys": [ ]
}
]
}

And then worked into the DID Doc according to the assembly rules
At any rate, I agree on the did doc service array point and maybe the indy did method spec should be updated to account for an attrib of this format
Christian Bormann

09/01/2022
Hmm yeah the upper part sounds like it but the bottom part makes it sound like you can just inject service information via diddocContent in ATTRIB - or at least I understood it as such 😄 ? We should probably figure that part out and clarify ^^
Do we have any disadvantage by allowing diddocContent with a service array in ATTRIB until people switch to did docs with service definition in NYM ?
How is acapy currently dealing with the changes of endpoint attrib format? We need to be backwards compatible I assume?
Daniel Bluhm

09/01/2022
I think the only disadvantage I see is that it is another "legacy" format that we'd have to support but yeah, you've hit the nail on the head, in order for ACA-Py to continue to support the profile and linked domains endpoints, we need something more than what the current set of conventions define.

As it currently stands, ACA-Py is accepting endpoint attrib json of the following formats:

{
"endpoint": "https://example.com",
"profile": "https://example.com/profile",
"linked_domains": "https://example/linked-domains"
}

This is the "original" format ACA-Py expected. Prior to #1863, it was not possible to express routing keys and types.

After that PR, the expected format is divergent from established conventions (my fault lol; I misunderstood the profile and linked domains):

{
"endpoint": {
"endpoint": "https://example.com",
"types": ["did-communication", ...],
"routingKeys": ["...", "..."],
},
"profile": "https://example.com/profile",
"linked_domains": "https://example.com/linked-domains",
}

ACA-Py will accept either of those formats currently.

In summary, ACA-Py is not currently handling the changes correctly.
@swcurran what do you think of the diddocContent in attrib idea as a bridge between did:sov and did:indy? I think the alternative for ACA-Py's current resolution behavior would be to only support profile and linked domains in the old format or to have them as siblings to the endpoint types and routingKeys attributes.
swcurran

09/01/2022
Lots of fun.

When I wrote the comment Daniel reports above in the "did:indy" spec., I thought the only ATTRIB format that was used was a endpoint/single URL. As these comments from a did;sov issue indicate, that assumption was wrong -- #1313 (comment)

Since this isn't well specified -- how about looking at the code that AFJ has and seeing if it makes sense to use? Look at Timo's comment following the GitHub issue comment linked above. The goal is consistency across Aries frameworks, after all. That said, AFJ has not really been pushed too hard on the server side, so that code may not have been used.

We could add a way to write "didDocContent" as a new-style ATTRIB as well, as the bridge. I think that is a good idea as it is a bridge to the did:indy feature. It sounds like not too much work -- a new source for the data, encoding/decoding the JSON. However, once you have it, it will be the same code as the "did:indy" processing. As Daniel notes, it creates a precedent, but I don't think it is a horrible one.

Sigh...legacy fun.

@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 6, 2022

Took a closer look at AFJ which supports endpoint attrib data like the following:

{
  "endpoint": "https://example.com",
  "types": ["did-communication", ...],
  "routingKeys": [...],
  "profile": "https://example.com/profile",
  "linked_domains": "https://example.com/linked-domains",
}

We'll prepare a fix for ACA-Py to have it behave the same way. Should be pretty quick.

cc @swcurran

@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 16, 2022

Fixed by #1934

@dbluhm dbluhm closed this as completed Sep 16, 2022
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

No branches or pull requests

1 participant