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

App state caching #297

Open
wants to merge 36 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@2color
Copy link
Contributor

commented Apr 30, 2019

What

  • Split app state event streams into two separate streams for past and future events
  • Add an init option for initialising the state (to replace the INITIALIZATION_TRIGGER pattern)
  • Deprecate the events option in favour of an externals option for external contracts

TODO

  • Accounts trigger built into the store

  • Provide a mechanism for caching external events. For this, app scripts should be able to pass in an external contract (as here) and pass a fromBlock used for fetching events the events.

  • Differentiate between actual cache and state passing to application (which uses the state cache key)

  • Pass cachedState into init function - #297 (comment)

  • emit a custom event for the reducer to know the sync status (the endWith operator might be useful)

  • Replace the external pastEvents handler's use of the Proxy class with web3.eth.Contract

  • Add web3.js options support to external's and pastEvents API

  • Add syncStatus()) for apps to know the sync status -#297 (comment)

  • Add web3.js options support to external's events API

Corresponding PRs in apps to use updated API

Voting - aragon/aragon-apps#836
Token Manager - aragon/aragon-apps#845
Finance - aragon/aragon-apps#854

Related issue

Fixes #294, fixes #6, fixes #63, supersedes #85.

Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated

@2color 2color force-pushed the app-state-caching branch from 56dd960 to 1d6b680 May 15, 2019

@2color 2color force-pushed the app-state-caching branch from 1d6b680 to fa51538 May 15, 2019

2color added some commits May 15, 2019

Consistent spelling
As much as it's counter intuitive for me to use z consistency is what
matterz

@2color 2color force-pushed the app-state-caching branch from 89b987d to af469fe May 16, 2019

@2color 2color marked this pull request as ready for review May 16, 2019

sohkai added some commits May 18, 2019

@sohkai
Copy link
Member

left a comment

Really nice progress @2color!

I've left a couple comments, the most important one is how we use the init() function. Rather than this being "pure" (from past state, at least), I'd rather initialize it with the cache value so apps can refresh any stale data at this initialization point (amongst other initialization activities).

Secondly, a bit of an API nitpick (and I was originally the one who took the shortcut with external.events() 🙈), in cases where we use getPastEvents() and events(), we should allow the user to specify the full range of options that web3.js supports.

Finally, I have cosmetic refactor in mind to merge the past and current event streams (allowing us to deduplicate the logic for the mergeScan) and avoid the final merge(). Let's work on it after these first comments are addressed :).

Show resolved Hide resolved packages/aragon-wrapper/src/rpc/handlers/external.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
Show resolved Hide resolved packages/aragon-api/src/index.js Outdated
// Use the last past state as the initial state for reducing current/future states
last(),
switchMap(pastState => {
return merge(currentEvents$, accounts$).pipe(

This comment has been minimized.

Copy link
@sohkai

sohkai May 18, 2019

Member

Can we define currentEvents in this block?

⚠️ Also, I think we may have a problem with the accounts$ observable being hot; ideally we would only kick it off here (the old semi-hot behaviour will not be a problem anymore after #305).

This comment has been minimized.

Copy link
@2color

2color May 21, 2019

Author Contributor

Can we define currentEvents in this block?

Good idea.

⚠️ Also, I think we may have a problem with the accounts$ observable being hot; ideally we would only kick it off here (the old semi-hot behaviour will not be a problem anymore after #305).

We only really care about the last value which publishReplay should ensure is available. Do you think it fits better here even if we don't have a hot observable problem?
I put it in the top block to avoid bloating this part with all the object creation part.

This comment has been minimized.

Copy link
@sohkai

sohkai May 21, 2019

Member

Ah yes, that's true! I think with #305 we will be able to just use app.accounts() to simplify this whole bit.

Show resolved Hide resolved packages/aragon-api/src/index.js
Show resolved Hide resolved packages/aragon-api/src/index.js
Show resolved Hide resolved packages/aragon-api/src/index.js

2color and others added some commits May 21, 2019

Update packages/aragon-api/src/index.js
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
Clamp block number for local chains
In local chains this can yield an invalid negative value
@2color

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

I just discovered a major bug in the current implementation. Because we rely on the cache key state to pass state to the application, we need to introduce a different cache key for the cached state. Otherwise, the block safety margin is not taken into consideration and every reduced state will be cached.

Right now we conflate the two actions:

Update:
I've addressed this problem in 819aeb9.

This might reduce the number of re-renders of the app because if nothing has changed since the cached block the app will just have the last state it had before closing.

@sohkai

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I just discovered a major bug in the current implementation. Because we rely on the cache key state to pass state to the application, we need to introduce a different cache key for the cached state.

I was thinking about this earlier, and ended up writing #294 (comment) in reasoning about this.

I'm curious if you might have noticed something that necessitates keeping the current state and cached state separate? I ultimately concluded that it's not a big deal to keep using the current state cache if developers know that some already-cached events may be repeated (even after these changes, it could still happen if a block reorg occurred when the app is running).

The nice benefit of keeping the current state cache as the "safe cached state", is that state won't appear to be briefly "rolled back" if a user closes the app and re-opens it.

@2color

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I'm curious if you might have noticed something that necessitates keeping the current state and cached state separate?

If the state is based on an event that was not included after a re-org, refetching the events from the cached block number doesn't guarantee that the state will not be invalid.

The nice benefit of keeping the current state cache as the "safe cached state", is that state won't appear to be briefly "rolled back" if a user closes the app and re-opens it.

With 819aeb9 the app will still render the latest state. It will just use the cached (slightly older) for refetching and reducing the delta from the last safety margin to the current head. So we still get most of the benefit without the supposed risk –as I described above–.

To summarise, I'm working under the assumption that anything 100 blocks old could get dropped and not be included in a reorg. If we can safely assume that reducing and caching an event that will get dropped is safe, we don't need 2 cache keys for cached state and current state.

2color added a commit to aragon/aragon-apps that referenced this pull request May 22, 2019

@2color

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Secondly, a bit of an API nitpick (and I was originally the one who took the shortcut with external.events() 🙈), in cases where we use getPastEvents() and events(), we should allow the user to specify the full range of options that web3.js supports.

I'll change it to an options object so that it can support topics and filter too, as per web3.js:

pastEvents: (options = {})
events: (options = {})

That means that calls will look like: pastEvents({ fromBlock: 10, toBlock: 20 }).

That begs the question of what do to do about the API for the app's contract?
With the introduced change it will feel inconsistent:

events (fromBlock)
pastEvents (fromBlock, toBlock)

Any thoughts?

@2color

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Regarding the syncStatus method. Right now the aragonAPI is used by apps and app scripts to communicate with the native context.
Introducing something like syncStatus would be a deviation from that, insofar as it's used to communicate state between the app script and the app.
Did you have any concrete thoughts on how that would look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.