Page MenuHomePhabricator

[keyserver] add community creation logic to farcaster webhook responder
ClosedPublic

Authored by will on Oct 22 2024, 9:44 PM.
Tags
None
Referenced Files
F3365944: D13775.id45445.diff
Mon, Nov 25, 8:35 AM
F3364271: D13775.id45528.diff
Mon, Nov 25, 3:49 AM
F3361705: D13775.id45485.diff
Sun, Nov 24, 6:58 PM
Unknown Object (File)
Fri, Nov 22, 5:29 PM
Unknown Object (File)
Fri, Nov 22, 5:29 PM
Unknown Object (File)
Fri, Nov 22, 5:28 PM
Unknown Object (File)
Wed, Nov 20, 3:18 AM
Unknown Object (File)
Tue, Nov 12, 3:49 PM
Subscribers

Details

Summary

This adds the logic for communtity creation to the farcaster webhook responder. We either decide to early return or continue to sidebar creation with the resulting channelCommunityID.

In the case that AUTHORITATIVE_FARCASTER_BOT is set, we create new communities if they haven't already been created and tagged through the blob service or default to the no channel community if no channel was specified.

Depends on D13834

Test Plan

I used rahul's testingthings channel for testing this. I created a webhook event by tagging myself with #createathread. I then copied this webhook event and used it to trigger the responder.

On staging, I created a dummy user with rahul's fid as well as a dummy user with my fid. In triggering the responder with authoritativeFarcasterBot set to true, confirmed that the community was created with rahul set to admin.

Reset everything to prior and ensured that if rahul's fid didn't exist on staging identity, retriggering the responder resulted in the same community but with the tagger (my fid) as the admin.

Verified community creation through iOS simulator. Properly set as tagged to farcaster testingthings channel

Retriggered responder to verify that it then picked up on the community already existing and returning the proper community threadID.

Depends on D13774

Diff Detail

Repository
rCOMM Comm
Branch
farcaster_bot
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ashoat requested changes to this revision.Oct 24 2024, 6:11 AM
ashoat added inline comments.
keyserver/src/responders/farcaster-webhook-responders.js
101–105

In my last review, I suggested doing this differently:

To make this work, we'd need some changes to createThread, so that it when it calls changeRole for these users, it passes a different role ID

I'd suggest replacing the definition as such:

// old
async function createThread(
  viewer: Viewer,
  request: ServerNewThinThreadRequest,
  options?: CreateThreadOptions,
): Promise<NewThreadResponse> {
  ...
}
// new
type CreateThreadRequest = $ReadOnly<{
  ...ServerNewThinThreadRequest,
  +initialAdminIDs?: ?$ReadOnlyArray<string>,
  +dontMakeViewerAdmin?: ?boolean,
}>;
async function createThread(
  viewer: Viewer,
  request: CreateThreadRequest,
  options?: CreateThreadOptions,
): Promise<NewThreadResponse> {
  ...
}

I was suggesting this approach because I wanted to avoid a robotext being printed about a user being promoted to admin. This matches the behavior introduced in D13300.

Can you walk me through why you decided to take a different approach here? I'm open to printing the robotext, but we should probably update joinThread too if that's the approach we want to take.

139

Do we need to wait until this point to start this promise?

141

We shouldn't need the ?? false here. It looks like you're using a single type in D13781 when you should be used two types

79–98 ↗(On Diff #45333)

Did this end up happening in a separate diff?

This revision now requires changes to proceed.Oct 24 2024, 6:11 AM
keyserver/src/responders/farcaster-webhook-responders.js
101–105

Got it! I think I understood your feedback to be that the problem was the lead/tagger being used in the create the community robotext. My thinking was that by having commbot be the user performing these actions in robotext, this concern would be mitigated.

On rereading your feedback, I think it would make sense to go with the approach you suggested

will marked 2 inline comments as done.Tue, Oct 29, 4:50 PM
will added inline comments.
keyserver/src/responders/farcaster-webhook-responders.js
79–98 ↗(On Diff #45333)

Rebased https://phab.comm.dev/D13756 to have a verification function

keyserver/src/responders/farcaster-webhook-responders.js
139

Understood this to mean we could introduce a Promise.all

will planned changes to this revision.Tue, Oct 29, 4:57 PM
will added inline comments.
keyserver/src/responders/farcaster-webhook-responders.js
101–105

Talked with Ashoat

101–105

^that got cut off. Going to rebase with the suggested changes

feedback and rebase

keyserver/src/responders/farcaster-webhook-responders.js
57 ↗(On Diff #45333)

This got taken care of with a function: getVerifiedUserIDForFID in https://phab.comm.dev/D13811

62–66 ↗(On Diff #45333)

Update to this thread. We create a community with commbot but also add commbot only as a viewer with the addViewerAsGhost option

65 ↗(On Diff #45333)

This was included in a rebase

ashoat requested changes to this revision.Wed, Oct 30, 2:50 PM
ashoat added inline comments.
keyserver/src/responders/farcaster-webhook-responders.js
49 ↗(On Diff #45485)

Why do you wait until the above two promises conclude before starting this one?

54–61 ↗(On Diff #45485)

Feels kinda messy. How about this instead?

const initialMemberIDs = [...new Set([
  leadUserID,
  taggerUserID,
  communityAdminID,
].filter(Boolean))];
145 ↗(On Diff #45485)
  1. Why do we wait until this point to start this promise?
  2. Do we end up needing this elsewhere? Wondering why it's here instead of being inside createTaggedFarcasterCommunity
147–148 ↗(On Diff #45485)

This comment from the prior review wasn't responded to:

We shouldn't need the ?? false here. It looks like you're using a single type in D13781 when you should be used two types

This revision now requires changes to proceed.Wed, Oct 30, 2:50 PM
keyserver/src/responders/farcaster-webhook-responders.js
49 ↗(On Diff #45485)

Discussed with Ashoat 1:1. Creating a promise variable and moving it upwards

54–61 ↗(On Diff #45485)

Included in next rebase

145 ↗(On Diff #45485)
  1. Creating a promise and moving it upwards
  2. Discussed 1:1 with Ashoat, we use it in creating the sidebar, https://phab.comm.dev/D13782
147–148 ↗(On Diff #45485)

Addressed in rebase

101–105

We

This revision is now accepted and ready to land.Wed, Oct 30, 6:35 PM
keyserver/src/responders/farcaster-webhook-responders.js
127–130 ↗(On Diff #45519)

@ashoat wanted to ask if we should rebase later diffs to include fields in this destruction syntax for code clarity.

Ex. much later in the diff, I do
const { hash: castHash, parent_hash: parentHash } = event.data;

and then use these variables in the very next line. However, from a code clarity standpoint, wondering if it'd be better to have them declared earlier even if we don't immediately use them.

keyserver/src/responders/farcaster-webhook-responders.js
127–130 ↗(On Diff #45519)

Talked about this offline – don't think it matters a ton, but personally I like to localize these things next to where they're used

add channel to deconstruct