Page MenuHomePhabricator

[lib] Replace ashoatKeyserverID with authoritativeKeyserverID
ClosedPublic

Authored by inka on Feb 9 2024, 2:09 AM.
Tags
None
Referenced Files
F3368241: D11006.diff
Mon, Nov 25, 6:52 PM
Unknown Object (File)
Fri, Nov 22, 3:51 PM
Unknown Object (File)
Fri, Nov 22, 3:17 PM
Unknown Object (File)
Fri, Nov 22, 3:11 PM
Unknown Object (File)
Fri, Nov 22, 2:55 PM
Unknown Object (File)
Fri, Nov 22, 2:53 PM
Unknown Object (File)
Fri, Nov 22, 2:39 PM
Unknown Object (File)
Thu, Nov 21, 1:04 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-6635/have-ashoatkeyserverid-fetch-is-value-from-a-config
Replacing ashoatKeyserverID with authoritativeKeyserverID, which is fetched from config, based on a platform.
Because keyserver depends on web, authoritative-keyserver.js is run on the keyserver. On the keyserver config is not set, so we have to work aroud that. For now the keyserver shouldn't ever use authoritativeKeyserverID. It could in the future, but it's outside of the scope of this task.
jest tests also need authoritativeKeyserverID, and they also don't register configs. We use a default value for them.

Test Plan

Tested that web and native can be run and no erros show up, tested that no errros show up on keyserver console.
Run jest tests and teset that they work and pass
Tested that it is possible to send messages, recieve them, create entries, fetch entries, log out, log in, and no errors show up

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 9 2024, 2:20 AM
Harbormaster failed remote builds in B26684: Diff 36898!
inka requested review of this revision.Feb 9 2024, 7:36 AM
lib/socket/activity-handler.react.js
18 ↗(On Diff #36929)

My diffs (D11012 and D11013) don't handle the "Handlers"... arguably those should get the keyserverID from the Socket too

We could land this diff as-is for those files, but arguably it would be easy (and better) to have Socket pass in keyserverID and to use that here

tomek requested changes to this revision.Feb 12 2024, 1:12 AM
tomek added inline comments.
lib/socket/activity-handler.react.js
18 ↗(On Diff #36929)

We could land this diff as-is for those files, but arguably it would be easy (and better) to have Socket pass in keyserverID and to use that here

I think it's better to keep the scope well-defined - that makes e.g. code review easier. For the handlers, there are some tasks defined https://linear.app/comm/issue/ENG-5776/update-socket-handlers which cover updating these.

lib/utils/authoritative-keyserver.js
8–10 ↗(On Diff #36929)

Keyserver shouldn't use authoritativeKeyserverID,

I don't think that's the case. getInitialReduxStateResponder uses ashoatKeyserverID which should become authoritativeKeyserverID

This revision now requires changes to proceed.Feb 12 2024, 1:12 AM

Rebase - remove changes from Socket, because D11013 replaced ashoatKeyserverID with props.keyserverID

lib/utils/authoritative-keyserver.js
8–10 ↗(On Diff #36929)

Actually, that will be replaced by a call to fetchIdentityInfo in ENG-6633
Also I think that would be logically incorrect, because a keyserver wants to return their values, so they should put them under their own id, not the authoritative keyservers id

This revision is now accepted and ready to land.Feb 12 2024, 3:17 AM

Rebase and update new usages of ashoatKeyserverID

Replace with function call - this is needed to ensure the value is read after the config is set.

inka requested review of this revision.Feb 20 2024, 4:20 AM
tomek added inline comments.
lib/reducers/keyserver-reducer.test.js
43 ↗(On Diff #37347)
This revision is now accepted and ready to land.Feb 21 2024, 1:30 AM
lib/utils/authoritative-keyserver.js
9 ↗(On Diff #37347)

This is a little bit fragile - we don't enforce this relationship. Maybe we can add an invariant

invariant(!process.env['KEYSERVER'], '...')

Or maybe we can check this condition instead of just hasConfig

if (process.env['KEYSERVER'] || !hasConfig())
lib/utils/authoritative-keyserver.js
14–16 ↗(On Diff #37392)

Does keyserver use this when it does server-side rendering of web?

lib/utils/authoritative-keyserver.js
14–16 ↗(On Diff #37392)

Hmm I didn't experience the above invariant trowing, so I don't think so. And I was able to login to the app and send messages using this code.
Based on D5983 it looks like we are only rendering the loading screen.

This comment made sense when we were calling this function top level, it doesn't anymore.
This file is being read on the keyserver (if I add a top level console.log in this file, it shows up in the keyserver console), I am not sure why, but the function should not be called