[keyserver] Rework updateRelationships on keyserver to use new API
ClosedPublic
Actions

Authored by ashoat on Mon, Sep 23, 10:36 PM.

Details

Reviewers

tomek

Commits

rCOMMd1dd89e65588: [keyserver] Rework updateRelationships on keyserver to use new API

Summary

This diff updates updateRelationships to take the new API as input, and adds code for converting the old input into the new format to legacyUpdateRelationshipsResponder

Depends on D13437

Test Plan

I tested the API as part of my work on ENG-9331

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Mon, Sep 23, 10:36 PM

ashoat added a child revision: D13439: [keyserver] Only create robotext for relationship actions if createRobotextInThinThread is true.

Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Sep 23, 10:51 PM

Harbormaster failed remote builds in B31795: Diff 44461!

Rebase

Harbormaster completed remote builds in B31806: Diff 44472.Mon, Sep 23, 11:54 PM

ashoat requested review of this revision.Mon, Sep 23, 11:54 PM

tomek accepted this revision.Tue, Sep 24, 1:54 AM

tomek added inline comments.

lib/types/relationship-types.js
93 ↗	(On Diff #44472)	How about renaming it to `createThinThread`? This sounds less confusing, because `createRobotextInThinThread = false` could also be interpreted that we want to create a thin thread but without a robotext.

This revision is now accepted and ready to land.Tue, Sep 24, 1:54 AM

tomek added inline comments.Tue, Sep 24, 3:28 AM

lib/types/relationship-types.js
96 ↗	(On Diff #44472)	Are we supporting `farcaster` action? Shouldn't we include `userIDsToFID` for it?

ashoat added inline comments.Tue, Sep 24, 10:13 AM

lib/types/relationship-types.js
93 ↗	(On Diff #44472)	I dislike `createThinThread` for two reasons: We don't always create a thin thread. We only create one if it doesn't already exist. It doesn't mention creating robotext, which is arguably the main thing that we're controlling. The thin thread creation is more of a side effect / implied (if no thread exists, we need to create one for the robotext). This sounds less confusing, because `createRobotextInThinThread = false` could also be interpreted that we want to create a thin thread but without a robotext. I'm not sure I share this interpretation. For me, this is saying "don't create robotext in thin thread". I don't see how it could read to imply that a thin thread should be created.
96 ↗	(On Diff #44472)	D13436 updates our code to ignore this input from the user. Instead of trusting the user's claims about FIDs, we will instead query the identity service