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

feat: proper resolutions APIs for AssetDIDs #714

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ntn-x2
Copy link
Member

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

Fixes https://github.com/KILTprotocol/ticket/issues/2093.

Improves the API around AssetDID resolution, mirroring what we already do for KILT DIDs.

Specifically:

  • Rename the DidResolver, DidDocumentExporter and DidDocument modules of the types package by prefixing them with Kilt. Nothing will break for users.
  • Rename the AssetDid.parse() function to AssetDid.resolve()
  • Add AssetDid.resolveCompliant(), which mirrors what we do for KILT DIDs
  • Include AssetDid.exportToDidDocument(), which also mirrors what we do for KILT DIDs

Unit tests for resolver and exporter have also been included.

@ntn-x2 ntn-x2 self-assigned this Feb 7, 2023
@ntn-x2 ntn-x2 marked this pull request as ready for review February 10, 2023 09:43
@ntn-x2 ntn-x2 requested a review from rflechtner February 10, 2023 09:44
@ntn-x2
Copy link
Member Author

ntn-x2 commented Feb 10, 2023

@rflechtner I guess because of the parse -> resolve renaming, this could not be included in a patch release for the SDK, so we have to wait for the next release (Eth linking) to eventually include this and build the universal resolver driver.

Comment on lines +19 to +21
export const ASSET_DID_CONTEXT_URL =
'ipfs://QmUAcsTVNfjGoZ3dcuHKikFJZpRiUkXCpbWcfxb1j5qnv4'

Copy link
Contributor

Choose a reason for hiding this comment

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

you added the URI here but you didn't add the context itself below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, missed that -> a3537e1

@ntn-x2 ntn-x2 requested a review from rflechtner February 16, 2023 09:29
Copy link
Contributor

@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.

A few general questions at this point:

  • Is asset DID "resolution" a feature that we need to properly release soon or can we live with the parsing functionality that we released in 0.31.0 for a while? I'm a little worried about merging this at this point because there are still so many open questions surrounding the specs and the vision for this feature and releasing something to the public that is still experimental which we then need to iterate over, change and break is very likely to cause us a bunch of extra work down the line. Maybe we find a way to beta-release an asset did resolver before we release and maintain it as part of the sdk?
  • I'm also asking because this is a huge amount of code for something that still has the feature set of a single regex-expression. I could see that if we fail to develop a vision for what asset DIDs bring to the table beyond what CAIP identifiers can do, we may fall back to just issuing credentials to CAIP-19 ids instead. But once it's out it's out.
  • More to the details of the implementation here, is there a reason we need to have both a pseudo-DID document AND an actual DID document for asset DID resolution? They are virtually identical, no? Wouldn't it be enough to only resolve to actual did documents to make the api surface more manageable?

Also left a few comments regarding the @context for the asset DID document - this unfortunately won't work as is and would need fixing before we go ahead. I'm also thinking we can improve the structure of these documents more generally.

Comment on lines +52 to +66
'@context': {
'@protected': true,
id: '@id',
chain:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
namespace:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
reference:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
asset:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
identifier:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I know JSON-LD is a handful, but this is not how it works. You've defined a bunch of synonyms here, so that a Did document is effectively transformed to this:

{
  "@id": "did:asset:xyz",
  "https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md": [
    {
      "https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md": [
        "assetInstance",
        "assetNamespace",
        "assetReference"
      ]
    },
    {
      "https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md": [
        "chainNamespace",
        "chainReference"
      ]
    }
  ]
}

I'd recommend something like

Suggested change
'@context': {
'@protected': true,
id: '@id',
chain:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
namespace:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
reference:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
asset:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
identifier:
'https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md',
},
},
{
"@context": {
"@protected": true,
"id": "@id",
"docs": "https://github.com/KILTprotocol/spec-asset-did/blob/43457f9d1119bff1c3152e368ddd4e3bdb9558cb/README.md#",
"chain": {
"@id": "docs:chain",
"@context": {
"namespace": "docs:chain-namespace",
"reference": "docs:chain-reference"
}
},
"asset": {
"@id": "docs:asset",
"@context": {
"identifier": "docs:asset-identifier",
"namespace": "docs:asset-namespace",
"reference": "docs:asset-reference"
}
}
}
},

