okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
331 stars 44 forks source link

DRY: 'chelonia/kv/set' should always use 'chelonia/queueInvocation' #2289

Closed taoeffect closed 1 month ago

taoeffect commented 3 months ago

Problem

The frontend code for calling 'chelonia/queueInvocation' is littered with calls to 'chelonia/queueInvocation', e.g.:

    return sbp('chelonia/queueInvocation', identityContractID, () => {
      return sbp('chelonia/kv/set', identityContractID, KV_KEYS.UNREAD_MESSAGES, data, {
        encryptionKeyId: sbp('chelonia/contract/currentKeyIdByName', identityContractID, 'cek'),
        signingKeyId: sbp('chelonia/contract/currentKeyIdByName', identityContractID, 'csk'),
        onconflict
      })
    })
    return sbp('chelonia/queueInvocation', identityContractID, () => {
      return sbp('chelonia/kv/set', identityContractID, KV_KEYS.PREFERENCES, data, {
        encryptionKeyId: sbp('chelonia/contract/currentKeyIdByName', identityContractID, 'cek'),
        signingKeyId: sbp('chelonia/contract/currentKeyIdByName', identityContractID, 'csk'),
        onconflict
      })
    })

Etc.

This:

EDIT: similar changes may need the done for /get, please see this message on Slack about an error.

Solution

Go through the entire codebase and remove all wrapped 'chelonia/kv/set' calls. Instead, have 'chelonia/kv/set' itself call 'chelonia/queueInvocation'.

taoeffect commented 3 months ago

Edited issue to add:

EDIT: similar changes may need the done for /get, please see this message on Slack about an error.

corrideat commented 2 months ago

I see your point, but I'm not sure about the solution. For reference, see PR #2304 where, while queueInvocation is still used, it's used slightly different. The reason why this is like this is for flexibility, as you may do things that also require waiting on the event queue when doing these updates.

An example of this is in this very issue: 'similar changes may need the done for /get'. Since generally onconflict will call get, that'd be an example of something that might deadlock.

I think a better solution is to leave the API as is, and instead write a wrapper for this common use case. That would address the DRY concern, keep things flexible and at the same time allow for more complex use cases.

taoeffect commented 2 months ago

I think a better solution is to leave the API as is, and instead write a wrapper for this common use case. That would address the DRY concern, keep things flexible and at the same time allow for more complex use cases.

OK, sounds reasonable 👍