Groundwork to move Chelonia into a service worker #1981

corrideat · 2024-05-01T18:12:03Z

No description provided.

cypress · 2024-05-03T17:33:17Z

Passing run #2213 ↗︎

0	111	8	0	0

Details:

Merge `c1dab49` into `7368f3f`...
Project: group-income	Commit: `c130841338 ℹ️`
Status: Passed	Duration: 10:04 💡
Started: May 3, 2024 5:23 PM	Ended: May 3, 2024 5:33 PM

Review all test suite changes for PR #1981 ↗︎

socket-security · 2024-05-20T02:44:12Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@sbp/sbp@2.4.0	None	`0`	62.1 kB	taoeffect

🚮 Removed packages: npm/@sbp/sbp@2.2.0

View full report↗︎

taoeffect · 2024-05-20T15:35:01Z

shared/serdes/index.js

+  Object.defineProperty(obj, raw, { value: true })
+  return obj


Could you:

Add a comment explaining why Object.defineProperty is used instead of obj[raw] = true?

I think this can be shortened to a 1-liner if you put the return in front of the Object on line 20

this can be shortened to a 1-liner if

I don't think that contributes to readability, because it's not self-evident that Object.defineProperty(a, b) returns a.

taoeffect · 2024-05-20T15:43:59Z

shared/serdes/index.js

