Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: json ld fixes and aca-py interop fixes #1865
base: main
Are you sure you want to change the base?
fix: json ld fixes and aca-py interop fixes #1865
Changes from 4 commits
eee749f
34f1f3e
2c9e53b
29f7d73
9eeb5c8
187cf4e
b641f1a
5a15fb4
ee6403c
cd1befd
53d496b
888909d
61ab47b
41d4180
4493bb9
79bd5ce
aedc9b3
d9940a2
2a10e10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change in regex mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is specifically changed for
did:peer:2
. With the updates to the protocol ofdid:peer:2
the services on the encoded version are not always a list and can be multiple service appended instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example did:peer so I can add a test covering this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should nog skip the service index i think, as then the index references will be messed up. I think we can skip adding it to the document, but also skip the index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also furious when does this happen? If a json transform fails? I don't think we should just swallow such an error. Can you explain why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this I realize this was a bit of a hold over from implementing this functionality in another library that had much stricter serialization rules than typescript does, specifically with unknown additional keys appearing causing errors. With the regex adjustment this is redundant for Credo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this logic as it is not needed in Credo because serialization is more fault tolerant than I thought at development time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my comment below about did:peer:2 adding multiple services this was added because some of the agents and mediator I tested against included didCommV2 services that were intended to be ignored by the agent if they do not use didCommV2. Have this try catch here successfully ignores it and keeps the indexes of the services we do recognize consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But by doing this the references between implementations will mess up. We would skip didcomm v2 and others would not skip it and thus the same id could point to different services. I think we should not do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFter looking at this in more depth -- we don't handle the DIDComm v2 serviceEndpoint being an object. So instead of doing this, we need to fix that parsing of serviceEndpoint instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have example did:peer:2 document with DIDComm v2 service for test? I have a PR locally that correctly fixes #1789, and thus should also fix this issue