Store notifications in server side KV store

[Silver-IT]

Summary of Changes

• Closes Store notifications in server-side KV store #1936

[cypress]

Passing run #2520 ↗︎

0

114

8

0

0

Details:

Merge ef972f4 into cee60cf...
Project: group-income	Commit: 466d00d071 ℹ️
Status: Passed	Duration: 10:37 💡
Started: Jul 2, 2024 10:56 PM	Ended: Jul 2, 2024 11:07 PM

Review all test suite changes for PR #2129 ↗︎

[Silver-IT]

Ready for the review!

[taoeffect]

Great work @Silver-IT!! Review ready!

[taoeffect]

Problem 1 - same hash for different notifications

Hmm, by excluding the body key, is it possible that two different notifications might get hashed the same way? Looking at the templates.js, it seems that it is possible.

But, I agree that the body should be excluded because it could change (e.g. when someone changes their display name, and then logs back in on a new device).

So what's needed is an identifier that is both unique to the notification, and also consistent across devices (meaning it doesn't matter when the notification is sent or on what device).

When calling gi.notifications/emit, we can pass in a variable called uniqueConsistentID.

• For notifications that are generated inside of contract sideEffects, we can use the hash parameter that is passed in to the sideEffect. This guarantees that the same unique consistent identifier is used for these notifications. E.g. do:

  uniqueConsistentID: `MEMBER_ADDED.${hash}`

• For notifications that are generated outside of sideEffects, e.g. the ones created by the periodic notifications system inside of mainNotificationsMixin.js, you will have to construct a unique consistent identifier yourself. One example is given below:

        emit ({ rootState, rootGetters }) {
          const period = rootGetters.groupSettings.distributionDate
          const groupID = /* ... code here to fetch corresponding groupID ... */
          sbp('gi.notifications/emit', 'NEAR_DISTRIBUTION_END', {
            // WARNING: see "Problem 2" below for why createdDate can cause problems
            createdDate: new Date().toISOString(),
            groupID: rootState.currentGroupId,
            period,
            uniqueConsistentID: `NEAR_DISTRIBUTION_END.${groupID}.${period}`
          })
        },

Problem 2 - different hash for the same notifications

As shown in the example above for NEAR_DISTRIBUTION_END, some notifications include dynamic data in them that changes depending on when the notification is posted.

This means that if you log in on another device, the same notification will have a different hash. In this case, the offending variable is createdDate.

So, both problems can be solved by ONLY using uniqueConsistentID.

In fact, by requiring all notifications to have a uniqueConsistentID, we can remove the makeNotificationHash function entirely and only rely on the uniqueConsistentID parameter to act as the key.

[Silver-IT]

by excluding the body key, is it possible that two different notifications might get hashed the same way?

Simply, the body is the outcome of translation. 😁 So we should not include it.

For the uniqueConsistentID, it's a good idea. And I don't think we don't need to add type in the uniqueConsistentID because the notification object has its type`.

const notification = {
  ...template,
  hash: '',
  avatarUserID: template.avatarUserID || sbp('state/vuex/getters').ourIdentityContractId,
  // Sets 'groupID' if this notification only pertains to a certain group.
  ...(template.scope === 'group' ? { groupID: data.groupID } : {}),
  // Store integer timestamps rather than ISO strings here to make age comparisons easier.
  timestamp: data.createdDate ? new Date(data.createdDate).getTime() : Date.now(),
  type
}
notification.hash = makeNotificationHash(notification)

And for the notifications created inside the contract sideEffect, I think there are no problems with the current function makeNotificationHash because it uses meta.createdDate as createdDate of notification itself.
But for the other notifications created outside of contract sideEffect, their createdDate could differ from one device to another even though they are same notifications. So, I think we should consider only this one.

[Silver-IT]

However, I totally agree with your idea that we should use uniqueConsistentID.
I thought you wanted to only add that field and still using makeNotificationHash function. But if we add uniqueConsistentID, we don't need makeNotificationHash function anymore. 😁👍

[Silver-IT]

@taoeffect, while I was working on this, I noticed that we don't need uniqueConsistentID and we can still have identifiable data to make hash using makeNotificationHash. The only reason why we need uniqueConsistentID was that we set createdDate inside the notificationData in the place outside of contract sideEffect.

sbp('gi.notifications/emit', 'NEAR_DISTRIBUTION_END', {
  createdDate: new Date().toISOString(),
  groupID: rootState.currentGroupId,
  period: rootGetters.groupSettings.distributionDate
})

But we don't need to set the createdDate here because we have a way to use the default value Date.now().

const notification = {
  ...template,
  hash: makeNotificationHash({ ...data, type }),
  avatarUserID: template.avatarUserID || sbp('state/vuex/getters').ourIdentityContractId,
  // Sets 'groupID' if this notification only pertains to a certain group.
  ...(template.scope === 'group' ? { groupID: data.groupID } : {}),
  // Store integer timestamps rather than ISO strings here to make age comparisons easier.
  timestamp: data.createdDate ? new Date(data.createdDate).getTime() : Date.now(),
  type
}

This means that we can easily using makeNotificationHash by removing createdDate in the place where we pass notificationData.

And to be honest, I really prefer to use makeNotificationHash because to make ID (uniqueConsistentID) manually is something like tricky temporary solution when we don't have the better solution. We also create hash for GIMessages using the identifiable value.

hash = createCID(value)

[taoeffect]

Removing createdDate from the hashed data only addresses Problem 2 in the comment I wrote above.

Problem 1 is still an issue. So I think we need to introduce uniqueConsistentID, unless you have some other idea.

[taoeffect]

I don't think there is a problem (Problem 1), because notificationData contains createdDate, which is meta.createdDate, when the we create notifications in the contract sideEffect.

There is a problem. It is possible for notifications to contain the same data but be different notifications.

For example, PAYMENT_THANKYOU_SENT has this:

  PAYMENT_THANKYOU_SENT (data: { creatorID: string, fromMemberID: string, toMemberID: string }) {
    return {
      avatarUserID: data.fromMemberID,
      body: L('{strong_}{name}{_strong} sent you a {strong_}thank you note{_strong} for your contribution.', {
        name: `${CHATROOM_MEMBER_MENTION_SPECIAL_CHAR}${data.fromMemberID}`,
        ...LTags('strong')
      }),
      creatorID: data.fromMemberID,
      icon: '',
      level: 'info',
      linkTo: `/payments?modal=ThankYouNoteModal&from=${data.fromMemberID}&to=${data.toMemberID}`,
      scope: 'group'
    }
  },

This notification will be sent multiple times, and it will have the same exact hash each time it is sent between these two members.

EDIT: There may be many other such notifications as there is nothing preventing the developer from creating such notifications. OTOH, with uniqueConsistentID the problem is explicitly considered and acknowledged.

[Silver-IT]

We need to focus on the data here, where we call the gi.notifications/emit function.

sbp('gi.contracts/group/emitNotificationsAfterSyncing', [contractID, innerSigningContractID], [{
  notificationName: 'PAYMENT_THANKYOU_SENT',
  notificationData: {
    createdDate: meta.createdDate,
    groupID: contractID,
    fromMemberID: innerSigningContractID,
    toMemberID: data.toMemberID
  }
}])

[taoeffect]

This is going to generate the same hash every time:

        sbp('gi.notifications/emit', 'NEW_DISTRIBUTION_PERIOD', {
          groupID: rootState.currentGroupId,
          creatorID: rootGetters.ourIdentityContractId,
          memberType: rootGetters.ourGroupProfile.incomeDetailsType === 'pledgeAmount' ? 'pledger' : 'receiver'
        })

This is going to continue to be a problem for the reasons I mentioned in the original post above.

Anyone creating these notifications is going to eventually accidentally create notifications that result in the same hash (Problem 1).

Instead of ignoring this problem and manually fixing it anyway when it comes up, I think it's much better to explicitly acknowledge the problem and require all notifications to define uniqueConsistentID manually. You need to do it anyway with the data. By defining uniqueConsistentID explicitly we inform all developers about this issue and force them to think about it.

[taoeffect]

If you really don't want to use uniqueConsistentID you will have to manually perform the same checks on the data that's passed in to gi.notifications/emit.

Also, in this case please add a comment above the definition of gi.notifications/emit explaining all of this.

My recommendation remains to prefer uniqueConsistentID over makeNotificationHash, simply because using uniqueConsistentID forces the developer to think about this problem, whereas with makeNotificationHash the developer is allowed to ignore it, but then passes on that burden to the reviewer who has to double-check their work and also not miss any mistakes the developer made in either running afoul of Problem 1 - same hash for different notifications or Problem 2 - different hash for the same notifications.

Either way, please document both problems 1 and 2 in a comment above gi.notifications/emit. 🙏

[Silver-IT]

Yeah, thanks.

[Silver-IT]

Ready for the review!

[taoeffect]

Great work @Silver-IT! Only a few remaining issues. Also the previous issue about the same hash has not been addressed.

[taoeffect]

Review ready!

[taoeffect]

FYI, this notification now runs afoul of Problem 2 - different hash for the same notifications because it uses new Date().toISOString().

Meaning: the same notification on different devices will have different hashes.

[Silver-IT]

This was my intention to make all the errors independent because

• Same error could occur several times
• Even though it seems to be same, the reason could be different from one to another, from device to device

[taoeffect]

Also a request for comment above gi.notifications/emit

[taoeffect]

If you really don't want to use uniqueConsistentID you will have to manually perform the same checks on the data that's passed in to gi.notifications/emit.

Also, in this case please add a comment above the definition of gi.notifications/emit explaining all of this.

My recommendation remains to prefer uniqueConsistentID over makeNotificationHash, simply because using uniqueConsistentID forces the developer to think about this problem, whereas with makeNotificationHash the developer is allowed to ignore it, but then passes on that burden to the reviewer who has to double-check their work and also not miss any mistakes the developer made in either running afoul of Problem 1 - same hash for different notifications or Problem 2 - different hash for the same notifications.

Either way, please document both problems 1 and 2 in a comment above gi.notifications/emit. 🙏

[taoeffect]

Fantastic work @Silver-IT!!! 🎉