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

Load connections didcomm protocols #1468

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jamshale
Copy link
Contributor

The didcomm protocol message types weren't loaded during setup which caused them to be missing when installed remotely and loaded via plugin.

This appears to make the v1 OATH tests pass but the other tests fail because this plugin binds to the BaseConnectionManager causing the OutOfBandManager to fail.

@jamshale
Copy link
Contributor Author

@dbluhm Was this the intended behavior? To either have the OOB connection manager or the v1 connection manager and not both?

@jamshale jamshale requested review from swcurran and dbluhm January 29, 2025 00:19
swcurran
swcurran previously approved these changes Jan 29, 2025
@jamshale
Copy link
Contributor Author

Looks like it's not an issue with the binding. Possibly overlapping classes of something. Trying to figure it out.

Currently did exchange doesn't work with the plugin.

@jamshale
Copy link
Contributor Author

I think this fixes it. Looks for an oob invitation and serializes it with the OOBInvitation class.

Added some development stuff.

Going to see if the tests pass now in OATH.

@jamshale
Copy link
Contributor Author

All the OATH tests pass with this branch so I think it's good to go... I admit I don't fully understand the data flow.

@jamshale jamshale requested a review from swcurran January 29, 2025 19:37
@@ -9,7 +9,7 @@ python = "^3.12"

# Define ACA-Py as an optional/extra dependency so it can be
# explicitly installed with the plugin if desired.
acapy-agent = { version = "~1.2.1", optional = true }
acapy-agent = { git = "https://github.com/openwallet-foundation/acapy.git", branch = "main", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to keep this change? I assume we would want the version vs. main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version 1.2.1 doesn't have the v1 connections protocol removed. This would be updated when the next release happens.

@@ -24,7 +24,7 @@ pytest-ruff = "^0.4.1"
black = "~24.4.2"

[tool.poetry.group.integration.dependencies]
aries-askar = { version = "~0.3.2" }
aries-askar = { version = "~0.4.2" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Update to 0.4.3 already, or hold that to update them all together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll update it to 0.4.3 after acapy main gets the change, because it's pointing to main. The rest can wait until the next acapy release.

I'm not entirely sure why this is here though. This was done a long time ago. If the plugin doesn't explicitly use askar independently I don't think this should be needed. I'll try and look into removing this integration group for all plugins.

@swcurran
Copy link
Contributor

A couple of questions.

@dbluhm
Copy link
Contributor

dbluhm commented Jan 29, 2025

I'm working on parsing this... Loading the protocol is needed but I thought the presence of the definition.py file would trigger that. I want to understand what's going on with the retrieve_invitation call as well before I call this good. Do you have a link to a test run that failed before introducing that particular change?

@jamshale
Copy link
Contributor Author

I'm running it again... Will post when it completes.

@jamshale
Copy link
Contributor Author

Yes the plugin definition file should have loaded the didcomm message types. It does this in firebase_push_notifications. However, the config it uses is plugin: firebase_push_notifications.v1_0. I noticed when I took this over it was done connections.v1_0 in the integration.yml and connections in default.yml. But I could only get it loading with connections. Maybe something in the structure is a tiny bit off?

@jamshale
Copy link
Contributor Author

bob_agent.log

Big file. But you can see the inviter get this stack trace whenever it uses oob connection.

�[0m2025-01-29 20:23:10,086 acapy_agent.messaging.models.base ERROR ConnectionInvitation message validation error:
Traceback (most recent call last):
File "/usr/local/lib/python3.12/site-packages/acapy_agent/messaging/models/base.py", line 196, in deserialize
schema.loads(obj) if isinstance(obj, str) else schema.load(obj),
^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 789, in load
return self._do_load(
^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 996, in _do_load
raise exc
marshmallow.exceptions.ValidationError: {('did', 'recipientKeys', 'serviceEndpoint'): ['Missing required field(s)']}
2025-01-29 20:23:10,088 acapy_agent.core.dispatcher ERROR Handler error: ConnectionManager.get_connection_targets
Traceback (most recent call last):
File "/usr/local/lib/python3.12/site-packages/acapy_agent/messaging/models/base.py", line 196, in deserialize
schema.loads(obj) if isinstance(obj, str) else schema.load(obj),
^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 789, in load
return self._do_load(
^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 996, in _do_load
raise exc
marshmallow.exceptions.ValidationError: {('did', 'recipientKeys', 'serviceEndpoint'): ['Missing required field(s)']}

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/usr/local/lib/python3.12/asyncio/tasks.py", line 314, in __step_run_and_handle_result
result = coro.send(None)
^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/connections/v1_0/manager.py", line 108, in get_connection_targets
targets = await self.fetch_connection_targets(connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/connections/v1_0/manager.py", line 152, in fetch_connection_targets
return await self._fetch_targets_for_connection_in_progress(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/connections/v1_0/manager.py", line 186, in _fetch_targets_for_connection_in_progress
invitation = await connection.retrieve_invitation(session)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/connections/v1_0/models/conn_record.py", line 46, in retrieve_invitation
return ConnectionInvitation.deserialize(ser)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/acapy_agent/messaging/agent_message.py", line 438, in deserialize
return super().deserialize(value, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/acapy_agent/messaging/models/base.py", line 200, in deserialize
raise BaseModelError(f"{cls.name} schema validation failed") from err
acapy_agent.messaging.models.base.BaseModelError: ConnectionInvitation schema validation failed
2025-01-29 20:23:10,090 acapy_agent.core.dispatcher ERROR Handler error: upgrade_middleware
Traceback (most recent call last):
File "/usr/local/lib/python3.12/site-packages/acapy_agent/messaging/models/base.py", line 196, in deserialize
schema.loads(obj) if isinstance(obj, str) else schema.load(obj),
^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 789, in load
return self._do_load(
^^^^^^^^^^^^^^
File "/usr/local/lib/python3.12/site-packages/marshmallow/schema.py", line 996, in _do_load
raise exc
marshmallow.exceptions.ValidationError: {('did', 'recipientKeys', 'serviceEndpoint'): ['Missing required field(s)']}

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.

3 participants