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

feat: Basic Messages V2 for DIDComm V2 #1546

Draft
wants to merge 8 commits into
base: feat/didcomm-v2
Choose a base branch
from

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Aug 17, 2023

Refactor Basic Messages modules to support both V1 and V2 protocols. At the moment, in order to not introduce breaking changes in API level, when messages are sent, the framework determines whether to use 1.0 or 2.0 according to connection's DIDComm version.

Some remaining work (either for a further PR or to discuss here):

  • Add protocolVersion record for BasicMessageRecord. This can be optional and make permanent when making a new release (migration script included)
  • Analyze how to support DIDComm V1 for Basic Messages 2.0, as the protocol might work with both versions. This will apply also for Discover Features 2.0 and others

Edit: Marked as Draft again as it will be dependent on #1576 and support Basic Messages V2 in both DIDComm V1 and DIDComm V2

@genaris genaris marked this pull request as ready for review August 19, 2023 22:13
@genaris genaris requested a review from a team as a code owner August 19, 2023 22:13
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Renaming basic message to v1basicmessage seems like a breaking change? Maybe we can export both with and without prefix (alias) and remove without prefix in next release


@injectable()
export class BasicMessagesApi {
private basicMessageService: BasicMessageService
export class BasicMessagesApi<BMPs extends BasicMessageProtocol[]> implements BasicMessagesApi<BMPs> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's needed to register the protocols dynamically in this case? It's consistent with issue and prove, but those have reasons to not support e.g. v1 (anoncreds only).

But if both v1 and v2 are registered by default, there's no reason to not make it configurable i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern was also followed in message pickup, I think mainly to allow:

  • Adding new protocol versions (or override behaviour of some of them) without the need of changing AFJ core
  • Creating 'pure DIDComm V2' agents where only the newer protocols are supported (this is the case if you are using a Wallet that does not support legacy DIDComm packing/unpacking). Also pure DIDComm V1 agents (e.g. Indy SDK or a browser wallet like this one)

For the second case, I think it will be useful to think about a "BaseProtocol" class/interface that forces protocols to indicate DIDComm versions where it is applicable for, and probably the same for Wallet (declare DIDComm versions supported), in order to register protocols only if the wallet is capable of handling messages for it.

@genaris
Copy link
Contributor Author

genaris commented Aug 21, 2023

Renaming basic message to v1basicmessage seems like a breaking change? Maybe we can export both with and without prefix (alias) and remove without prefix in next release

Do you think I'd need also to duplicate BasicMessageStateChangedEvent? It's payload.message property is now
V1BasicMessage | V2BasicMessage. In V2BasicMessage I've added some getters with the same behaviour of the ones from V1BasicMessages so should be fine in most handlers, but it is strictly not the same type.

@TimoGlastra
Copy link
Contributor

TimoGlastra commented Aug 28, 2023

Do you think I'd need also to duplicate BasicMessageStateChangedEvent? It's payload.message property is now
V1BasicMessage | V2BasicMessage. In V2BasicMessage I've added some getters with the same behaviour of the ones from V1BasicMessages so should be fine in most handlers, but it is strictly not the same type.

Yeah this is getting into complexities. I'm not sure ... Technically it's a breaking change, and it can result in issues in existing deployments. If we want to be on the safe side, we should push this in 0.5. If you don't use v2 it will never actually contain it in the event, but TS doesn't care about that...

So yes, maybe we should create an event type with only v1 message, and another one with both. Then in the next version we remove the old one in favour of the new one. Then we don't have a breaking change :)

@genaris
Copy link
Contributor Author

genaris commented Aug 28, 2023

Yeah this is getting into complexities. I'm not sure ... Technically it's a breaking change, and it can result in issues in existing deployments. If we want to be on the safe side, we should push this in 0.5. If you don't use v2 it will never actually contain it in the event, but TS doesn't care about that...

So yes, maybe we should create an event type with only v1 message, and another one with both. Then in the next version we remove the old one in favour of the new one. Then we don't have a breaking change :)

I think if we stick to this 'non-breaking' strategy, for the moment we can simply avoid touching anything in BasicMessageEvents and in V2BasicMessageProtocol not emit any specific event at all: we can simply rely on AgentProcesssedEvent in the demo and tests until we do a proper breaking change implementation with all pending issues solved (including API changes to specify protocol version, allow V2BasicMessage in DIDCommV1, etc.).

Might that work for the moment?

@genaris genaris marked this pull request as draft September 16, 2023 18:40
@TimoGlastra TimoGlastra added this to the 0.6 milestone Feb 14, 2024
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.

2 participants