This diff introduces the create_or_update_farcaster_channel_tag endpoint so that we can call the createOrUpdateFarcasterChannelTag function we just introduced from the client
Depends on D11808
Details
Details
In my local stack I was able to call the create_or_update_farcaster_channel_tag endpoint and use createOrUpdateFarcasterChannelTag function from the client
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 17 ↗ | (On Diff #39474) | The farcasterChannelID looks like memes, crypto, neynar for example, and are better typed as t.String rather than tID |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 17 ↗ | (On Diff #39474) | We should decide if it is tID by checking if it is something specific to a keyserver and if a client should prefix it with the keyserver ID. In this case, it seems like we don't need to prefix, right? |
| 23 ↗ | (On Diff #39474) | Why do we include a holder in a response? |
| 26 ↗ | (On Diff #39474) | Should we expose this endpoint without handling permissions? |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 23 ↗ | (On Diff #39474) | |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 17 ↗ | (On Diff #39474) | No we don't so that's why I think t.String is better here |
| 23 ↗ | (On Diff #39474) | We are returning the blob holder here so that we can append the blob holder to the communityInfo in the community client db store as an intermediary step until I start my next project where I will be introducing a mariadb communities table in the keyserver. https://linear.app/comm/issue/ENG-7804/append-farcastertagblobholder-field-to-communityinfo Once I have introduced the new mariadb communites table on the keysever we will remove the blob holder as part of the response for this function which are tracked here: |
| 26 ↗ | (On Diff #39474) | We will include the permissions to this endpoint as part of https://linear.app/comm/issue/ENG-7648/introduce-a-new-permission-for-updatingmanaging-connecting-farcaster which is a task in my next project where I introduce the mariadb community table to the keyserver Regarding the concerns about exposing this endpoint w/o handling permissions I can add a isDev check and if for some reason anybody that's not a dev tries to cal this endpoint we will just immediately throw an error |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 23 ↗ | (On Diff #39474) | This seems like it's creating more work for us to do later. You'll be putting the blob holder in the communityInfo on clients, which forces us to have to later write a migration to clear it out. Is this really the best direction? |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 23 ↗ | (On Diff #39474) | Some context in this comment. @ginsu, are you planning to wait on landing this whole project until you conclude the following project? |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 23 ↗ | (On Diff #39474) | Given that everyone's client just has an empty store for communityStore, I was just planning on landing it as is since we don't have to worry about migrations since the communityStore is empty  I was able to populate my communityStore in my dev environment by patching in D11167. One direction we can go is to just not land the diffs for appending the blobHolder in the communityInfo since it will just get removed shortly afterwards in a subsequent diff |
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 23 ↗ | (On Diff #39474) | Following up to the comment above: Will link the diff below that introduces this gate when that is in review |
Comment Actions
Discussed this with @ginsu offline and he indicated he wanted to land this now, so I suggested gating the logic so that we're sure that Farcaster channels are not currently tagged in production, so that we can make sure we have no blob holders that need to be cleared from clients later.
I guess @ginsu's reason for all of this is that he wants to make it possible to delete Farcaster channel tags from clients while we're in this intermediate state. But if we're not launching it to production, I guess I'm not entirely sure how important it is to support deleting Farcaster channel tags on clients.
Not sure it makes sense to block the diff on this, but curious for @tomek's thoughts.
EDIT
It actually looks like we don't delete Farcaster channel tags from client – I can see in D11813 that the keyserver does the actual deletion. It looks like we're just having the keyserver return the blob holder to the client, and then having the client persist it, and then send it back to the keyserver. Seems a bit roundabout, but most of the code is already implemented.
Comment Actions
Not going to block it, but the approach doesn't make too much sense to me. Introducing something just to be removed a little later could be avoided by a better sequencing of work.
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 26 ↗ | (On Diff #39474) | Adding this check might help, so let's add it. But overall, a better approach would be to work on the endpoint code first, in a couple of diffs, and later in the last diff, expose it in endpoints.js. That might require having some local changes in order to be able to test. |
Comment Actions
Going to land this diff now, but overall agree with the feedback regarding that reviewing this work would be easier to follow if I initially sequenced the work better. Will make sure that next time I introduce endpoints or work more in the keyserver code I think more critically on this.
| keyserver/src/responders/farcaster-channel-tag-responders.js | ||
|---|---|---|
| 26 ↗ | (On Diff #39474) | In D11808, we gate the entire endpoint so it will immediately throw a server error regardless if you are a dev or not which should be a "stricter" check than isDev |