Details

keyserver/src/creators/farcaster-channel-tag-creator.js
28 ↗	(On Diff #40027)	Removed this since when I was working I found myself confused with if I was using the `farcasterChannelID` from the `request` or the `farcasterChannelID` from the community info I just fetched from `fetchCommunityInfosPromise`
lib/actions/community-actions.js
51 ↗	(On Diff #40027)	Initially we were returning the `blobHolder` so that we can temporarily store it in redux; however, now that we can store the `blobHolder` in the communities table on the keyserver, we shouldn't return this back to the client

Harbormaster completed remote builds in B28813: Diff 40027.May 9 2024, 9:44 AM

ginsu requested review of this revision.May 9 2024, 9:44 AM

ginsu added a child revision: D11978: [keyserver/lib] update deleteFarcasterChannelTag to use communites table.May 9 2024, 11:02 AM

ashoat added inline comments.May 9 2024, 4:08 PM

keyserver/src/creators/farcaster-channel-tag-creator.js
39–42 ↗	(On Diff #40027)	I would inline these
78–82 ↗	(On Diff #40027)	Alignment issues
87–88 ↗	(On Diff #40027)	Can these be named `oldChannelID` and `oldHolder` for clarity?

tomek accepted this revision.May 10 2024, 4:04 AM

tomek added inline comments.

keyserver/src/creators/farcaster-channel-tag-creator.js
77–88 ↗	(On Diff #40027)	There's a slight edge case here - values in the DB could have changed between fetch and update. It would be ideal if this update query would return the values from before the update. Maybe it is something a `RETURNING` clause could achieve?

This revision is now accepted and ready to land.May 10 2024, 4:04 AM

ginsu added inline comments.May 22 2024, 7:10 AM

keyserver/src/creators/farcaster-channel-tag-creator.js
77–88 ↗	(On Diff #40027)	This was pointed out in D11978, but MariaDB doesn't support RETURNING with UPDATE statements https://mariadb.com/kb/en/update/. In D11978, one of the alternative solutions that was suggested was to use a transaction to help with this. I actually found that `encryptAndUpdateOlmSession` also uses the transaction approach so I updated the query to use the same approach as well. https://github.com/CommE2E/comm/blob/master/keyserver/src/updaters/olm-session-updater.js#L63-L85

address comments + use transaction approach

Tested everything end-2-end and everything still works as previously. However, would appreciate a second pair of eyes on the query as a sanity check + since I made some significant changes to query logic.

Harbormaster completed remote builds in B29168: Diff 40539.May 22 2024, 7:26 AM

kamil resigned from this revision.May 27 2024, 2:56 AM

tomek accepted this revision.May 27 2024, 4:53 AM

tomek added inline comments.

keyserver/src/creators/farcaster-channel-tag-creator.js
40 ↗	(On Diff #40539)	From the performance point of view, it would be better to query by ID instead of fetching all the communities. But given that there shouldn't be too many communities, I'm not sure if it's worth changing now.
75–84 ↗	(On Diff #40539)	Please fix the indentations

This revision is now accepted and ready to land.May 27 2024, 4:53 AM

ashoat added inline comments.May 28 2024, 12:03 PM

keyserver/src/creators/farcaster-channel-tag-creator.js
40 ↗	(On Diff #40539)	There might be 1000s of communities on my personal keyserver. I think it would be good to have the ability to query for a specific community, and it doesn't seem like it would be too complicated

ginsu added inline comments.Jun 5 2024, 10:35 AM

keyserver/src/creators/farcaster-channel-tag-creator.js
40 ↗	(On Diff #40539)	Going to land this diff as is (after fixing the indentation problem below) and will address this comment in a follow diff. Created this task to track: https://linear.app/comm/issue/ENG-8374/update-createorupdatefarcasterchanneltag-to-query-by-a-specific