diff --git a/README.md b/README.md index c5e2443..8b8425f 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Chainlink-polkadot -This repository contains all relevant project necessary to have [polkadot](https://polkadot.network/) and [substrate](https://www.parity.io/substrate/) chains interact with [chainlink](https://chain.link/). +This repository contains all relevant projects necessary to have [polkadot](https://polkadot.network/) and [substrate](https://www.parity.io/substrate/) chains interact with [chainlink](https://chain.link/). This is WIP and will evolve frequently. diff --git a/pallet-chainlink/Cargo.toml b/pallet-chainlink/Cargo.toml index df18330..29f37e0 100644 --- a/pallet-chainlink/Cargo.toml +++ b/pallet-chainlink/Cargo.toml @@ -6,6 +6,7 @@ edition = "2018" [dependencies] codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] } +# REVIEW: Use version released on crates.io. sp-std = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' } sp-runtime = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' } frame-support = { version = "2.0.0", default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '3e651110aa06aa835790df63410a29676243fc54' } diff --git a/pallet-chainlink/README.md b/pallet-chainlink/README.md index f9d43f7..a7be6ad 100644 --- a/pallet-chainlink/README.md +++ b/pallet-chainlink/README.md @@ -2,7 +2,7 @@ ## Purpose -This pallet allows to interract with [chainlink](https://chain.link/). +This pallet allows to interact with [chainlink](https://chain.link/). ## Installation @@ -21,6 +21,7 @@ Add the following section: [dependencies.chainlink] default_features = false package = 'pallet-chainlink' +git = 'https://github.com/smartcontractkit/chainlink-polkadot.git' ``` And amend the `std` section so that it shows like this: diff --git a/pallet-chainlink/src/lib.rs b/pallet-chainlink/src/lib.rs index b77a5da..ac2e037 100644 --- a/pallet-chainlink/src/lib.rs +++ b/pallet-chainlink/src/lib.rs @@ -1,14 +1,14 @@ //! # A pallet to interact with Chainlink nodes //! //! \## Overview -//! +//! //! `pallet-chainlink` allows to request external data from chainlink operators. This is done by emiting a well-known event (`OracleEvent`) //! embedding all required data. This event is listened by operators that in turns call back the `callback` function with the associated data. -//! +//! //! To initiate a request, users call `initiate_request` with the relevant details, the `operator` AccountId and the `fee` they agree to spend to get the result. -//! +//! //! To be valid, an operator must register its AccountId first hand via `register_operator`. -//! +//! //! \## Terminology //! Operator: a member of chainlink that provides result to requests, in exchange of a fee payment //! Request: details about what the user expects as result. Must match a Specification supported by an identified Operator @@ -18,12 +18,12 @@ use sp_std::if_std; #[warn(unused_imports)] -use codec::{Codec, Decode, Encode}; +use codec::Codec; use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, Parameter, dispatch::DispatchResult}; -use frame_support::traits::{Get, ReservableCurrency}; +use frame_support::traits::{Get, ReservableCurrency, Currency}; use sp_std::prelude::*; use sp_runtime::traits::Dispatchable; -use frame_system::{ensure_signed, RawOrigin}; +use frame_system::ensure_signed; use frame_system as system; // A trait allowing to inject Operator results back into the specified Call @@ -42,6 +42,9 @@ pub trait Trait: frame_system::Trait { type ValidityPeriod: Get; } +// REVIEW: Use this for transfering currency. +type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; + // Uniquely identify a request's specification understood by an Operator pub type SpecIndex = u32; // Uniquely identify a request for a considered Operator @@ -60,17 +63,19 @@ decl_storage! { // A map of details of each running request // TODO migrate to 'natural' hasher once migrated to 2.0 - pub Requests get(fn request): linked_map hasher(blake2_256) RequestIdentifier => (T::AccountId, Vec, T::BlockNumber, u32); + // REVIEW: Consider using a struct for the Requests instead of a tuple to increase + // readability. + pub Requests get(fn request): linked_map hasher(blake2_256) RequestIdentifier => (T::AccountId, Vec, T::BlockNumber, BalanceOf); } } decl_event!( - pub enum Event where AccountId = ::AccountId { + pub enum Event where AccountId = ::AccountId, Balance = BalanceOf { // A request has been accepted. Corresponding fee paiement is reserved - OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec, Vec, u32), + OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec, Vec, Balance), // A request has been answered. Corresponding fee paiement is transfered - OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec, u32), + OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec, Balance), // A new operator has been registered OperatorRegistered(AccountId), @@ -106,12 +111,13 @@ decl_module! { fn deposit_event() = default; + // REVIEW: Use `///` instead of `//` to make these doc comments that are part of the crate documentation. // Register a new Operator. // Fails with `OperatorAlreadyRegistered` if this Operator (identified by `origin`) has already been registered. pub fn register_operator(origin) -> DispatchResult { let who : ::AccountId = ensure_signed(origin)?; - ensure!(!>::exists(who.clone()), Error::::OperatorAlreadyRegistered); + ensure!(!>::exists(&who), Error::::OperatorAlreadyRegistered); Operators::::insert(&who, true); @@ -138,18 +144,29 @@ decl_module! { // If provided fee is sufficient, Operator must send back the request result in `callback` Extrinsic which then will dispatch back to the request originator callback identified by `callback`. // The fee is `reserved` and only actually transferred when the result is provided in the callback. // Operators are expected to listen to `OracleRequest` events. This event contains all the required information to perform the request and provide back the result. - pub fn initiate_request(origin, operator: T::AccountId, spec_index: SpecIndex, data_version: DataVersion, data: Vec, fee: u32, callback: ::Callback) -> DispatchResult { + // REVIEW: Use a `BalanceOf` type for the fee instead of `u32` as shown here: https://substrate.dev/recipes/3-entrees/currency.html + pub fn initiate_request(origin, operator: T::AccountId, spec_index: SpecIndex, data_version: DataVersion, data: Vec, fee: BalanceOf, callback: ::Callback) -> DispatchResult { let who : ::AccountId = ensure_signed(origin.clone())?; - ensure!(>::exists(operator.clone()), Error::::UnknownOperator); - ensure!(fee > 0, Error::::InsufficientFee); + ensure!(>::exists(&operator), Error::::UnknownOperator); + // REVIEW: Should probably be at least `ExistentialDeposit` + ensure!(fee > BalanceOf::::from(0), Error::::InsufficientFee); T::Currency::reserve(&who, fee.into())?; let request_id = NextRequestIdentifier::get(); + // REVIEW: This can overflow. You can make a maximum of `u64::max_value()` requests. + // Default behavior for `u64` is to wrap around to 0, but you might want to + // make this explicit. + // I think using `wrapping_add` could be fine here, because it should be fine to + // start at 0 when you reach `u64::max_value()`. NextRequestIdentifier::put(request_id + 1); + // REVIEW: Is it intentional that requests are only valid during the current block? let now = frame_system::Module::::block_number(); + // REVIEW: You might want to think about and document that your requests can be overwritten + // as soon as the request id wraps around. + // REVIEW: Is the `Vec` intended for forward compatibility? It seems superfluous here. Requests::::insert(request_id.clone(), (operator.clone(), vec![callback], now, fee)); Self::deposit_event(RawEvent::OracleRequest(operator, spec_index, request_id, who, data_version, data, "Chainlink.callback".into(), fee)); @@ -164,11 +181,17 @@ decl_module! { fn callback(origin, request_id: RequestIdentifier, result: Vec) -> DispatchResult { let who : ::AccountId = ensure_signed(origin.clone())?; - ensure!(>::exists(request_id.clone()), Error::::UnknownRequest); - ensure!(>::get(request_id.clone()).0 == who, Error::::WrongOperator); - - let (operator, callback, _, fee) = >::take(request_id.clone()); - + ensure!(>::exists(&request_id), Error::::UnknownRequest); + let (operator, callback, _, fee) = >::get(&request_id); + ensure!(operator == who, Error::::WrongOperator); + + // REVIEW: This does not make sure that the fee is payed. `repatriate_reserved` removes + // *up to* the amount passed. [See here](https://substrate.dev/rustdocs/master/frame_support/traits/trait.ReservableCurrency.html#tymethod.repatriate_reserved) + // Check `reserved_balance()` to make sure that the fee is payable via this method. + // Maybe use a different payment method and check `total_balance()`. I don't know + // Substrate's Currency module well enough to tell. + // REVIEW: This happens *after* the request is `take`n from storage. Is that intended? + // See ["verify first, write last"](https://substrate.dev/recipes/2-appetizers/1-hello-substrate.html#inside-a-dispatchable-call) motto. T::Currency::repatriate_reserved(&who, &operator, fee.into())?; // Dispatch the result to the original callback registered by the caller @@ -181,8 +204,6 @@ decl_module! { // Identify requests that are considered dead and remove them fn on_finalize(n: T::BlockNumber) { - // Adding a request requires a fee, enumeration is safe - // TODO replace 'enumerate' with 'iter' once available for (request_identifier, (_account_id, _data, block_number, _fee)) in Requests::::enumerate() { if n > block_number + T::ValidityPeriod::get() { // No result has been received in time @@ -200,6 +221,7 @@ decl_module! { mod tests { use super::*; + use codec::{Decode, Encode}; use frame_support::{impl_outer_event, impl_outer_origin, parameter_types, weights::Weight}; use sp_core::H256; // The testing primitives are very useful for avoiding having to work with signatures @@ -281,7 +303,7 @@ mod tests { } type System = frame_system::Module; - + fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); balances::GenesisConfig::{ @@ -294,9 +316,9 @@ mod tests { mod module2 { use super::*; - + pub trait Trait: frame_system::Trait {} - + frame_support::decl_module! { pub struct Module for enum Call where origin: ::Origin @@ -323,7 +345,7 @@ mod tests { } } } - + } #[test] @@ -380,7 +402,7 @@ mod tests { let parameters = ("a", "b"); let data = parameters.encode(); assert!(>::initiate_request(Origin::signed(2), 1, 1, 1, data.clone(), 2, module2::Call::::callback(vec![]).into()).is_ok()); - + assert_eq!( *System::events().last().unwrap(), EventRecord { @@ -413,4 +435,4 @@ mod tests { } -} \ No newline at end of file +} diff --git a/substrate-node-example/runtime/src/example.rs b/substrate-node-example/runtime/src/example.rs index 834591e..86fa0a1 100644 --- a/substrate-node-example/runtime/src/example.rs +++ b/substrate-node-example/runtime/src/example.rs @@ -23,7 +23,7 @@ decl_module! { let parameters = ("get", "https://min-api.cryptocompare.com/data/pricemultifull?fsyms=ETH&tsyms=USD", "path", "RAW.ETH.USD.PRICE", "times", "100000000"); let call: ::Callback = Call::callback(vec![]).into(); - >::initiate_request(origin, operator, 1, 0, parameters.encode(), 100, call.into())?; + >::initiate_request(origin, operator, 1, 0, parameters.encode(), 100.into(), call.into())?; Ok(()) }