E2e protocol ricardo 20231025 login sync #1780

corrideat · 2023-10-27T17:27:15Z

No description provided.

Silver-IT · 2023-10-30T05:57:30Z

Having checked this PR mainly focusing on the user flow, I could see several issues.

Issue 1

Steps to reproduce
- [Tab1] user1 signs up, and creates a group
- [Tab2] user2 joins the group, and closes the tab
- [Tab1] user1 kicks the u2 from the group
- [Tab2] Opens 'http://localhost:8000'. Got 5 errors in the console
- [Tab1] Got error in console too.
Screenshot
- Error in Tab1
- 5 Errors in Tab2
What should happen
- No errors should be happening. And the important thing is that there MUST be no errors in Tab1.
- gi.actions/group/join shouldn't be called. But it is called twice.
DeadLock Output
- [Tab1] {}
- [Tab2] {}

Silver-IT · 2023-10-30T06:27:22Z

Issue 2

Steps to reproduce
- [Tab1] user1 signs up, and creates a group
- [Tab2] user2 joins the group, and closes the tab
- [Tab1] user1 kicks the u2 from the group
- [Tab3] user2 logs in, and got 5 errors.
Screenshot
- 5 Errors in Tab2
What should happen
- No errors should be happening.
- gi.actions/group/join should be called ONLY once. But it is called twice.
DeadLock Output
- [Tab1] {}
- [Tab2] {}

Silver-IT · 2023-10-30T07:14:11Z

Issue 3

Steps to reproduce
- [Tab1] user1 signs up, and creates a group.
- [Tab2] user2 joins the group.
- [Tab1] user1 kicks the u2 from the group. Got 1 error in console.
- [Tab2] user2 joins the group again with the initial invite link.
- [Tab2] Wrong work flow to join. 6 Errors in console. Seems there are many redundant function calls.
- [Tab1] Many logs in console than expected. Seems there are too many function calls. And duplicated notifications.
Screenshot
- Error in Tab1 (Step 3)
- Errors in Tab2 (Step 5)
- Duplicated notifications in Tab1 (Step 6)
What should happen
- No errors should be happening.
- No duplicated notifications in Tab1
- gi.actions/group/join should be called ONLY once. But it is called 4 times.
DeadLock Output
- [Tab1]
- [Tab2] {}

Silver-IT · 2023-10-30T08:24:16Z

Issue 4

Steps to reproduce
- [Tab1] user1 signs up, and creates a group.
- [Tab2] user2 joins the group.
- [Tab3] user3 joins the group, and closes the tab.
- [Tab2] user2 creates a proposal to remove user3 from the group.
- [Tab1] user1 approves the proposal, and kicks user3 from the group. Got an error in the notifications.
- [Tab4] user3 tries to log in, and 8 errors BANG.
Screenshot
- Error in Tab1 (Step 5)
  (user1 = alexjin, user2 = greg, user3 = andrea)
- Errors in Tab4 (Step 6)
What should happen
- No errors should be happening.
- gi.actions/group/join should be called ONLY once. But it is called twice in Tab4 (Step 6).
DeadLock Output
- [Tab1]
- [Tab2] {}
- [Tab4] {}

Silver-IT · 2023-10-30T08:33:02Z

Issue 5

Steps to reproduce
- [Tab1] user1 signs up, and creates a group.
- [Tab2] user2 joins the group.
- [Tab3] user1 logs in.
- [Tab4] user2 logs in.
- [Tab1] user1 kicks user2 from the group.
- [Tab1], [Tab2], [Tab4] Errors BANG.
Screenshot
- Error2 in Tab1 (Step 6)
  (user1 = alexjin, user2 = greg)
- Errors in Tab2 (Step 6)
- Errors in Tab4 (Step 6)
What should happen
- No errors should be happening.
DeadLock Output
- [Tab1] {}
- [Tab2] {}
- [Tab3] {}
- [Tab4] {}

corrideat · 2023-11-03T17:56:50Z

Thanks again, @Silver-IT!

As discussed:

I've made side-effects mostly work as they did earlier
I've added a check on syncContractAndWatchKeys to avoid syncing contracts that have been subscribed to

