Feature: forward pack message #20
Conversation
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
didcomm_messaging/routing.py
Outdated
| packed_message = await self.packaging.pack( | ||
| json.dumps( | ||
| { | ||
| "typ": "application/didcomm-plain+json", |
There was a problem hiding this comment.
As we ended up discussing briefly earlier, the typ shouldn't be required in the message; does this cause other didcomm v2 implementations to fail if omitted though?
There was a problem hiding this comment.
If I leave it off, at present, both the mediator and diddy-bot fail to validate the message and throw the following exception:
MalformedMessageCode.INVALID_PLAINTEXT
It appears that https://github.com/decentralized-identity/didcomm-demo may be adding the typ field automatically when it is missing.
didcomm_messaging/routing.py
Outdated
| json.dumps( | ||
| { | ||
| "typ": "application/didcomm-plain+json", | ||
| "type": "https://didcomm.org/routing/2.0/forward", | ||
| "id": str(uuid.uuid4()), | ||
| # "frm": did, # Do we need this? Spec shows it missing in the example | ||
| "to": [service["did"]], | ||
| "body": {"next": next_target["did"]}, | ||
| "attachments": [ | ||
| { | ||
| "id": str(uuid.uuid4()), | ||
| "media_type": "application/didcomm-encrypted+json", | ||
| "data": { | ||
| "json": json.loads(packed_message), | ||
| }, | ||
| }, | ||
| ], | ||
| } | ||
| ), | ||
| [service["did"]], | ||
| ) |
There was a problem hiding this comment.
I think I'm mildly interested in seeing this be split out into a separate... something lol. Just so the structuring of the forward message isn't strongly tied to this method. Whether it's just a separate helper method or something more formal, I don't have strong feelings on at the moment.
There was a problem hiding this comment.
For now, I've split this out into a helper function. I would love to see a "forward settings" thing get passed down that allows us to expire forwarded messages (since it is part of the spec)
| if not await self.resolver.is_resolvable(to): | ||
| return [] |
There was a problem hiding this comment.
I'm not sure this is the right way to handle an unresolvable DID in a chain; should't this effectively render the message undeliverable rather than potentially stopping in the middle? We would end up delivering the message to a mediator in the middle of the chain... which might still work I'm not sure lol but I don't know if that's desirable behavior if the final recipient specifically intended it to start at a certain point.
There was a problem hiding this comment.
I believe that the idea was that we call down until we have nothing to route to anymore. In theory, it is possible for "to" to be an HTTP uri, which is not resolvable as a DID but is a valid destination. Returning an empty list effectively meant that we hit the end of the chain and we had no further wrapping to do.
There was a problem hiding this comment.
At least, that was my thought process when I first wrote it.
didcomm_messaging/__init__.py
Outdated
| def get_endpoint(self, protocol: str) -> str: | ||
| return self.get_service(protocol).service_endpoint.uri | ||
|
|
||
| def get_service(self, protocol: str) -> DIDCommV2Service: | ||
| return self.filter_services_by_protocol(protocol)[0] |
There was a problem hiding this comment.
I like this but I think it would be good to name the methods such that it's clear that you're going to just get the first matching endpoint.
There was a problem hiding this comment.
That's what I initially thought, but I also wanted it to be short and concise enough that it didn't seem like a complicated method 🤔
I already didn't like the filter_services_by_protocol method due to it's length, but I had no idea how to shorten it.... Now that I'm thinking about it, the parameter name should fulfill the "by_protocol" part of the name, so I could probably drop that.
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
|
Alright, with the latest changes, this PR should close #5! \o/ |
Signed-off-by: Colton Wolkins (Indicio work address) <[email protected]>
First pass at #5, completely forgot that there was mention in there about routing keys. This change properly follows all did urls via services blocks, however.
closes #5