-
-
Notifications
You must be signed in to change notification settings - Fork 175
identity-service-api: Add 3PID bind endpoint. #621
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
Conversation
crates/ruma-identity-service-api/src/association/bind_3pid/v2.rs
Outdated
Show resolved
Hide resolved
crates/ruma-identity-service-api/src/association/bind_3pid/v2.rs
Outdated
Show resolved
Hide resolved
crates/ruma-identity-service-api/src/association/bind_3pid/v2.rs
Outdated
Show resolved
Hide resolved
Use borrowed types and unwrap and fit comment on a single line.
iinuwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good: I have a couple of changes for you.
(And sorry for the longer response time: I'll try to get to your PRs within 24-48 hours usually. If you want, you can feel free to submit multiple PRs at once. (Maybe in different modules so no rebasing is needed when we merge them.)
| pub mxid: UserId, | ||
|
|
||
| /// A UNIX timestamp before which the association is not known to be valid. | ||
| pub not_before: UInt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For time-based fields, we use the types from ruma_common::time. From the examples given in the spec, and the note here, this looks like it represents milliseconds. can you use the ruma_common::time::MillisecondsSinceUnixEpoch struct? Same with the other two below.
(Note to self: add references to these structs to the contributors guide.)
crates/ruma-identity-service-api/src/association/bind_3pid/v2.rs
Outdated
Show resolved
Hide resolved
crates/ruma-identity-service-api/src/association/bind_3pid/v2.rs
Outdated
Show resolved
Hide resolved
Derive Serialize and Deserialize for ServerSignatures.
jplatte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will let iinuwa re-review, but I only have one more style nit.
Resolves: #330
I've used
pub signatures: BTreeMap<ServerNameBox, BTreeMap<ServerSigningKeyId, String>>but I feel like there might be a better type for that. Maybe something similar to ServerSignatures?