-
Notifications
You must be signed in to change notification settings - Fork 71
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
Sort out OOB Handshake Protocols #583
Comments
The error message is raised from: |
Thanks @cvarjao. Do you think we could expect an empty array to be handled/treated as if the option was not set (this is what ACA-Py seems to do), or do we need to gather consensus on what "optional" attribute means first? Trying to figure out where to route this issue next, since VC-AuthN is just a "consumer". |
I do not understand the protocol at the level to provide any meaningful feedback, but sounds like credo/aca-py need to have some alignment on how it is implemented though. |
I'm running into this after my fix for the |
Please pull/use the latest |
As per Aries RFC 0434 I don't see any meaningful case where handshake_protocols contains an empty array. I guess ACA-Py tries to not send anything because your flow is a connection-less one (i.e. no connection reuse is expected, as I see in #513). Although this can be fixed in ACA-Py and also in Credo's invitation acceptance method, I think your suggestion about considering an empty array from ACA-Py response as None is correct (and maybe faster to implement). In any case, I'll create a PR in Credo to handle this situation properly. |
So the protocol problem is that in the RFC, The clarification in the protocol should be that by optional, either is valid and mean the same thing -- not including Not ideal -- the protocol should have been clear from the start in saying that "optional" means don't include it when it is not needed. I think that was intended, but obviously others interpreted it differently. |
Thanks @genaris ! For this, unless anyone has a better idea I think we can leave this issue open, once there ends up being a BC Wallet release that integrates the Credo change reimplement the fix in VCAuth-N that will allow handshake protocols back into the OOB message. (other option is config that toggles it if someone needs the handshake protocols and is using this codebase in the meantime) |
I got some help from ChatGPT on clarifying the protocol — including what a sender SHOULD do and what a recipient MUST do. What do you think? Certainly! Here is the protocol description in Markdown: MessagesThe out-of-band protocol consists of a single message that is sent by the sender. Invitation:
|
Wow @swcurran , it seems that Chat GPT is learning fast 😄 . I think it gave some good definitions about what 'optional' means. Maybe Aries RFC 0003 and DIDComm Book or could be a good place for them. |
FYI credo-ts v0.5.8 has been released. It includes the fix for this issue. |
OOB proofs are working to the BC Wallet provided we do not put anything in
handshake_protocols
The call to ACA-Py to make the OOB includes the handshake_protocols in the response, but we were not including that in our model (by mistake!) so it does not end up in the QR Code/Deep Link payload.
Fixing that and having it go through as
[]
causes an invalid QR code with an error in the walletSetting content in there, didcomm or connections causes the wallet to spin.
Some context here: #582
I don't know if we are missing some other fields in our OOB invitation creation in order to be using handshake_protocols as a blank, or filled array (or which of those cases we'd want for sure)
Or not sure if this is an issue on the BC Wallet side, or Credo or something...
need some investigation then can add back in handshake_protocols to our model.
The text was updated successfully, but these errors were encountered: