Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix confused and outdated 'chelonia/contract/sync' situation #2298

Closed
taoeffect opened this issue Aug 12, 2024 · 0 comments · Fixed by #2303
Closed

Fix confused and outdated 'chelonia/contract/sync' situation #2298

taoeffect opened this issue Aug 12, 2024 · 0 comments · Fixed by #2303
Assignees
Labels
App:Frontend Kind:Bug Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Kind:Documentation Note:Contracts Issues involving modifications to contracts Priority:High

Comments

@taoeffect
Copy link
Member

taoeffect commented Aug 12, 2024

Problem

Our Information-Flow.md documentation is outdated for how to sync a contract.

It currently says that developers should use 'chelonia/contract/sync', but that's wrong, as they're supposed to use 'cheloina/contract/retain' along with 'chelonia/contract/release'.

A separate problem is this code inside of 'chelonia/contract/sync':

      if (!forcedSync && this.subscriptionSet.has(contractID)) {
        const rootState = sbp(this.config.stateSelector)
        if (!rootState[contractID]?._volatile?.dirty) {
          return sbp('chelonia/private/queueEvent', contractID, ['chelonia/private/noop'])
        }
      }

It's unclear to me what this code is doing, and it should be clearly commented.

Solution

  1. Search the Group Income side of the app and replace all calls to 'chelonia/contract/sync' with 'chelonia/contract/retain' (and release).
  2. Then anywhere the concept of resyncing a currently sync'd and retained contract is needed, either call a new selector (e.g. 'chelonia/contract/forceSync'), or change the behavior of 'chelonia/contract/sync' so that it does force syncing. Be careful if you pick the "change behavior" route, since if any old versions of contracts that call that expecting the old behavior to be there, then the behavior shouldn't be changed (since those are deployed for life now). My preference would be to change the behavior for 'chelonia/contract/sync', so that forceSync is true by default, and tell developers to call this on contracts upon logging in (or coming back online). Go through the app and search for places where we need to explicitly set forceSync: false.
  3. Update the Information Flow section with the latest correct guidance (use retain most of the time, and sync upon login).
@taoeffect taoeffect added Kind:Bug App:Frontend Priority:High Kind:Documentation Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Note:Contracts Issues involving modifications to contracts labels Aug 12, 2024
corrideat added a commit that referenced this issue Aug 16, 2024
taoeffect pushed a commit that referenced this issue Sep 16, 2024
* Clarify use of sync. Closes #2298

* Feedback

* Improve comment

* Debug

* Remove debug statements

* Add comment and fix selector name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App:Frontend Kind:Bug Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Kind:Documentation Note:Contracts Issues involving modifications to contracts Priority:High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants