Project Review

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.
 

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' }

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:

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<Self::BlockNumber>;
 }
 
+// REVIEW: Use this for transfering currency.
+type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::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::Callback>, 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::Callback>, T::BlockNumber, BalanceOf<T>);
     }
 }
 
 decl_event!(
-	pub enum Event<T> where AccountId = <T as frame_system::Trait>::AccountId {
+	pub enum Event<T> where AccountId = <T as frame_system::Trait>::AccountId, Balance = BalanceOf<T> {
 		// A request has been accepted. Corresponding fee paiement is reserved
-		OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec<u8>, Vec<u8>, u32),
+		OracleRequest(AccountId, SpecIndex, RequestIdentifier, AccountId, DataVersion, Vec<u8>, Vec<u8>, Balance),
 
 		// A request has been answered. Corresponding fee paiement is transfered
-		OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec<u8>, u32),
+		OracleAnswer(AccountId, RequestIdentifier, AccountId, Vec<u8>, 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 : <T as frame_system::Trait>::AccountId = ensure_signed(origin)?;
 
-			ensure!(!<Operators<T>>::exists(who.clone()), Error::<T>::OperatorAlreadyRegistered);
+			ensure!(!<Operators<T>>::exists(&who), Error::<T>::OperatorAlreadyRegistered);
 
 			Operators::<T>::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<u8>, fee: u32, callback: <T as Trait>::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<u8>, fee: BalanceOf<T>, callback: <T as Trait>::Callback) -> DispatchResult {
 			let who : <T as frame_system::Trait>::AccountId = ensure_signed(origin.clone())?;
 
-			ensure!(<Operators<T>>::exists(operator.clone()), Error::<T>::UnknownOperator);
-			ensure!(fee > 0, Error::<T>::InsufficientFee);
+			ensure!(<Operators<T>>::exists(&operator), Error::<T>::UnknownOperator);
+			// REVIEW: Should probably be at least `ExistentialDeposit`
+			ensure!(fee > BalanceOf::<T>::from(0), Error::<T>::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::<T>::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::<T>::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<u8>) -> DispatchResult {
 			let who : <T as frame_system::Trait>::AccountId = ensure_signed(origin.clone())?;
 
-			ensure!(<Requests<T>>::exists(request_id.clone()), Error::<T>::UnknownRequest);
-			ensure!(<Requests<T>>::get(request_id.clone()).0 == who, Error::<T>::WrongOperator);
-
-			let (operator, callback, _, fee) = <Requests<T>>::take(request_id.clone());
-
+			ensure!(<Requests<T>>::exists(&request_id), Error::<T>::UnknownRequest);
+                        let (operator, callback, _, fee) = <Requests<T>>::get(&request_id);
+			ensure!(operator == who, Error::<T>::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::<T>::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<Runtime>;
-	
+
 	fn new_test_ext() -> sp_io::TestExternalities {
 		let mut t = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
 		balances::GenesisConfig::<Runtime>{
@@ -294,9 +316,9 @@ mod tests {
 
 	mod module2 {
 		use super::*;
-	
+
 		pub trait Trait: frame_system::Trait {}
-	
+
 		frame_support::decl_module! {
 			pub struct Module<T: Trait> for enum Call
 				where origin: <T as frame_system::Trait>::Origin
@@ -323,7 +345,7 @@ mod tests {
 				}
 			}
 		}
-	
+
 	}
 
 	#[test]
@@ -380,7 +402,7 @@ mod tests {
 			let parameters = ("a", "b");
 			let data = parameters.encode();
 			assert!(<Module<Runtime>>::initiate_request(Origin::signed(2), 1, 1, 1, data.clone(), 2, module2::Call::<Runtime>::callback(vec![]).into()).is_ok());
-			
+
 			assert_eq!(
 				*System::events().last().unwrap(),
 				EventRecord {
@@ -413,4 +435,4 @@ mod tests {
 
 	}
 
-}
+}

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: <T as Trait>::Callback = Call::callback(vec![]).into();
 
-            <chainlink::Module<T>>::initiate_request(origin, operator, 1, 0, parameters.encode(), 100, call.into())?;
+            <chainlink::Module<T>>::initiate_request(origin, operator, 1, 0, parameters.encode(), 100.into(), call.into())?;
 
             Ok(())
         }