Page MenuHomePhabricator

[native] implement getOutboundKeys method in CommRustModule
ClosedPublic

Authored by varun on Oct 2 2023, 8:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jul 4, 5:38 PM
Unknown Object (File)
Tue, Jul 2, 9:39 PM
Unknown Object (File)
Tue, Jul 2, 9:39 PM
Unknown Object (File)
Tue, Jul 2, 9:39 PM
Unknown Object (File)
Tue, Jul 2, 9:39 PM
Unknown Object (File)
Tue, Jul 2, 9:36 PM
Unknown Object (File)
Sun, Jun 30, 6:59 PM
Unknown Object (File)
Sun, Jun 30, 8:20 AM
Subscribers

Details

Summary

we need to get the keyserver's keys to set up an Olm session on the client.

Depends on D9345

Test Plan

successfully retrieved my local keyserver's keys by calling the CommRustModule method from js

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Oct 2 2023, 8:24 PM
kamil requested changes to this revision.Oct 3 2023, 11:05 AM

I have some questions, if those don't make sense I'll try to unblock this as soon as possible

lib/types/identity-service-types.js
11 ↗(On Diff #31602)

based on Rust types it's more like +socialProof: ?string but don't have a strong opinion on that

16–17 ↗(On Diff #31602)

Why this is not defined in the Rust struct? If this is different than struct OutboundKeyInfoResponse shouldn't be named differently to avoid confusion?

native/native_rust_library/src/lib.rs
652 ↗(On Diff #31602)

After looking on master this method is still not implemented, and don't see this in the stack - so not really sure how it was tested. Am I missing something?

659 ↗(On Diff #31602)

this is really confusing, isn't get() a better option?

native/schema/CommRustModuleSchema.js
54 ↗(On Diff #31602)

Based on Rust code looks like identifierType can have only two values, walletAddress and username, is that correct?

If yes - probably it will be safe to force those two types in Flow, so it should be identifierType: 'walletAddress' | 'username',.

I don't know but guessing codegen can't handle type unions - if that's true you can use a trick we use in different schemas with type for codegen and type exported for Flow, see UtilsModuleSpec.

This revision now requires changes to proceed.Oct 3 2023, 11:05 AM

address feedback

lib/types/identity-service-types.js
11 ↗(On Diff #31602)

makes sense

16–17 ↗(On Diff #31602)

fixed, thanks

native/native_rust_library/src/lib.rs
652 ↗(On Diff #31602)

sorry, meant to put that diff up first. it's up now as you've already seen :)

659 ↗(On Diff #31602)

we actually use this pattern a lot because get() only returns a reference. i'd prefer that we continue this way

native/schema/CommRustModuleSchema.js
54 ↗(On Diff #31602)

Looks good to me, thanks for addressing all comments

native/native_rust_library/src/lib.rs
659 ↗(On Diff #31602)

ahh right, looks good then

This revision is now accepted and ready to land.Oct 9 2023, 1:00 AM