In addition, I've implemented fixes for all of the issues reported here, except issue 3. I'll continue working on this until issue 3 is resolved.

Regarding issue 1, there is the double sync, that I've tracked down to the login functionality. I'll work on addressing this as well (but it seems to be the case that implementing the join|leaveGroup actions would help solving it too).

@taoeffect: You'll probably be pleased to see that I've mostly removed sync from recreateEvent, something you weren't entirely happy with. There are some vestiges of it that remain, because sometimes the pubsub functions don't work as expected and some events are never received. Once this is addressed in pubsub, the syncContract added to publishEvent can be removed.

corrideat · 2023-11-04T21:10:43Z

@Silver-IT I've made another commit addressing issue 3. The solution so far is using sbp('okTurtles.data/set', 'JOINING_CHATROOM-' + params.contractID, ?) to avoid removing the chatroom contract when joining.

Besides that, two issues remain.

One is at leaving; specifically, when someone else leaves a chatroom we have left:

To write to a contract, we need to be subscribed to it. This is done in encryptedAction, but for contracts that call /remove in a side-effect, this is essentially pointless. This is an error.

The second issue is at joining. You'll see that the /join action calls /sync. This is 'unnecessary', as encryptedAction already does this. However, similarly to what happened above, the first /sync will be pointless because of the side-effect removing the contract, requiring a second call to sync that sets the JOINING_CHATROOM-${cID} key.

I think that we need to either adapt encryptedAction to take pre- and post-hooks (we could call them, e.g., prologue and epilogue) to handle this, or somehow re-think how /remove gets called (I think that one thing that would work is doing reference counting, but there may be other options too). These are not mutually exclusive, so we could very well do both.

Related to this, there is a third issue, which is a variant of issue 3:

rejoin fails
- [tab1] u1: create group
- [tab2] u2: join using invite link
- [tab2] u2: close tab / navigate away
- [tab1] u1: remove u2
- [tab2] u2: open tab with invite link

To recap, apart from the issues mentioned, which are related to removing contracts, there is the double-sync issue I mentioned earlier.

Although double-syncs are a concern, I'll focus now on trying to address the actual issues that happen from missing contract states, which are errors.

Do you have a suggestion on how we could call /remove in an elegant way?

Silver-IT

Nice work, @corrideat. I did a very partial review, and left a just two comments.
I can see many updates in contract parts (model/contracts, controller/actions) but I don't think most of them are needed. I have almost finished my PR #1728 and I want you to have even a brief look at it. I fixed most of the login issues with the smaller changes than this.

Also will check the 5 Issues (described above) again, and left comments here.

Silver-IT · 2023-11-06T02:04:28Z

frontend/model/contracts/group.js

+        sbp('chelonia/queueInvocation', contractID, () => sbp('gi.contracts/group/leaveGroup', { data, meta, contractID }, { state, getters })).catch(e => {
+          console.error(`[gi.contracts/group/removeMember/sideEffect] Error ${e.name} during queueInvocation for ${contractID}`, e)
        })


We don't need to call gi.contracts/group/leaveGroup when the member has joined the group after this action.
So it would be necessary to check the group contract profile state in this invocation.
Please reference my PR for this.