which also signals that an asset namespace is a different thing than a chain namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly looking at this from the linked data angle makes me think that the spec is not quite fully mature yet in this regard. If you look at it as a set of subject - predicate - object statements about a subject (an asset or asset class), the current structure does not really make sense.
First, if the subject of the document is an asset (identified by the asset did), which predicate does asset express? Next, the subject in this statement is interpreted as a node from an LD perspective because it's an object. That means it's an identifiable "thing" in itself; ideally it would have an id too. Also it's the subject of the identifier, namespace, and reference predicates; but as I see it, these statements rather relate to the asset directly, and thus the subject of the did document.
Nesting makes more sense to me in case of the chain predicate if you read it as a "lives on" predicate; in that case you have asset x -> lives on -> chain y and chain y -> has CAIP-2 namespace -> namespace a + chain y -> has CAIP-2 reference -> reference b. In that case I'd argue that the node should have an id which would be the chain's full CAIP-2 identifier (not sure whether it needs a type too?).

I'd recommend you always try these things in the json-ld playground first; especially the table view is quite nice to summarise all the statements you're expressing in your doc. Have a look at this for example, which could also serve as a starting point to improve the did doc we return: shortened url

Copy link
Member Author

Choose a reason for hiding this comment

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

This should have been discussed in the AssetDID specification then, I don't think this is the best place to talk about this. It's just implementation of something we have agreed on before. I am fine digging into the spec itself, and perhaps defining a top-level property that can include both chain and asset side. Probably the spec was rushed and not really checked by anyone else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like your suggestion actually. And it's a very bad mistake on my side to not even try to use the JSON-LD playground, which is an amazing tool. I opened a draft PR in the spec repo, we can discuss there about what the best context definition is and what the best document for a resolved AssetDID is -> KILTprotocol/spec-asset-did#1. We will come back to this PR afterwards. I think there is no issue in fixing the specification with breaking changes, since nobody relies on them anyway now. We only have a parse function in the SDK so far.

asset: {
namespace: assetNamespace,
reference: assetReference,
identifier: assetInstance,
Copy link
Contributor

Choose a reason for hiding this comment

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

results in identifier: undefined if not given, which is not what you would see in a DID document

@ntn-x2
Copy link
Member Author

ntn-x2 commented Feb 17, 2023

@rflechtner I see your point. Indeed the AssetDID spec v1 was designed since the beginning to be a simple wrapper around CAIP identifiers. The goal would be for it to evolve in v2 to include credentials as part of the resolution process, perhaps, and more, for instance alsoKnownAs for bridged assets and so on.

I acknowledge at this point what we have might not be much, and my idea for a "better" interface was indeed to set things up for improvements in the resolution protocol at a later time.

Is asset DID "resolution" a feature that we need to properly release soon or can we live with the parsing functionality that we released in 0.31.0 for a while?

We can definitely go ahead with the parsing functionality. The point is, we call them DIDs, and I would expect people to have a resolve function to resolve the DID, not a parse one. In this sense, you can also then call light DID resolution parse since it's entirely self-contained, but we call it resolve because it's still fetching/parsing/producing information starting from and identifier.

I'm also asking because this is a huge amount of code for something that still has the feature set of a single regex-expression. I could see that if we fail to develop a vision for what asset DIDs bring to the table beyond what CAIP identifiers can do, we may fall back to just issuing credentials to CAIP-19 ids instead. But once it's out it's out.

Yes, but I think having the identifier as a DID allows us to build on top of a standard. If we fall back to using CAIP identifiers, we can definitely never ever build anything on top of that, since there is no standard we can build on. I am open to perhaps update the AssetDID standard to include information you think is missing, but from day 1 we kind of agreed this is the "bare minimum" DID spec for assets, and that we would build on top of that once we have time and if we see fit.

More to the details of the implementation here, is there a reason we need to have both a pseudo-DID document AND an actual DID document for asset DID resolution? They are virtually identical, no? Wouldn't it be enough to only resolve to actual did documents to make the api surface more manageable?

It was mainly to keep the user experience similar to what we have for KILT DIDs. Objects the SDK can more easily understand, and something that can be exported (mainly used by the resolver, in the future). Again, right now this might not make a lot of sense because of what we have, but I would see the two DIDs to evolve together in the SDK. Maybe that's an wrong assumption/vision on my side? I also see public credentials and normal credentials evolving together, since for me they are conceptually very similar.

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