diff --git a/client/js-wasm/Cargo.toml b/client/js-wasm/Cargo.toml index e5521a0..6227272 100644 --- a/client/js-wasm/Cargo.toml +++ b/client/js-wasm/Cargo.toml @@ -28,6 +28,7 @@ nolik-metadata = { path = "../metadata" } getrandom = { version = "0.2", default-features = false, features = ["js"] } crypto_box = { version = "0.8" } js-sys = "0.3.61" +# SBP-M1 review: Please remove this commented dependency # anyhow = "1.0" diff --git a/client/js-wasm/README.md b/client/js-wasm/README.md index 96ef5ff..35dc2d4 100644 --- a/client/js-wasm/README.md +++ b/client/js-wasm/README.md @@ -1,4 +1,5 @@ ## 🚴 Usage +SBP-M1 review: Please add a some module's description ### 🛠️ Build with `wasm-pack build` diff --git a/client/js-wasm/src/lib.rs b/client/js-wasm/src/lib.rs index d9844b8..c7ae4fb 100644 --- a/client/js-wasm/src/lib.rs +++ b/client/js-wasm/src/lib.rs @@ -136,6 +136,7 @@ pub fn new_encrypted_metadata( let public_nonce = Nonce::::from_slice(public_nonce.as_slice()); let sender_pk = PublicKey::from(js_value_to_array::(sender_pk.into())?); + // SBP-M1 review: public nonce can be passed directly as the value is being dereferenced by the compiler immediately let (meta, secret_nonce) = MessageMetadata::new_encrypted( &origin, &public_nonce, diff --git a/client/metadata/src/lib.rs b/client/metadata/src/lib.rs index b73b1bd..4e26492 100644 --- a/client/metadata/src/lib.rs +++ b/client/metadata/src/lib.rs @@ -25,6 +25,8 @@ mod ffi { aead::{AeadCore, OsRng}, PublicKey, SalsaBox, SecretKey, }; + + // SBP-M1 review: Please complete ToDos // TODO: use scale codec instead of serde_json, also on a client use serde::{Deserialize, Serialize}; use std::{ diff --git a/client/metadata/src/messages.rs b/client/metadata/src/messages.rs index 673d602..8d99f8a 100644 --- a/client/metadata/src/messages.rs +++ b/client/metadata/src/messages.rs @@ -69,6 +69,7 @@ where D: Deserializer<'a>, { use serde::de::Error; + // SBP-M1 review: no need to borrow, direct string can be passed in decode function String::deserialize(deserializer).and_then(|string| { general_purpose::STANDARD .decode(&string) diff --git a/node/src/benchmarking.rs b/node/src/benchmarking.rs index ccde23b..4b21d2c 100644 --- a/node/src/benchmarking.rs +++ b/node/src/benchmarking.rs @@ -95,6 +95,7 @@ impl frame_benchmarking_cli::ExtrinsicBuilder for TransferKeepAliveBuilder { "transfer_keep_alive" } + // SBP-M1 review: No need to use into() for conversion in self.value, as value is u128 already fn build(&self, nonce: u32) -> std::result::Result { let acc = Sr25519Keyring::Bob.pair(); let extrinsic: OpaqueExtrinsic = create_benchmark_extrinsic( @@ -160,6 +161,7 @@ pub fn create_benchmark_extrinsic( ); let signature = raw_payload.using_encoded(|e| sender.sign(e)); + // SBP-M1 review: redundant clone, can be removed runtime::UncheckedExtrinsic::new_signed( call.clone(), sp_runtime::AccountId32::from(sender.public()).into(), diff --git a/node/src/rpc.rs b/node/src/rpc.rs index a51696e..c292a3b 100644 --- a/node/src/rpc.rs +++ b/node/src/rpc.rs @@ -45,6 +45,7 @@ where let mut module = RpcModule::new(()); let FullDeps { client, pool, deny_unsafe } = deps; + // SBP-M1 review: No need to use clone for pool, module.merge(System::new(client.clone(), pool.clone(), deny_unsafe).into_rpc())?; module.merge(TransactionPayment::new(client).into_rpc())?; diff --git a/node/src/service.rs b/node/src/service.rs index 13b2336..7abe89b 100644 --- a/node/src/service.rs +++ b/node/src/service.rs @@ -147,6 +147,7 @@ pub fn new_partial( }) } +// SBP-M1 review: Try to deal with static memory (if possible), &str can be incorporated instead of &String fn remote_keystore(_url: &String) -> Result, &'static str> { // FIXME: here would the concrete keystore be built, // must return a concrete type (NOT `LocalKeystore`) that diff --git a/pallets/nolik/README.md b/pallets/nolik/README.md index a336371..4ae1c1e 100644 --- a/pallets/nolik/README.md +++ b/pallets/nolik/README.md @@ -1,6 +1,7 @@ # Pallet Nolik A [Substrate](https://substrate.io) FRAME pallet for sending encrypted messages between blockchain accounts +SBP-M1 review: Typo should be fixed (eg: echange) ## Overview Nolik is a protocol for secure and verifiable data echange between parties that do not trust each other. It is designed to connect people without any form of censorship or third-party control. @@ -44,4 +45,4 @@ Required parameters The main functionality is covered by unit tests. To run the tests use the command: -`cargo test` \ No newline at end of file +`cargo test` diff --git a/pallets/nolik/src/lib.rs b/pallets/nolik/src/lib.rs index 97b5028..6e066e1 100644 --- a/pallets/nolik/src/lib.rs +++ b/pallets/nolik/src/lib.rs @@ -69,16 +69,20 @@ pub mod pallet { /// bytes so no encoding and no heap allocation is needed prior to putting the message to /// off-chain storage. #[pallet::call_index(0)] + // SBP-M1 review: Implement benchmarking and use benchmarked weight #[pallet::weight(10_000)] pub fn send_message( origin: OriginFor, metadata: MessageMetadata, + // SBP-M1 review: BoundedVec should be used to improve security message: Vec, ) -> DispatchResult { let account = ensure_signed(origin)?; Self::check_message(&message, &metadata)?; let counter = MessageCounter::::get(); + + // SBP-M1: Can be simplified like this `counter.checked_add(1).ok_or(>::MessageCounterOverflow)?` let (counter, overflowed) = counter.overflowing_add(1); // u128 should not overflow, practically impossible if overflowed { @@ -86,6 +90,7 @@ pub mod pallet { } let key = Self::derived_key(&account, counter - 1); + // SBP-M1 review: please remove commented code // frame_support::log::info!("The offchain key !!! {:02x?}", key); // save message to offchain storage