web/selectors/socket-selectors.js
63–64 ↗	(On Diff #23651)	This seems like a really significant change. Tons of questions pop up... Are we changing the format of this in the database? Do we need a migration? If I recall correctly, this format needs to be kept consistent in a lot of places. Where else needs to be updated? Do we have a task to track this? Looking at the MySQL database on my personal keyserver, it appears like "Public" has been included previously, so perhaps this is just you fixing up an error that never got to prod? (I hope so...) Separately, high-level meta-point: you should really expect your reviewer to call you out for things like this. It will save everyone some time if you annotate changes like this ahead-of-time.

This revision now requires changes to proceed.Mar 13 2023, 9:54 AM

atul requested review of this revision.Mar 13 2023, 10:47 AM

atul added inline comments.

web/selectors/socket-selectors.js
63–64 ↗	(On Diff #23651)	Are we changing the format of this in the database? Do we need a migration? No If I recall correctly, this format needs to be kept consistent in a lot of places. Where else needs to be updated? Do we have a task to track this? The keys in Redux `CryptoStore` are `primaryIdentityKeys` and `notificationIdentityKeys`. Those can be changed in a migration to `primaryIdentityPublicKeys` and `notificationIdentityPublicKeys` for consistency. I have a diff locally to make this exact change. Looking at the MySQL database on my personal keyserver, it appears like "Public" has been included previously, so perhaps this is just you fixing up an error that never got to prod? (I hope so...) Yes, this was an issue that was caught locally and never got to prod. The selector is being used for the first time in this diff and the issue was found in the course of testing thanks to the validators on the `keyserver` side. Separately, high-level meta-point: you should really expect your reviewer to call you out for things like this. It will save everyone some time if you annotate changes like this ahead-of-time. Sure