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

Offchain Workers: Example Pallet #4989

Merged
merged 24 commits into from Feb 20, 2020
Merged

Offchain Workers: Example Pallet #4989

merged 24 commits into from Feb 20, 2020

Conversation

@tomusdrw
Copy link
Contributor

tomusdrw commented Feb 19, 2020

This PR introduces an example pallet, which showcases simple usage of offchain workers. I've tried to focus on documenting all code patterns and some possible edge-cases and write some basic tests for the pallet. It obviously does not cover all cases (like signed payload inside an unsigned transaction), but should be a good enough example to start with. I believe that most complicated examples should become part of the recipes repository.

I've decided to make it separate from the pallet-example to avoid polluting the module with OCW-specific stuff. I think it's easier to digest both in isolation.

Since I'm not a great writer, I'd appreciate all feedback on the comments and ideas how to rephrase them. Also happy to accept direct commits to that branch to extend the documentation.

There are two significant changes to the substrate codebase, namely:

  1. We initialize the logger in executive. - Since our core pallets initialize it anyway, it doesn't make much sense to repeat that line in every OCW.
  2. Re-export sp_core::offchain from sp_runtime::offchain. The split there is pretty much arbitrary (the latter contains some traits that are needed by the client as well), so I thought it's easier to just have it in a single place.
tomusdrw added 18 commits Jan 28, 2020
tomusdrw added 3 commits Feb 19, 2020
Copy link
Contributor

JoshOrndorff left a comment

cc @jimmychu0807

This example is awesome! I would love to have it as part of the recipes. We have an issue for OCW recipe already. I also think this makes more sense in the recipes being an example of how to use a Substrate feature, rather than a re-usable pallet that others might include in their runtime. I would be happy to help move it there if you like.

frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
// Since the local storage is common for all offchain workers, it's a good practice
// to prepend your entry with the module name.
Comment on lines +246 to +247

This comment has been minimized.

Copy link
@JoshOrndorff

JoshOrndorff Feb 19, 2020

Contributor

It's common to more than just offchain workers right? Isn't it common to the entire node?

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Feb 20, 2020

Author Contributor

Yes, it's a single instance for the whole node. Should I rephrase that somehow to make it more clear?

frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Outdated Show resolved Hide resolved
frame/example-offchain-worker/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

kianenigma left a comment

I think this should indeed be in the 2.0 release. Just needs to have the spelling nits fixed + and my personal grumble that our preferred line-width repo-wide is 100 chars, not 60 or 80 :D but that might be a story for a different day.

Co-Authored-By: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
Co-Authored-By: Gavin Wood <gavin@parity.io>
@apopiak

This comment has been minimized.

Copy link
Contributor

apopiak commented Feb 20, 2020

I think this should indeed be in the 2.0 release. Just needs to have the spelling nits fixed + and my personal grumble that our preferred line-width repo-wide is 100 chars, not 60 or 80 :D but that might be a story for a different day.

I agree with Joshy's comment that it's odd to have this non-functional example in frame and that it's a better fit for the recipes or somewhere else in the repo (some pallet-examples directory or something).

@tomusdrw

This comment has been minimized.

Copy link
Contributor Author

tomusdrw commented Feb 20, 2020

I agree with Joshy's comment that it's odd to have this non-functional example in frame and that it's a better fit for the recipes or somewhere else in the repo (some pallet-examples directory or something).

I think the main premise to have pallet-example and node-template (and even node or frame) within the same repo is to make sure they are properly updated along the fast development cycle we have.
I feel that pallet-example-offchain-worker is in the same vein now, but I'm happy to revist that after we stabilise 2.0 and extract frame to it's own repo.

@gnunicorn

This comment has been minimized.

Copy link
Member

gnunicorn commented Feb 20, 2020

needs merge.

@kianenigma kianenigma merged commit 308273f into master Feb 20, 2020
12 checks passed
12 checks passed
continuous-integration/gitlab-cargo-check-benches Build stage: test; status: success
Details
continuous-integration/gitlab-cargo-check-subkey Build stage: test; status: success
Details
continuous-integration/gitlab-check-line-width Build stage: test; status: success
Details
continuous-integration/gitlab-check-runtime Build stage: test; status: success
Details
continuous-integration/gitlab-check-web-wasm Build stage: test; status: success
Details
continuous-integration/gitlab-check_warnings Build stage: build; status: success
Details
continuous-integration/gitlab-test-dependency-rules Build stage: test; status: success
Details
continuous-integration/gitlab-test-frame-staking Build stage: test; status: success
Details
continuous-integration/gitlab-test-full-crypto-feature Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable-int Build stage: test; status: success
Details
continuous-integration/gitlab-test-wasmtime Build stage: test; status: success
Details
@kianenigma kianenigma deleted the td-offchain-worker-example branch Feb 20, 2020
@tomusdrw tomusdrw added the B0-silent label Feb 21, 2020
General-Beck added a commit to General-Beck/substrate that referenced this pull request Mar 4, 2020
* Example of offchain worker pallet.

* Fix compilation issues.

* Use serde_json to parse JSON.

* Add some basic tests.

* Working on docs.

* Fix compilation

* Finish docs for signed.

* Work on unsigned send.

* Add some tests and missing docs.

* Add example of StorageValueRef

* Add weight.

* Extra \n

* Fix im-online test.

* Bump runtime.

* Fix tests.

* Apply suggestions from code review

Co-Authored-By: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
Co-Authored-By: Gavin Wood <gavin@parity.io>

* Address review comments.

Co-authored-by: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
Co-authored-by: Gavin Wood <github@gavwood.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.