sideEffect ({ data, meta, contractID }, { state, getters }) { const rootState = sbp('state/vuex/state') const rootGetters = sbp('state/vuex/getters') const contracts = rootState.contracts || {} const { username } = rootState.loggedIn if (data.member === username) { // If this member is re-joining the group, ignore the rest // so the member doesn't remove themself again. sbp('chelonia/queueInvocation', contractID, async () => { if (rootState[contractID].profiles?.[username]?.status === PROFILE_STATUS.REMOVED) { // NOTE: should remove archived data from IndexedStorage // regarding the current group (proposals, payments) await sbp('gi.contracts/group/removeArchivedProposals', contractID) await sbp('gi.contracts/group/removeArchivedPayments', contractID)

Silver-IT · 2023-11-06T23:34:38Z

frontend/model/contracts/group.js

      preSendCheck: (_, state) => {
        // Avoid sending a duplicate action if the person has already
        // left or been removed from the chatroom
        return !!state?.users?.[member]
      }


Inside group contract, it doesn't make sense to decide the next operation depending on the chatroom contract state.

Silver-IT · 2023-11-07T05:52:51Z

Issue 1 (#1780 (comment)) This issue is not fixed yet.

This screenshot shows the console logs filtered by gi.actions/group on Tab2 in Step 5 .

Problem: three errors and 2 redundant gi.actions/group/join function calls

Silver-IT · 2023-11-07T06:06:37Z

Issue 2 (#1780 (comment)) This issue is not fixed yet.

This screenshot shows the console logs filtered by gi.actions/group/join on Tab3 in Step 4.

Problem: 1 redundant gi.actions/group/join function call.

Silver-IT · 2023-11-07T06:21:56Z

Issue 3 (#1780 (comment)) seems not to be fixed yet.

Problem: two errors in console

taoeffect

Preliminary review

taoeffect · 2023-11-07T18:20:05Z

shared/domains/chelonia/GIMessage.js

+    } catch {
+      // Signature or encryption error
+      return undefined


No console.warn is needed here?

This is doing the same as was it was doing earlier, more or less. There could be a console.warn, but that is already done elsewhere and it feels repetitive to warn multiple times.

Then a comment about that should be here...

taoeffect · 2023-11-07T18:20:34Z

shared/domains/chelonia/GIMessage.js

@@ -407,7 +406,7 @@ export class GIMessage {
    return `${desc}|${this.hash()} of ${this.contractID()}>`
  }

-  isFirstMessage (): boolean { return !this.head().previousHEAD }
+  isFirstMessage (): boolean { return !this.head().contractID }


Why was this changed?

Because I removed a bunch of the repetitive code in chelonia.js that was looking up the previous HEAD, which is now done by publishEvent. Since the previous HEAD could now momentarily be null, the check as it was would fail. Note that either condition is correct (if the previous HEAD were set).

taoeffect · 2023-11-07T18:23:36Z

frontend/views/pages/PendingApproval.vue

@@ -36,7 +37,7 @@ export default ({
    ...mapState(['currentGroupId']),
    ourGroupProfile () {
      if (!this.ephemeral.groupIdWhenMounted) return
-      return this.$store.state[this.ephemeral.groupIdWhenMounted]?.profiles?.[this.ourUsername]
+      return this.$store.state[this.ephemeral.groupIdWhenMounted]?.profiles?.[this.ourUsername]?.status === PROFILE_STATUS.ACTIVE


This change changes the meaning of this computed property.

The computed property is named ourGroupProfile, which implies it returns our group profile. But now this no longer happens, instead it returns a boolean.

So either the condition needs to be removed or the name of the computed property needs to be changed.

taoeffect · 2023-11-07T18:27:45Z

frontend/common/errors.js

+
+export class GIErrorMissingSigningKeyError extends Error {
+  constructor (...params: any[]) {
+    super(...params)
+    // this.name = this.constructor.name
+    this.name = 'GIErrorMissingSigningKeyError' // string literal so minifier doesn't overwrite
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, this.constructor)
+    }
+  }
+}


I think this is a good time to copy over that utility function from Chelonia to define these using 1-liners...

taoeffect · 2023-11-07T18:49:55Z

shared/domains/chelonia/internals.js

+
+        // TODO: The [pubsub] code seems to miss events that happened between
+        // a call to sync and the subscription time. This is a temporary measure
+        // to handle this until [pubsub] is updated.
+        if (entry.height() === lastAttemptedHeight) {
+          await sbp('okTurtles.eventQueue/queueEvent', entry.contractID(), ['chelonia/private/in/syncContract', entry.contractID()])


So I agree that we should address this in this PR... one way to go about it is to make sure the pubsub is connected before 'chelonia/contract/sync' does the sync.

We might also want to verify that we are in fact connected to the pubsub server before allowing messages to be sent.

taoeffect · 2023-11-07T18:54:58Z

frontend/model/contracts/group.js

+            memberKey, member, profile, active: (profile: any)?.status === PROFILE_STATUS.ACTIVE
+          })
+          if (memberKey === member && (profile: any)?.status === PROFILE_STATUS.ACTIVE) {
+            return [memberKey, { status: PROFILE_STATUS.REMOVED }]


This line seems to be removing many profile fields... that seems wrong.

There isn't much in the profile and this is for removed members. Would you prefer { ...profile, status: PROFILE_STATUS.REMOVED } instead?

Yes, I think that would be much more robust.

taoeffect · 2023-11-07T18:57:04Z

frontend/model/contracts/group.js

+  // When a group is being left, we want to also leave chatrooms,
+  // including private chatrooms. Since the user issuing the action
+  // may not be a member of the chatroom, we use the group's CSK
+  // unconditionally in this situation, which should be a key in the
+  // chatroom (either the CSK or the groupKey)
+  if (leavingGroup) {
+    const signingKeyId = sbp('chelonia/contract/currentKeyIdByName', state, 'csk', true)
+
+    // If we don't have a CSK, it is because we've already been removed.
+    // Proceeding would cause an error


These are good comments 👍

taoeffect · 2023-11-07T18:58:02Z

frontend/model/contracts/group.js

+    }
+
+    extraParams.signingKeyId = signingKeyId
+    extraParams.innerSigningContractID = null


Does innerSigningContractID = null need to be specified? If yes, could you add a comment explaining why?

Yes, it needs to be specified or else it'll be set to the identity CSK.

frontend/model/contracts/group.js

taoeffect · 2023-11-07T19:15:43Z

shared/domains/chelonia/utils.js

+  const newOp = opT === GIMessage.OP_ATOMIC && (newRawOpV: any).length === 1
+    ? (newRawOpV: any)[0]
+    : [opT, newRawOpV]


What's going on here? Are you sure this is right? Is ? (newRawOpV: any)[0] correct?

Why is this being done? Is it rewriting what the user is sending? We should probably keep the developer intent and not modify messages the developer is sending.

taoeffect · 2023-11-07T19:21:07Z

shared/domains/chelonia/internals.js

      }
+      return Promise.resolve(sbp(`${manifestHash}/${action}/sideEffect`, mutation))


Why is this wrapping the result in Promise.resolve? What if an error occurred? What's the point of later checking .then and Promise.allSettled if they're guaranteed to settled because of the Promise.resolve?

Or is that not how it works? If so please add a comment explaining because this is confusing...

Silver-IT · 2023-11-09T12:27:19Z

Still existing issues:
- Issue 1(E2e protocol ricardo 20231025 login sync #1780 (comment))
- Issue 2 (E2e protocol ricardo 20231025 login sync #1780 (comment))
- Issue 3 (E2e protocol ricardo 20231025 login sync #1780 (comment))
- Issue 4 (E2e protocol ricardo 20231025 login sync #1780 (comment))
  Still two function gi.actions/group/join calls

* Pass params from sync to syncContract * Use fresh state in signData and encryptData * Check membership status in Join.vue

Silver-IT · 2023-11-10T00:31:40Z

@corrideat, @taoeffect. I have just checked the test scenario.

Still existing issues in terms of workflow

Issue 1
Problem:
- user2 has successfully logged in.
- One error occurred.
Issue 2
Problem:
- Two errors occurred.
- gi.actions/group/join is called twice. It should be called only once.
Issue 3
Problem:
- Error in [Tab2] after being kicked from the group. (Not before to try join)
- Three gi.actions/group/join calls. Only two calls are needed.
Issue 4
Problem:
- 4 errors in console in Tab4
- gi.actions/group/join is called twice. It should be called only once.
Issue 5
Seems fine.

(user1 = alexjin, user2 = greg, user3 = andrea)

IMHO, I don't approve the many updates in model/contracts and controllers/actions. Especially, I can't approve the way to leave chatroom when leaving group.

…itching to newly created chatroom)

taoeffect

This is looking really solid now, thanks @Silver-IT for finding those issues and @corrideat for figuring out the fixes! I think it's ready to merge into e2e-protocol but with also double-check with @Silver-IT to see if he can confirm that the issues he found have been fixed.

taoeffect · 2023-11-16T18:02:16Z

shared/domains/chelonia/internals.js

@@ -926,8 +969,8 @@ export default (sbp('sbp/selectors/register', {
      // of waiting, we need to remove it ourselves
      const keyId = findKeyIdByName(contractState, keyName)
      if (!keyId) {
+        console.log('@@@@ pendingWatch --- pushed to keysToDelete', { contractID, externalContractID, keyName, state: cloneDeep(contractState) })


Is this logging still needed?

corrideat added 3 commits October 25, 2023 19:09

Login sync fixes

45ee249

Fix various issues related to login and re-joining

f3bed17

Make saveOurLoginState async

abd54ab

corrideat requested review from taoeffect and Silver-IT October 29, 2023 01:37

Various improvments and bugfixes

889c43f

Bugfixes. Revert changes to side-effects

94188fe

corrideat marked this pull request as ready for review November 3, 2023 18:10

corrideat marked this pull request as draft November 3, 2023 18:11

bump

028513f

Address re-joining of chatrooms

9364fdf

corrideat force-pushed the e2e-protocol-ricardo-20231025-login-sync branch from c8cbd3b to 9364fdf Compare November 4, 2023 21:11

Silver-IT requested changes Nov 6, 2023

View reviewed changes

taoeffect requested changes Nov 7, 2023

View reviewed changes

taoeffect reviewed Nov 7, 2023

View reviewed changes

frontend/model/contracts/group.js Show resolved Hide resolved

taoeffect reviewed Nov 7, 2023

View reviewed changes

corrideat added 6 commits November 7, 2023 20:42

Address PR review feedback

51a9522

Deferred remove

65a6d74

Fixes to "chelonia/private/deleteRevokedKeys"

0596f30

Re-add sync to publishEvent

2a6d64d

Bugfix: reducer at deleteRevokedKeys

db5b43b

Test state.contracts[cID] instead of state[cID]

4f9f4ce

corrideat added 2 commits November 9, 2023 19:58

Bugfixes:

4f91d6b

* Pass params from sync to syncContract * Use fresh state in signData and encryptData * Check membership status in Join.vue

Move joining #General and adding group key into side-effect

9d47c5c

corrideat added 11 commits November 10, 2023 19:58

Bugfixes

d03fcc6

Clear deferredRemove flag

99f6b9d

Check if we have the decryption key first

b77a024

Updated error message

5db6ff1

Remove debug logs and fix multiple join calls

1695b6d

Remove unnecessary sync of identity contract

d461dde

Bugfixes

c67ac54

Fix UI inconsistency issues (wrong message when joining / leaving; sw…

43db5e1

…itching to newly created chatroom)

Encrypt OP_KEY_SHARE envelope

e680374

Add missing sync when added to chatroom

b103de9

Remove extraneous logging

c124023

taoeffect approved these changes Nov 16, 2023

View reviewed changes

corrideat added 3 commits November 16, 2023 20:10

Add destructor to leave chatrooms

8ef6917

Leave chatrooms on removeOurselves

905bbbc

Remove JOINING_CHATROOM

7b2a3a2

corrideat marked this pull request as ready for review November 17, 2023 17:16

taoeffect merged commit baacd80 into e2e-protocol-ricardo Nov 17, 2023
2 checks passed

taoeffect deleted the e2e-protocol-ricardo-20231025-login-sync branch November 17, 2023 17:16

		}
		return Promise.resolve(sbp(`${manifestHash}/${action}/sideEffect`, mutation))

E2e protocol ricardo 20231025 login sync #1780

E2e protocol ricardo 20231025 login sync #1780

Conversation

corrideat commented Oct 27, 2023

Silver-IT commented Oct 30, 2023

Silver-IT commented Oct 30, 2023

Silver-IT commented Oct 30, 2023

Silver-IT commented Oct 30, 2023

Silver-IT commented Oct 30, 2023

corrideat commented Nov 3, 2023

corrideat commented Nov 4, 2023

Silver-IT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silver-IT commented Nov 7, 2023 • edited Loading

Silver-IT commented Nov 7, 2023

Silver-IT commented Nov 7, 2023

taoeffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corrideat Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silver-IT commented Nov 9, 2023

Silver-IT commented Nov 10, 2023

Still existing issues in terms of workflow

taoeffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silver-IT commented Nov 7, 2023 •

edited

Loading

corrideat Nov 7, 2023 •

edited

Loading

taoeffect Nov 7, 2023 •

edited

Loading