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

chore: fix context definition #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: fix context definition #1

wants to merge 2 commits into from

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Feb 17, 2023

Based on our Slack discussion and PR in KILTprotocol/sdk-js#714.

Copy link
Collaborator

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

I like it! I think with that structure and context we have a very solid base for representing the information contained in a CAIP-19 identifier in a LD fashion. We could explain better what the predicates mean exactly (I think that is the point of predicates being resolvable URLs in the LD approach), but for starters we need to defined the structure and I think we're on to something here. I can already see how we can build on top of this, e.g.

  • The alsoKnownAs field could contain the standard form of the CAIP-19 identifier
  • We could define chain @types such as EthereumBased, SubstrateBased
  • We could define @types or a predicate which tell you whether you are looking at an asset class or an asset instance
  • etc. etc.

There's also a larger discussion to be had regarding how we imagine the fit of CAIP-19, a referencing framework, and DIDs, an SSI framework, but I'm bracketing that here bc I don't think it's the right place.

{
"@protected": true,
"id": "@id",
"asset-did-spec": "https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check what the predicates expand to? Not sure, but you might need to add a # to make sure the result is a valid URL

README.md Show resolved Hide resolved
"id": "@id",
"asset-did-spec": "https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md",
"chain": {
"@id": "asset-did-spec:chain",
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could add "@type":"@id", for completeness, tells the processor that we expect the value of chain to be an IRI. Just covers the case where you'd have "chain": "eip155:0" in your document so yeah, just for completeness.

README.md Outdated

```json
{
"@context": [
"https://www.w3.org/ns/did/v1",
"https://kilt.io/asset-did-context.json"
"https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md"
Copy link
Collaborator

Choose a reason for hiding this comment

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

the context url is expected to return the context document, i.e. JSON, so linking to a markdown file doesn't make too much sense here. I think you can keep it as it was (or use an ipfs URI)

@ntn-x2
Copy link
Member Author

ntn-x2 commented Feb 23, 2023

@rflechtner I addressed your points in 04af93d, thanks a lot for the suggestions!

I decided to actually go for an embedded context in the example resolution, and I think I will mirror that in the SDK as well, since it actually does make sense to me, that we ship a new context whenever we change something in the resolver anyway. Maybe we should do the same for KILT DIDs as well, what do you think?

Anyway, this is a screenshot of how the context is evaluated now with the example data, which looks good enough to me.

Screenshot 2023-02-23 at 17 27 03

@ntn-x2 ntn-x2 marked this pull request as ready for review February 23, 2023 16:27
Copy link
Collaborator

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

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

Nice, but one of my comments (https://github.com/KILTprotocol/spec-asset-did/pull/1/files#r1112908032) still needs to be addressed before we can go ahead. I think this breaks JSON-LD processing.

I decided to actually go for an embedded context in the example resolution, and I think I will mirror that in the SDK as well, since it actually does make sense to me, that we ship a new context whenever we change something in the resolver anyway. Maybe we should do the same for KILT DIDs as well, what do you think?

I'm not too sure about the embedding; all this extra data may look confusing to somebody who doesn't know what JSON-LD is. And from an efficiency standpoint it's better to fetch a context definition once instead of including with every did resolution of course. But I'm not totally against it.
For KILT DIDs, the context definition internally links to yet another context definition, so I'm not sure we gain a lot by embedding it; and the same arguments about size and complexity apply.

"https://kilt.io/asset-did-context.json"
{
"@protected": true,
"asset-did-spec": "https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still broken I think. Try adding the hash in the playground and observe how the expanded result changes

Suggested change
"asset-did-spec": "https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md",
"asset-did-spec": "https://github.com/KILTprotocol/spec-asset-did/blob/main/README.md#",

@rflechtner
Copy link
Collaborator

Anyway, this is a screenshot of how the context is evaluated now with the example data, which looks good enough to me.

Screenshot 2023-02-23 at 17 27 03

BTW I think there is a bug in this viewer; it should look like what I pasted below but you only get this if you use @id instead of id (which are supposed to be synonyms?). As far as I can tell the table view gives you identical statements for both variants.

Screenshot 2023-02-28 at 13 51 54

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.

2 participants