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

Align Forward message with RFC 0094: msg must be JSON dict, not a string #240

Merged

Conversation

jovfer
Copy link
Contributor

@jovfer jovfer commented Oct 28, 2019

As defined in the RFC nested msg in Forward must be nested JSON object returned by pack message

https://github.com/hyperledger/aries-rfcs/tree/master/concepts/0094-cross-domain-messaging#corerouting10forward

@jovfer jovfer force-pushed the hotfix/fix_forwarding_msg branch from d5e9062 to 3585b50 Compare October 28, 2019 10:08
@jovfer jovfer changed the title Align Forward message to RFC 0094: msg should be JSON dict, not a string Align Forward message with RFC 0094: msg should be JSON dict, not a string Oct 28, 2019
@codecov-io
Copy link

Codecov Report

Merging #240 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   73.51%   73.51%   +<.01%     
==========================================
  Files         219      219              
  Lines       10267    10268       +1     
==========================================
+ Hits         7548     7549       +1     
  Misses       2719     2719

@jovfer jovfer changed the title Align Forward message with RFC 0094: msg should be JSON dict, not a string Align Forward message with RFC 0094: msg must be JSON dict, not a string Oct 28, 2019
@jovfer
Copy link
Contributor Author

jovfer commented Oct 28, 2019

@kdenhartog FYI

@swcurran swcurran added the blocked This PR is blocked pending coordination label Dec 10, 2019
@swcurran
Copy link
Contributor

swcurran commented Dec 10, 2019

As I understand it, this PR matches the RFC. @andrewwhitehead @tmarkovski - how can we resolve this? Do we need to coordinate new releases of Streetcred Mobile app and ACA-Py to get this changed?

Thanks

@tmarkovski
Copy link

Yes, we need to coordinate the release to avoid breaking changes. I'm a little unclear though, libindy returns the pack result as string, but the forward wants it as object. In strongly typed languages this adds a step of converting the string to a JSON first. What is the reason for the type change in the forward message? Especially since we talked about reworking the routing standard?

@swcurran
Copy link
Contributor

@jovfer @tmarkovski @andrewwhitehead - could one of you take the lead on this for discussion on the next Aries Working Group call so we can get this clarified and an approach defined?

@TelegramSam - could you add this to the agenda?

@jovfer jovfer force-pushed the hotfix/fix_forwarding_msg branch from 3585b50 to 44ed4af Compare December 25, 2019 10:59
@esplinr
Copy link

esplinr commented Jan 22, 2020

It sounds like the change is made in Street Cred (please correct if I'm wrong @tmarkovski). Does that mean we can accept this PR so that we have increased interoperability in the next release of ACA-py?

@swcurran
Copy link
Contributor

Sigh... No notes in the issue, no update on what we planned to do. #frustrating.

Does anyone know where we documented what we would do?

@andrewwhitehead - did we decide anything on this? I thought we weren't going to do anything, but I can't find anything about what was decided. Thanks.

@ianco
Copy link
Contributor

ianco commented Jan 22, 2020

I think we just need to merge this PR.

FYI I made these same changes on a local branch and tested interconnectivity between aca-py and VCX successfully.

@swcurran
Copy link
Contributor

swcurran commented Jan 22, 2020 via email

@tmarkovski
Copy link

@swcurran This feature is currently implemented in Aries .NET and tested for interop with Vcx.
Streetcred hasn't applied this specific version to our mobile app or agents, but will do so with the Jan 30 release.

@swcurran
Copy link
Contributor

Our plan is to merge this now and package. When the Streetcred agent hits the App Store, we'll immediately redeploy all our apps so that we minimize the down time.

@andrewwhitehead - please go ahead with the merge and plan out the rest of the 0.4.1 changes.

Hopefully we don't have to do this break production dance in the future. AIP and the community upgrade process will help with this. On a positive note, we can do the work ahead of time to get the deployments ready to go.

@WadeBarnes @esune - you will be getting 0.4.1 images soon and we'll need to have a list of redeployments done as soon as we hear the new Streetcred app is in the app store. I'll add a ticket for this in the von repo.

@tmarkovski - please let us know when the app is in the app store.

Thanks!

@andrewwhitehead andrewwhitehead merged commit cba01dd into openwallet-foundation:master Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This PR is blocked pending coordination
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants