[lib] Add action type to remove peer user
ClosedPublic
Actions

Authored by bartek on Aug 4 2024, 11:45 PM.

Details

Reviewers

inka
kamil
tomek

Commits

rCOMM20970f6988a2: [lib] Add action type to remove peer user

Summary

Similiar to D12961, but for relyingOnAuthoritativeKeyserve = false.
Added new action type that removes user IDs from both userStore and auxUserStore.
The action is dispatched when a peer receives message about account deletion.

Depends on D12961

Test Plan

The same as D12961, but this time faked relyingOnAuthoritativeKeyserver = false by reversing conditions for this flag in reducers.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Aug 4 2024, 11:45 PM

bartek held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptAug 4 2024, 11:45 PM

Harbormaster failed remote builds in B30880: Diff 43060!Aug 5 2024, 12:08 AM

bartek published this revision for review.Aug 5 2024, 2:27 AM

I think we can remove relyingOnAuthoritativeKeyserver from the condition, even if we still have an authoritative keyserver why not proceed with processing P2P messages as we should, in the worst case we attempt to delete the same peer twice, but wondering what @inka is thinking.

lib/reducers/user-reducer.js
548–559 ↗	(On Diff #43060)	I would inline those two as we don't call `findStoreInconsistencies` here

This revision is now accepted and ready to land.Aug 5 2024, 4:56 AM

I think we can remove relyingOnAuthoritativeKeyserver from the condition, even if we still have an authoritative keyserver why not proceed with processing P2P messages as we should, in the worst case we attempt to delete the same peer twice, but wondering what @inka is thinking.

I agree. Make sure to remove both places the condition is used

lib/reducers/aux-user-reducer.js
203 ↗	(On Diff #43060)	Shouldn't we be returning `removeUsersOps`?

This revision now requires changes to proceed.Aug 5 2024, 5:17 AM

Return missing ops, remove conditions

Inline unnecessary variable

inka accepted this revision.Aug 5 2024, 5:34 AM

This revision is now accepted and ready to land.Aug 5 2024, 5:34 AM

This revision was landed with ongoing or failed builds.Aug 5 2024, 5:54 AM

Closed by commit rCOMM20970f6988a2: [lib] Add action type to remove peer user (authored by bartek). · Explain Why

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMM20970f6988a2: [lib] Add action type to remove peer user.

Harbormaster failed remote builds in B30917: Diff 43097!Aug 5 2024, 6:45 AM

Harbormaster failed remote builds in B30918: Diff 43098!

ashoat added inline comments.Aug 5 2024, 11:24 AM

lib/reducers/aux-user-reducer.js
190 ↗	(On Diff #43107)	Confused why we don't check `relyingOnAuthoritativeKeyserver` here, but we do check for it in `user-reducer.js` below
lib/reducers/user-reducer.js
539 ↗	(On Diff #43107)	@inka said this in their review: I agree. Make sure to remove both places the condition is used Was this place missed?

In the initial revision (https://phab.comm.dev/D12962?id=43060) there were 3 places with relyingOnAuthoritativeKeyserver check:

before action dispatch
in AuxUserReducer
in UserReducer

Perhaps I misunderstood the feedback.
I was under impression that it should be removed from AuxUserReducer (my thinking when reading @kamil's feedback: "removing peer twice" -> peer list is stored in aux user store) so I removed "both conditions" (@inka's feedback) 1. and 2, leaving UserStore up to authoritative keyserver.