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

ACL caching #233

Merged
merged 32 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@2color
Copy link
Contributor

commented Jan 9, 2019

What

  • Simplify initAcl
  • Cache the permissions object (reduced ACL events)

Why

  • This builds on #232
  • A separate PR is for experimenting with caching while leaving the discussion about the initial refactor in the other PR
  • Still contains a lot of logging for debugging

How

  • Add the ability to fetch events from a specific block number
  • Cache blocks that we have finished processing

Open Questions

  • Is there a way to know that I've finished processing all the events of a block?
    • Yes with getPastLogs because the call resolves with a single value

Currently I wait until I get the next block and only then do I assume that I've gotten all the events for the previous.
This means that the last block's events are not cached and have to be fetched again.

Potential improvements

  • Create unit tests (hopefully with as little test doubles as possible) for the caching functionality. This should help in turn with the next steps of abstracting cleaning and also provide another form of documentation for the logic.
  • Update the blockNumber in cache to a later point if we're certain no relevant events came in later blocks - reduce load on node filtering from an earlier point every time.

2color added some commits Jan 8, 2019

Simplify the initAcl logic
Retrieve all events and there by drastically simplify the method.
Because the Acl contract only emits the two events currently,
this doesn't introduce any network overhead for extra data.

Because the events of the two types we're interested in are now
ordered in the stream, we no longer need the added skipWhile
operator and the eventSet passed along

@izqui izqui requested review from izqui and sohkai Jan 10, 2019

@izqui izqui added the Aragon One label Jan 10, 2019

@izqui

izqui approved these changes Jan 10, 2019

Copy link
Member

left a comment

Just some minor comments. I understand some of the logging will be removed before merging.

Show resolved Hide resolved packages/aragon-wrapper/src/core/proxy/index.js Outdated
Show resolved Hide resolved packages/aragon-wrapper/src/index.js Outdated
Show resolved Hide resolved packages/aragon-wrapper/src/index.js Outdated
@2color

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Run fetch -> first event message processing first message -> permissions ready
1 with cache 3175.10205078125ms 35.1689453125ms
2 with cache 3690.963134765625ms 34.9951171875ms
3 with cache 2444.296142578125ms 35.690185546875ms
4 with cache 5280.4609375ms 36.490234375ms
5 no cache 3935.661865234375ms 150.660888671875ms
6 no cache 3319.256591796875ms 131.905029296875ms

I did a few tests to see where the lag originates.

While this PR reduces the processing time by about 100ms (This is of course a correlate of the number of events which it bound to grow over time for active DAOs).

My suggestion moving forward would be to investigate whether it makes sense to let the cached permissions flow out of the function before waiting for the first events to arrive.

@sohkai sohkai added this to the A1 Sprint: 4.2 milestone Feb 18, 2019

@sohkai sohkai removed the Aragon One label Feb 18, 2019

2color added some commits Mar 12, 2019

Merge remote-tracking branch 'origin/master' into simplify-get-acl-wi…
…th-cache

* origin/master: (28 commits)
  chore: Rename packages (#255)
  wrapper: v4.0.0-beta.1
  Migrate to rxjs 6 (#123)
  wrapper: v3.0.0
  wrapper: remove unnecessary wait in cache test (#250)
  chore: upgrade lerna (#239)
  docs: Remove the documentation package and inline code documentation (#247)
  CI: call npm run build in build step
  CI: run size after build
  CI: add all actions to workflow
  CI: trigger bootstrap step after install
  CI workflow fixes
  Correct workflow
  Add bootstrap stage
  Update workflow to resolve actions
  Add test workflow
  docs: update APP.md (#246)
  wrapper: v3.0.0-beta.5
  client: v1.1.1
  chore: upgrade radspec (#244)
  ...

@2color 2color referenced this pull request Mar 22, 2019

Closed

Simplify the initAcl logic #232

2color added some commits Apr 26, 2019

Merge remote-tracking branch 'origin/master' into simplify-get-acl-wi…
…th-cache

* origin/master: (29 commits)
  Update README.md (#291)
  Wrapper: clarify comments about forwarding path finding strategy (#289)
  Split quick start doc from intro (#287)
  @aragon/wrapper 5.0.0-rc.2
  wrapper: prettify setApp() descriptions (#284)
  wrapper: fix callsscripts decoding (#283)
  Wrapper: add installedRepos observable (#268)
  fix: avoid infinitely looping through forwarders when looking for a transaction path (#285)
  wrapper: enforce message to sign is string (#282)
  api: v1.1.0
  rpc-messenger: v1.1.0
  feat: @aragon/wrapper api cleanup (#279)
  Wrapper: handle SetApp for updated apps (#267)
  feat: Add message signing (#276)
  Wrapper: don't assign initializationBlock on non-kernel proxies when unneeded (#266)
  chore: ignore package-lock.jsons (#280)
  API: propagate errors to single-response APIs (#277)
  Rpc Messenger: dedupe message bus across requests (#278)
  Fix: changes after review (#274)
  Docs update: include react api, aragon app architecture & fixes (#271)
  ...

@2color 2color changed the title [wip] Simplify get acl and add caching ACL caching Apr 30, 2019

@sohkai
Copy link
Member

left a comment

Looking good, just noticed a simplification that allows us to "flatten" the pipeline so it's a bit more clear :).

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

2color added some commits May 8, 2019

Separate caching logic from acl reduction
- Simplify the process of checking for completed blocks with pairwise
- Eliminate the need for a set of processed blocks
- Set the cacheBlockHeight one upon initialisation

@sohkai sohkai referenced this pull request May 10, 2019

Open

aragonAPI ACL caching (app list + permissions) #97

0 of 1 task complete
@sohkai

sohkai approved these changes May 13, 2019

Copy link
Member

left a comment

Awesome job @2color, looking forward to this getting in 😍. Left a few last short suggestions.

Show resolved Hide resolved packages/aragon-wrapper/src/cache/index.js Outdated
Show resolved Hide resolved packages/aragon-wrapper/src/index.js Outdated
Show resolved Hide resolved packages/aragon-wrapper/src/utils/index.js
Show resolved Hide resolved packages/aragon-wrapper/src/utils/index.test.js Outdated

@sohkai sohkai referenced this pull request May 13, 2019

Closed

0.7.2 #102

17 of 20 tasks complete

2color added some commits May 14, 2019

Merge remote-tracking branch 'origin/master' into simplify-get-acl-wi…
…th-cache

* origin/master:
  @aragon/wrapper 5.0.0-rc.5
  fix: add back name and identifer to describe script (#299)
  wrapper: return original step if radspec couldn't be evaluated (#298)
  @aragon/wrapper 5.0.0-rc.3
  wrapper: handle intent baskets (#286)
  @aragon/wrapper 5.0.0-rc.3
  Wrapper: add artifacts for aragonPM apps (#273)
  Wrapper: override fetched app info with on-chain values (#288)

@2color 2color merged commit 4a44c8c into aragon:master May 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
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.