-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor Offchain Worker examples #14280
base: master
Are you sure you want to change the base?
Conversation
…rdless of block number)
bot fmt |
@kianenigma https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3278607 was started for your command Comment |
@kianenigma Command |
There was a problem hiding this 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.
//! 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
|
||
/// A friendlier name for the error that is going to be returned in | ||
/// case we are in the grace period. | ||
const RECENTLY_SENT: () = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RECENTLY_SENT: () = (); |
I dont think this adds much.
There was a problem hiding this comment.
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?
substrate/frame/examples/offchain-worker-price-oracle/src/lib.rs
Lines 257 to 258 in a08af90
Ok(Some(block)) if block_number < block + T::GracePeriod::get() => | |
Err(RECENTLY_SENT), |
There was a problem hiding this comment.
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:
Err(RECENTLY_SENT),
Please let me know how you'd like me to move forward.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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 🙄
There was a problem hiding this comment.
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:
substrate/frame/examples/offchain-worker-price-oracle/src/lib.rs
Lines 230 to 236 in 5ec0432
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. |
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
The CI pipeline was cancelled due to failure one of the required jobs. |
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 thepong
can happen as many kinds of transactions, namely: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:
cryptocompare
's API in this case)