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: OOB - Handling of minor versions #1940

Merged

Conversation

shaangill025
Copy link
Contributor

Signed-off-by: Shaanjot Gill [email protected]

File "/home/indy/.venv/lib/python3.6/site-packages/aries_cloudagent/utils/classloader.py", line 97, in load_class
   |     if "." in class_name:
   | TypeError: argument of type 'NoneType' is not iterable
  • Working on adding minor changes for adding OOB 1.1 and a mechanism for ACA-Py to respond with the same supported version, basically, handshake-reuse-accepted 1.1 for handshake-reuse 1.1 and handshake-reuse-accepted 1.0 for handshake-reuse 1.0.

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to support OOB 1.0 and 1.1.

aries_cloudagent/protocols/out_of_band/definition.py Outdated Show resolved Hide resolved
@swcurran swcurran added this to the 1.0.0 milestone Sep 20, 2022
@swcurran swcurran modified the milestones: Version 1.0.0, Version 0.7.5 Sep 20, 2022
shaangill025 and others added 3 commits September 22, 2022 05:18
Signed-off-by: Shaanjot Gill <[email protected]>
Signed-off-by: DESKTOP-TAED8OJ\shaangill025 <[email protected]>
Signed-off-by: Shaanjot Gill <[email protected]>
@shaangill025 shaangill025 force-pushed the fix_oob_accept_minor_version branch from 75151f2 to ca8308f Compare September 22, 2022 12:18
@shaangill025 shaangill025 marked this pull request as ready for review September 22, 2022 12:23
@shaangill025
Copy link
Contributor Author

@swcurran As discussed, the problem reports [version-with-degraded-features, fields-ignored-due-to-version-mismatch] as specified in RFC0003 have been implemented but commented out.

@swcurran
Copy link
Contributor

@dbluhm FYI.

For all. I suggested that to get this out faster we NOT send the "warning" problem reports -- e.g. responding with higher version of responding with lower version. Are we good with that, or since they are already done and commented out, should we support them. I'm a bit nervous of mobile agents getting problem reports they don't expect and splatting an error on the screen.

@ianco
Copy link
Contributor

ianco commented Sep 22, 2022

I suggested that to get this out faster we NOT send the "warning" problem reports

I agree ... if this is going in a patch release we should minimize the change we are introducing. The full functionality can go into the next 1.0.0-rcx release

ianco
ianco previously approved these changes Sep 22, 2022
swcurran
swcurran previously approved these changes Sep 23, 2022
@swcurran
Copy link
Contributor

@WadeBarnes -- looks like the SonarCloud config is processing test code again. Could you please take a look at that?

Do we need to be concerned about the 4 relevant code smells? The complexity of the version checking code (I assume that is just the way it is) and the raising of the exceptions. All good with those?

The unused variables in the test code and the commented out code are fine/intended, I'm sure.

Thanks

@classmethod
def assign_version_to_message_type(cls, version: str):
"""Assign version to Meta.message_type."""
cls.Meta.message_type = Template(cls.Meta.message_type).substitute(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this overwrite message_type for all the class instances going forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To update message_type in Meta, I found this can only be done in the classmethod before initialization. Regarding overwriting, $version is only present in message_types for OOB v1_0, so it will overwrite. But when we have a hardcoded version such as in other protocols then it won't overwrite.

@shaangill025 shaangill025 dismissed stale reviews from swcurran and ianco via 8d332a8 September 23, 2022 20:40
@swcurran
Copy link
Contributor

@WadeBarnes --- is there a way other than pushing another commit to trigger a re-execution of Circle/CI when things like this happen? I hadn't heard about the Circle/CI issues -- any update on that? Should we take the plunge here and switch to another tool?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

No Coverage information No Coverage information
3.0% 3.0% Duplication

@swcurran swcurran merged commit e62d5ba into openwallet-foundation:main Sep 29, 2022
@TheTechmage TheTechmage mentioned this pull request Oct 6, 2022
4 tasks
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.

OOB Handling of both 1.0 and 1.1 messages
4 participants