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

Fix: present-proof v1 send-proposal flow #1811

Merged
merged 6 commits into from
Jun 21, 2022

Conversation

shaangill025
Copy link
Contributor

@swcurran
Copy link
Contributor

@etschelp -- can you test this with the problem you reported in #1809? Would like to get this merged and included in the release. And would REALLY like to get the release out :-).

Thanks, @shaangill025 !

@etschelp
Copy link
Contributor

Ok, works again. What worries me though is that #1710 broke this because these lines were removed and now they have been added again, which raises two concerns:

  1. Why wasn't there a failing test? - I think it would makes sense to add a couple of lines to: test_presentation_request_handler.py to also check for the thread_id
  2. Why were those lines removed, and might adding them back cause issues with the OOB changes in turn?

I will also comment this on said PR so that Timo can chime in.

etschelp referenced this pull request Jun 17, 2022
@TimoGlastra
Copy link
Contributor

It's unclear to me how this will fix the issue with the thread id?

Also @etschelp, the code you commented on in my PR is not the same code that is being changed here.

I changed the functionality because we don't always a connection id anymore (connectionless, or connection but not yet in record because of create-offer/create-request). Were you able to verify the thread id is incorrect? If that's the case I think we need to fix this in another way

@etschelp
Copy link
Contributor

@TimoGlastra sorry, you are right, I'm totally confused now. All I know for sure is that if the holder starts with /send-proposal the thread id changes during the exchange leading to a storage error on the verifier side. This was introduced either with or around 1710. It works again with this PR.

@shaangill025
Copy link
Contributor Author

shaangill025 commented Jun 17, 2022

It's unclear to me how this will fix the issue with the thread id?

Earlier when initiated from proposal, presentation_request_dict would not be assigned [None] for the record and then upon calling assign_thread_from [holder] while creating presentation it would use presentation message id as thid which was incorrect.

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jun 17, 2022

Found the issue. I changed the assign_thread_from to use the presentation request message dict thread instead of the presentation record thread id. This is done because with OOB we now also leverage the pthid which we need to include in the next message.

As the dict wasn't being stored this wouldn't assign anything and it would generate a new thread_id. So it makes sense this fixes the issue

edit: relevant code: fcb464c#diff-d6a966f1c3464ace4fe2f7ef483077f4f3f3fbf0ae58fcf0d8bb8a2507df853bR280

@swcurran swcurran merged commit 5ac6086 into openwallet-foundation:main Jun 21, 2022
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.

Prover mixes up thread_id in present-proof/send-proposal flow
5 participants