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

Refactor OCW processing #20

Merged
merged 4 commits into from
Jul 20, 2020
Merged

Refactor OCW processing #20

merged 4 commits into from
Jul 20, 2020

Conversation

stiiifff
Copy link
Owner

No description provided.

@stiiifff stiiifff marked this pull request as ready for review July 17, 2020 11:08
@jimmychu0807 jimmychu0807 mentioned this pull request Jul 19, 2020
@jimmychu0807
Copy link
Collaborator

I think overall this is better than the previous(my) primitive implementation:

  • using offchain storage to store up to which block of events we have processed
  • processing http request events in a batch from previously processed block all the way to the current block
    👍

We can merge this to master first. One thing I want to improve on is about the locking. Recent Substrate has implemented Storage Lock, so we can just use the lock instead of using a storage value to simulate the locking mechanism. Do you want to fix it in this PR altogether, or leave it as a todo for next iteration?

(todo: I know I need to update the recipe to use showcase this storage lock instead of using the storage value to simulate the locking mechanism 😄 )

Copy link
Collaborator

@riusricardo riusricardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and agree that we should change the lock type.
I would merge this, the lock change isn't a blocker.

@stiiifff stiiifff merged commit 5718b20 into master Jul 20, 2020
@stiiifff stiiifff deleted the refactor-ocw branch July 20, 2020 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants