Page MenuHomePhabricator

[Keyserver/rust] Add publish_prekeys implementation
ClosedPublic

Authored by jon on Jul 11 2023, 9:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 6, 7:02 AM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:59 PM
Unknown Object (File)
Sat, May 4, 4:58 PM
Subscribers

Details

Summary

Implement the rust bindings for publishing prekeys to
identity service.

Part of https://linear.app/comm/issue/ENG-3944

Depends on D8464

Test Plan

Tested in a later diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/addons/rust-node-addon/src/identity_client/prekey.rs
30–38 ↗(On Diff #28588)

I understand that these unwraps are temporary until getCommConfig is implemented

Fix accidental rebase error

varun requested changes to this revision.Jul 11 2023, 2:06 PM
varun added inline comments.
keyserver/addons/rust-node-addon/build.rs
8 ↗(On Diff #28596)

mentioned this in another diff but i think we can just have one .proto file

keyserver/addons/rust-node-addon/rust-binding-types.js
39–45 ↗(On Diff #28596)

these should be camelCase

keyserver/addons/rust-node-addon/src/identity_client/prekey.rs
24–28 ↗(On Diff #28596)

we already have a helper function get_identity_service_channel that does this. can you please take a look at some of the other client methods in rust-node-addon?

34–37 ↗(On Diff #28596)

why do we use append instead of insert?

30–38 ↗(On Diff #28588)

we'll still have to call the parse method then, right?

This revision now requires changes to proceed.Jul 11 2023, 2:06 PM
jon marked 6 inline comments as done.

Address feedback, refactor auth client into self-contained module

keyserver/addons/rust-node-addon/build.rs
8 ↗(On Diff #28596)

We had a comm discussion, decided to eventually separate the two.

keyserver/addons/rust-node-addon/rust-binding-types.js
39–45 ↗(On Diff #28596)

oops

keyserver/addons/rust-node-addon/src/identity_client/prekey.rs
24–28 ↗(On Diff #28596)

I tried this initially with just trying to pass the interceptor as a function, and it was pretty gnarly with the types, and I chose not to.

However, going the struct + impl route was a lot easier, so done.

34–37 ↗(On Diff #28596)

first example I ran across used it, but insert is a destructive insertion, so it's closer to what I would want anyway.

Thanks

30–38 ↗(On Diff #28588)

we'll still have to call the parse method then, right?

yes, it's because metadata is a MetadataMap<Ascii>, so the utf8 string has to be parsed into just the ascii subset of allowed characters.

I understand that these unwraps are temporary until getCommConfig is implemented

Handling the error is still necessary, just did unwrap() while I had failing tests. I'll do a proper handling function and address this.

varun requested changes to this revision.Jul 17 2023, 11:06 PM

it seems like some of my feedback wasn't addressed, maybe a rebase issue?

This revision now requires changes to proceed.Jul 17 2023, 11:06 PM

Apply missed feedback during refactor

This revision is now accepted and ready to land.Jul 22 2023, 10:02 AM