+const deserializerTable = Object.create(null)
+
+// The `deserializer` function reconstructs data on the receiving side
+export const deserializer = (data: any) => {


What is passed in? I see JSON.stringify is called on data. Normally a deserializer takes in a serialized value, which in the case of JSON is a string. However, the fact that JSON.stringify is being used here indicates that this is not a JSON string. If so, then it sounds like data is the result of calling JSON.parse elsewhere on the serialized JSON string that was passed over the message port..? Or am I mistaken? Anyway, this all should be clarified and explaining in a comment.

You're maybe getting confused because of the calls to JSON.stringify and JSON.parse. Those are incidental and an implementation detail. Both serialize and deserialize use JSON.parse(JSON.stringify(data, a), b). The 'magic' is in what a and b do, and JSON happens to be used because it traverses objects and is relatively fast.

taoeffect · 2024-05-20T15:52:14Z

shared/serdes/index.js

+    if (value && value[raw]) return value
+    if (value === undefined) return rawResult(['_', '_'])
+    if (!value) return value
+    if (Array.isArray(value) && value[0] === '_') return rawResult(['_', '_', ...value])


Can you add a comment explaining what this is checking for?

taoeffect · 2024-05-24T17:00:50Z

frontend/controller/actions/group.js

-        // joinChatRoom sideEffect will trigger a call to 'gi.actions/chatroom/join', we want
-        // to wait for that action to be received and processed, and then switch the UI to the
-        // new chatroom. We do this here instead of in the sideEffect for chatroom/join to
-        // avoid causing the UI to change in other open tabs/windows, as per bug:
-        // https://github.com/okTurtles/group-income/issues/1960
-        onprocessed: (msg) => {
-          const fnEventHandled = (cID, message) => {
-            if (cID === chatRoomID) {
-              if (sbp('state/vuex/getters').isJoinedChatRoom(chatRoomID)) {
-                sbp('state/vuex/commit', 'setCurrentChatRoomId', { chatRoomID, groupID: msg.contractID() })
-                sbp('okTurtles.events/off', EVENT_HANDLED, fnEventHandled)
-              }
-            }
-          }
-          sbp('okTurtles.events/on', EVENT_HANDLED, fnEventHandled)
-        },


Why was this removed?

Because that deals with the Vuex state and doesn't belong into actions.

taoeffect · 2024-05-24T17:05:10Z

frontend/controller/app/group.js

+  'gi.app/group/addAndJoinChatRoom': async function (params: GIActionParams) {
+    const chatRoomID = await sbp('gi.actions/group/addAndJoinChatRoom', params)
+    await sbp('chelonia/contract/wait', chatRoomID)
+    // joinChatRoom sideEffect will trigger a call to 'gi.actions/chatroom/join', we want
+    // to wait for that action to be received and processed, and then switch the UI to the
+    // new chatroom. We do this here instead of in the sideEffect for chatroom/join to
+    // avoid causing the UI to change in other open tabs/windows, as per bug:
+    // https://github.com/okTurtles/group-income/issues/1960
+    if (sbp('state/vuex/getters').isJoinedChatRoom(chatRoomID)) {
+      sbp('state/vuex/commit', 'setCurrentChatRoomId', { chatRoomID, groupID: params.contractID })
+    }
+
+    return chatRoomID
+  }


Why was this separated out like this?

I see no explanation for the separation of actions and app folders.

Could you maybe create a README.md inside of controller/ that explains this?

I'm not sure if this is an actual question or just a request to create a README.md explaining it, but the app name was your idea. The reason for the separation is that app runs on the... app (as in, in the browser window context) and actions run in the service worker context.

just a request to create a README.md explaining it

👍

taoeffect · 2024-05-24T17:06:14Z

frontend/controller/app/identity.js

+      //            loading the website instead of stalling out.
+      /* try {
+        if (!state) {
+          // Make sure we don't unsubscribe from our own identity contract
+          // Note that this should be done _after_ calling
+          // `chelonia/storeSecretKeys`: If the following line results in
+          // syncing the identity contract and fetching events, the secret keys
+          // for processing them will not be available otherwise.
+          await sbp('chelonia/contract/retain', identityContractID)
+        } else {
+          // If there is a state, we've already retained the identity contract
+          // but might need to fetch the latest events
+          await sbp('chelonia/contract/sync', identityContractID, { force: true })
+        }
+      } catch (err) {
+        sbp('okTurtles.events/emit', LOGIN_ERROR, { username, identityContractID, error: err })
+        const errMessage = err?.message || String(err)
+        console.error('Error during login contract sync', errMessage)
+
+        const promptOptions = {
+          heading: L('Login error'),
+          question: L('Do you want to log out? Error details: {err}.', { err: err.message }),
+          primaryButton: L('No'),
+          secondaryButton: L('Yes')
+        }
+
+        const result = await sbp('gi.ui/prompt', promptOptions)
+        if (!result) {
+          return sbp('gi.app/identity/logout')
+        } else {
+          throw err
+        }
+      }
+      */


This was important and commonly used code that comes into play frequently during development (anytime the server is restarted).

Why is it commented out and how will this be handled instead?

EDIT: oh I see, is the code below supposed to replace it?

taoeffect · 2024-05-24T17:10:37Z

frontend/controller/serviceworkers/sw-primary.js

+  /* hooks: {
+    handleEventError: (e: Error, message: GIMessage) => {
+      if (e.name === 'ChelErrorUnrecoverable') {
+        sbp('gi.ui/seriousErrorBanner', e)
+      }
+      if (sbp('okTurtles.data/get', 'sideEffectError') !== message.hash()) {
+        // Avoid duplicate notifications for the same message.
+        errorNotification('handleEvent', e, message)
+      }
+    },
+    processError: (e: Error, message: GIMessage, msgMeta: { signingKeyId: string, signingContractID: string, innerSigningKeyId: string, innerSigningContractID: string }) => {
+      if (e.name === 'GIErrorIgnoreAndBan') {
+        sbp('okTurtles.eventQueue/queueEvent', message.contractID(), [
+          'gi.actions/group/autobanUser', message, e, msgMeta
+        ])
+      }
+      // For now, we ignore all missing keys errors
+      if (e.name === 'ChelErrorDecryptionKeyNotFound') {
+        return
+      }
+      errorNotification('process', e, message)
+    },
+    sideEffectError: (e: Error, message: GIMessage) => {
+      sbp('gi.ui/seriousErrorBanner', e)
+      sbp('okTurtles.data/set', 'sideEffectError', message.hash())
+      errorNotification('sideEffect', e, message)
+    }
+  } */


How are these going to be handled?

This is still in progress and I don't have an answer at this time. The problematic part here is the notifications. Having contracts run in a SW will require re-thinking how notifications work.

taoeffect · 2024-05-24T17:16:56Z

frontend/model/database.js

  'gi.db/settings/save': function (user: string, value: any): Promise<*> {
-    return appSettings.setItem(user, value)
+    return appSettings.setItem('u' + user, value)


What's the reason for this u prefix? (Can you add a comment?)

It's to disambiguate the type of data that's being stored. I can add a comment.

taoeffect · 2024-05-24T17:22:06Z

frontend/model/state.js

+  groupProfile (state, getters) {
+    return member => {
+      const profiles = getters.currentGroupState.profiles
+      return profiles && profiles[member] && {
+        ...profiles[member],
+        get lastLoggedIn () {
+          return getters.currentGroupLastLoggedIn[member] || this.joinedDate
+        }
+      }
+    }
+  },
+  groupProfiles (state, getters) {
+    const profiles = {}
+    for (const member in (getters.currentGroupState.profiles || {})) {
+      const profile = getters.groupProfile(member)
+      if (profile.status === PROFILE_STATUS.ACTIVE) {
+        profiles[member] = profile
+      }
+    }
+    return profiles
+  },
+  groupCreatedDate (state, getters) {
+    return getters.groupProfile(getters.currentGroupOwnerID).joinedDate
+  },
+  groupMincomeAmount (state, getters) {
+    return getters.groupSettings.mincomeAmount
+  },
+  groupMincomeCurrency (state, getters) {
+    return getters.groupSettings.mincomeCurrency
+  },


As far as I can tell, these and related getters were copied here from the group contract.

Why?

Why were they not deleted from group.js? (DRY)

Getters are useful in contracts. If you are moving so many of them out of contracts then we need to come up with a total replacement for them and rewrite a lot of the code in the group.js contract.

Or we need to come up with a way to make them work the way they did before (or in similar fashion) without violating DRY.

As far as I can tell, these and related getters were copied here from the group contract.

Correct.

Because that seemed like the easiest thing to do with the fewest number of changes required (an alternative would have been to move them to a different file which is then imported from both places).

Because they're needed there. In particular, after this change, contract getters and root getters are no longer the same and cannot call each other (at least, not synchronously).

Getters are useful in contracts. If you are moving so many of them out of contracts then we need to come up with a total replacement for them and rewrite a lot of the code in the group.js contract.

That's correct. I think we should probably get rid of the concept of getters in contracts altogether, as that's a Vue.js concept anyhow. However, a shorter term alternative is to import them from a different shared file, to avoid source duplication (there'll still be duplication in the build artifacts).

I definitely agree we would need to at the very least fix the DRY issue in the source files.

I think we should probably get rid of the concept of getters in contracts altogether, as that's a Vue.js concept anyhow.

It's a very useful concept for building Vue.js GUIs. We would need to keep it just for that, as otherwise it would require rewriting a bunch of the frontend code as well, and we don't want to do that not just because we don't have time for it, but because we want to support Vue.js and similar libraries that use the concept of getters.

So what is your proposal...?

It's a very useful concept for building Vue.js GUIs

Precisely my point. It's useful for Vue.js. It's not generally useful outside of Vue.js ('it' refers to the 'getters' API, not to the idea of a function that returns a value), because it's not a universal concept (e.g., Redux doesn't have them) and even if it were a common enough concept, it's currently only useful (in the sense of having 'getters' be part of a contract definition) because the API is made to be exactly what Vue.js uses, which may be different for other frameworks.

I'm not saying to get rid of them on Vue.js, Vue.js can still use them. What I'm saying is that Vue.js doesn't have access to them in contracts and so the whole idea that getters can be shared is just not feasible. So, for contracts, there'd be nothing to replace them. However, since contracts can run arbitrary code, they can define their own getters and for applications that use Vue.js, they can use the same getter signature that's used in Vue.js.

To expand on the not feasible part:

I mean that it cannot be literally the same function reference, as it's done now on master, where sbp('state/vuex/getters').groupCreatedDate will use some code that's defined in the group contract. This is because the UI is running on the main thread and the group contract is running in a SW (potentially in yet another context after sandboxing). What can be done, as I suggested, is moving the group getters definitions to a different file, and importing that from the app and the contract.

corrideat requested a review from taoeffect May 1, 2024 18:12

WIP 1

def677e

corrideat force-pushed the feat/isolate-contract-state branch 3 times, most recently from d1d6044 to 9639c3b Compare May 3, 2024 15:09

WIP

c1dab49

corrideat force-pushed the feat/isolate-contract-state branch from 9639c3b to c1dab49 Compare May 3, 2024 17:21

corrideat added 2 commits May 10, 2024 18:43

WIP; not working

de516d1

Fix routing issues

a416e8f

corrideat mentioned this pull request May 13, 2024

Assume all calls to Chelonia are async #1994

Open

corrideat added 2 commits May 15, 2024 21:50

Serdes function

1249fe3

Merge branch 'master' into feat/isolate-contract-state

05c872c

taoeffect reviewed May 20, 2024

View reviewed changes

WIP

b34a226

taoeffect reviewed May 20, 2024

View reviewed changes

corrideat added 5 commits May 20, 2024 18:52

WIP

b2a615c

Fixes to cross-boundary fns

92d54fe

Merge branch 'master' into feat/isolate-contract-state

b51a79d

SW Lifecycle

7c7c751

WIP

74cab63

taoeffect reviewed May 24, 2024

View reviewed changes

Service worker sync

0a4136b

corrideat added 6 commits May 30, 2024 15:21

WIP

35c74f4

Various improvements to SW

0f04f99

Fixes related to group-chat.spec.js

9a9e05a

Fix: tests hang on start

2e9758a

Merge branch 'master' into feat/isolate-contract-state

378e08a

Fixes after merge

ce187c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Groundwork to move Chelonia into a service worker #1981

Groundwork to move Chelonia into a service worker #1981

corrideat commented May 1, 2024

cypress bot commented May 3, 2024

socket-security bot commented May 20, 2024 •

edited

taoeffect May 20, 2024

corrideat May 24, 2024

taoeffect May 20, 2024

corrideat May 24, 2024

taoeffect May 20, 2024

taoeffect May 24, 2024

corrideat May 24, 2024

taoeffect May 24, 2024

corrideat May 24, 2024 •

edited

taoeffect May 24, 2024

taoeffect May 24, 2024 •

edited

taoeffect May 24, 2024

corrideat May 24, 2024

taoeffect May 24, 2024

corrideat May 24, 2024

taoeffect May 24, 2024

corrideat May 24, 2024

taoeffect May 24, 2024

corrideat May 24, 2024 •

edited

corrideat May 24, 2024

Groundwork to move Chelonia into a service worker #1981

Are you sure you want to change the base?

Groundwork to move Chelonia into a service worker #1981

Conversation

corrideat commented May 1, 2024

cypress bot commented May 3, 2024

Passing run #2213 ↗︎

Review all test suite changes for PR #1981 ↗︎

socket-security bot commented May 20, 2024 • edited

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

corrideat May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect May 24, 2024 • edited

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

corrideat May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socket-security bot commented May 20, 2024 •

edited

corrideat May 24, 2024 •

edited

taoeffect May 24, 2024 •

edited

corrideat May 24, 2024 •

edited