Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

refactor Offchain Worker examples #14280

Open
wants to merge 77 commits into
base: master
Choose a base branch
from

Conversation

bernardoaraujor
Copy link
Contributor

As discussed in:

this PR is refactoring FRAME's Offchain Worker examples in order to mitigate common misunderstandings that we've been seeing in the community (which can result in security issues)

the original OCW example was actually split into two separate examples:

Ping Pong

inspired by https://gnunicorn.github.io/substrate-offchain-cb/

this example attempts to illustrate a scenario where some user input triggers some action by the OCWs... in this case, these are simple ping-pong events, where the pong can happen as many kinds of transactions, namely:

  • signed (coming from authorized/permissioned accounts)
  • unsigned
  • unsigned with signed payload

the comments and docs put a strong emphasis into the security issues that come from using unsigned transactions, and the user is encouraged to use signed transactions from authorized/permissioned accounts instead.

Price Oracle

follows the same spirit from the original example (a BTC/USD price oracle), but new prices can only be submitted by authorized/permissioned accounts, which removes the possibilty of malicious actors submitting false prices via unsigned transactions.

The main purpose of this example is to:

  • illustrate how to use authorized/permissioned accounts for OCWs
  • illustrate how to use HTTP calls to external resources (cryptocompare's API in this case)
  • illustrate how to coordinate potentially concurrent OCW threads via local offchain storage

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. and removed T1-runtime This PR/Issue is related to the topic “runtime”. labels Jul 27, 2023
@kianenigma
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Jul 27, 2023

@kianenigma https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3278607 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 30-b936f8bb-a8c2-489a-9e7f-7441dc86f786 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 27, 2023

@kianenigma Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3278607 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3278607/artifacts/download.

@paritytech-ci paritytech-ci requested a review from a team July 28, 2023 04:00
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Yea looks good, just some nits.

frame/examples/offchain-worker-ping-pong/Cargo.toml Outdated Show resolved Hide resolved
//! interact with an offchain worker asynchronously. It also showcases the
//! potential pitfalls and security considerations that come with it.
//!
//! It is based on [this example by
Copy link
Member

Choose a reason for hiding this comment

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

Do you know by any chance what license this was originally published under?

Choose a reason for hiding this comment

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

The original offchain-worker example pallet was published under Apache-2.0:

// SPDX-License-Identifier: Apache-2.0

frame/examples/offchain-worker-ping-pong/src/lib.rs Outdated Show resolved Hide resolved
frame/examples/offchain-worker-ping-pong/src/lib.rs Outdated Show resolved Hide resolved
frame/examples/offchain-worker-ping-pong/src/lib.rs Outdated Show resolved Hide resolved

/// A friendlier name for the error that is going to be returned in
/// case we are in the grace period.
const RECENTLY_SENT: () = ();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const RECENTLY_SENT: () = ();

I dont think this adds much.

Choose a reason for hiding this comment

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

I do understand the author's original intent to have a descriptive error. So in the places where it is used, replace it with an empty tuple?

Ok(Some(block)) if block_number < block + T::GracePeriod::get() =>
Err(RECENTLY_SENT),

Choose a reason for hiding this comment

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

Also want to note that this came from the original implementation:

Please let me know how you'd like me to move forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please return () as error without any alias, it is not a good rust pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

frame/examples/offchain-worker-price-oracle/src/lib.rs Outdated Show resolved Hide resolved
@bernardoaraujor bernardoaraujor removed the request for review from Daanvdplas July 28, 2023 17:36
brunopgalvao and others added 9 commits July 28, 2023 15:00
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
let val = StorageValueRef::persistent(b"example_ocw::last_send");
// The Local Storage is persisted and shared between runs of the
// offchain workers, and offchain workers may run concurrently. We
// can use the `mutate` function, to write a storage entry in an
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole line wrapping here seems 80 rather than 100 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma it seems that your comment is pointing to the code before this commit, which addresses line wrapping

here's the same snippet after the commit:

let val = StorageValueRef::persistent(b"example_ocw::last_send");
// The Local Storage is persisted and shared between runs of the offchain workers, and
// offchain workers may run concurrently. We can use the `mutate` function, to write a
// storage entry in an atomic fashion. Under the hood it uses `compare_and_set`
// low-level method of local storage API, which means that only one worker will be able
// to "acquire a lock" and send a transaction if multiple workers happen to be executed
// concurrently.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate-macos
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3369557

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.