search

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

Explain more clearly why the 'direction' exists for GIMessage #2312

Closed taoeffect closed 2 months ago

taoeffect commented 2 months ago

Problem

This comment is insufficient:

          // Process message to ensure that it is valid. Should this thow,
          // we propagate the error.
          // Because of this, 'chelonia/private/in/processMessage' SHOULD NOT
          // change the global Chelonia state and it MUST NOT call any
          // side-effects or change the global state in a way that affects
          // the meaning of any future messages or successive invocations.
          await sbp('chelonia/private/in/processMessage', entry, cloneDeep(state || {}))

It doesn't explain why we are first processing outgoing messages. It just says, "to ensure that it is valid." That is not a sufficient explanation.

Solution

Expand this comment to give a specific example of why this must be done.

The reason why this is a big deal is because I keep forgetting whether or not this is really necessary. In an ideal world, this wouldn't be necessary. It would be fine for two clients to send the same message twice (and have the second rejected upon receipt). It would greatly simplify our implementation if we could get rid of this. However, if this is actually needed to prevent real bugs, then that must be explained in detail — the specific situation — where and why this is needed.

corrideat commented 2 months ago

Sure, the explanation can be expanded, but I disagree with the premise of it not being necessary.

In an ideal world, this wouldn't be necessary. It would be fine for two clients to send the same message twice (and have the second rejected upon receipt).

This is how things work now too. However, that doesn't mean that you should send invalid messages, because it's wasteful in terms of storage, processing, etc. This is the same reason why IIRC, previously (as in, before I was involved with the project [1]) the validate function was called.

It would greatly simplify our implementation if we could get rid of this.

I don't think it would simplify anything. Validating messages before sending, as mentioned, was pre-existing logic. This only expanded on that functionality as it turns out that the proper way to do validation is running the entire contract code (as the future is impossible to predict).

If you're referring to this:

// Because of this, 'chelonia/private/in/processMessage' SHOULD NOT // change the global Chelonia state and it MUST NOT call any

Then that's also false (although I see the point that the comment could be updated). Process is called, for example, from ChatMain.vue. If processing a message changed the global state, the app would behave differently based on what ChatMain.vue does or does not do, which is not correct. In other words, even if validation for outgoing messages was removed (which it shouldn't be), it's still safer and correct not to assume a single global state that contracts can change.

[1] See https://github.com/okTurtles/group-income/blob/5d3e6b380f98990ab359c1fa8104f8bad90b4e93/shared/domains/chelonia/chelonia.js#L231

taoeffect commented 2 months ago

I don't think it would simplify anything.

It would though - all of the code related to direction could then be removed. It would be one less thing to think about.