fix: light DID decoding logic #725
Conversation
| objectToSerialize[SERVICES_MAP_KEY] = service.map( | ||
| ({ id, serviceEndpoint, type }) => ({ | ||
| id: resourceIdToChain(id), | ||
| serviceEndpoint, | ||
| type, | ||
| }) | ||
| ) |
There was a problem hiding this comment.
hmm how does this help? I think it was more compatible the way it was before. This way we'll just end up with serialised service endpoints where serviceEndpoint is undefined and url is silently dropped.
There was a problem hiding this comment.
This is serialization of the new stuff. So new endpoints always serialize to id, serviceEndpoint, type. The trick is in the DEserialization, which attemps to use serviceEndpoint and type, and falls back to types and urls if those are not found.
There was a problem hiding this comment.
In this snippet here I replaced ...rest, which was anyway just serviceEndpoint and type with its building components.
There was a problem hiding this comment.
I reverted the use of ...rest and now the unit tests are passing. I was not expecting anything to break actually. Probably I have the wrong understanding of what the spread operator does, or at least of how it's CBOR encoded.
There was a problem hiding this comment.
so what the spread using ...rest does is transform the id property and keep the remaining properties as-is, whatever they are. If you use explicit key names, you just make assumptions about which keys are there, which then breaks if the keys have different names. If you wanted to make sure that uri is encoded as serviceEndpoints, you'd need to do it the way you do it below. But I think we re-encode on resolving a light did, so that could result in the light did resolving to a different light did.
There was a problem hiding this comment.
Yes, but I thought that ...rest was indeed those keys. That's why I'm confused.
There was a problem hiding this comment.
Well not if you're dealing with a legacy DID, then it's the legacy keys
There was a problem hiding this comment.
although the services validation runs beforehand and should not allow the use of any other keys 🤔 anyway, it's good that we reverted it bc that was not the issue and the remainder syntax is less brittle.
|
before you merge: I realised that even though these old light dids seem to exist, they violate our own specs - so do we really need to support them? |
|
At least the example uses the proper keys ( |
|
Because we changed the spec. Updating the spec does not mean we should not support old DIDs, right? If we do, that would also be fine, but then the resolver (driver) should not fail with 500 internal error. I think since we have the fix here, it would not hurt to have it in. I also don't expect there to be that many old DIDs left. But some might, and might also even have credentials. |
|
Or maybe I am still missing something that happens between deserialization and re-serialization. I only know the SDK throws and the DID resolver returns 500. What fix do we want to implement then? |
|
Ok let's merge it then. |
* Fix light DID decoding logic * Remove irrelevant resolver unit test * Revert usage of ...rest
* fix: jsdocs for public credential fetch* functions (#723) * fix: light DID decoding logic (#725) * chore: move DidExporter snippets to docs (#722) * chore: v0.31.1 --------- Co-authored-by: Antonio <[email protected]>
No description provided.