Verification event improvements

[poljar]

The first commit introduces the new key agreement method, relevant MSC can be found here and the accompanied spec update is here.

The second commit just exposes the content fields of the start event publicly, as the event is otherwise useless to client libraries and clients.

[jplatte]

Thanks! Just two small nits:

[jplatte]

Oh, also the constructor needs to be updated. As pointed out in the spec PR, it introduced a breaking change in not requiring the curve25519 method to always be present anymore (I really wonder why this was required at all before).

[poljar]

If you mean in Ruma, probably because the spec says so over here

Must include at least curve25519.

Why the spec mentions this 🤷.

There's some other inconsistency with the events, e.g. the start event has an enum as the content which depends on the verification method (m.sas.v1) while the accept event has the method as a field in the content, should we make this consistent as well while I'm at it?

[jplatte]

If you mean in Ruma

I meant in the spec.

Why the spec mentions this 🤷.

Yeah, I've commented over at the spec PR about maybe not doing the same thing again.

EDIT: Actually, in a way this ship has sailed because the MSC says "curve25519-hkdf-sha256 is introduced, and will be mandatory for clients to support when performing SAS verification".

There's some other inconsistency with the events, e.g. the start event has an enum as the content which depends on the verification method (m.sas.v1) while the accept event has the method as a field in the content, should we make this consistent as well while I'm at it?

This may be just us mirroring the spec, but if it's not and if both events seem equally likely to have method-specific data (or already do), making them uniform would be great! (if one of these does not hold, we may want to re-evaluate)

[poljar]

Unless some other event type will be used to accept different verification methods I would say that the accept event will as well have method specific data.

On the other hand one of the other known methods doesn't use the start event at all, over here.

I guess to be on the safe side we should allow method specific data.

[poljar]

Not sure what the nightly is on about, seems unrelated to my changes. I updated the PR now, hopefully I didn't miss anything.

I'll leave the accept event updating for another PR. There's also a bunch of other work to do with these (in room variants for those, more event types verification.request/ready...), which will I also need to tackle.

[jplatte]

While we're stricly speaking not compatible with either version of the spec like this, I really like this solution.

[jplatte]

I am surprised rustfmt doesn't complain about this. Can you make it use block indentation since generally the standard formatting for Rust is never visual indentation?

So probably

Suggested change

	return Err(InvalidInput("`key_agreement_protocols` must contain at \
	least `KeyAgreementProtocol::Curve25519` or \
	`KeyAgreementProtocol::Curve25519HkdfSha256`".into()));
	return Err(InvalidInput(
	"`key_agreement_protocols` must contain at least \
	`KeyAgreementProtocol::Curve25519` or \
	`KeyAgreementProtocol::Curve25519HkdfSha256`".into(),
	));

(but maybe if you write that rustfmt changes something up again)

Same for the two error messages below.

[poljar]

It did complain and I ran cargo fmt, or am I missing something about this? Github claims that the suggestion is outdated so it's probably fine now.

[jplatte]

Oh, seems GitHub just showed me an outdated diff 🤦

[jplatte]

Not sure what the nightly is on about, seems unrelated to my changes.

Yes, just a new clippy lint that was introduced whose suggestion I'll apply separately from this PR :)

I'll leave the accept event updating for another PR. There's also a bunch of other work to do with these (in room variants for those, more event types verification.request/ready...), which will I also need to tackle.

👍

[jplatte]

Rebased and merged, thanks!