From b5ab0b1c86560744e1d72832245f6d5a5ae2c596 Mon Sep 17 00:00:00 2001 From: Frank Bell Date: Thu, 3 Aug 2023 19:12:54 +0100 Subject: [PATCH] SBP-M1 review comments --- crates/sugarfunge-api-types/Cargo.toml | 4 ++ crates/sugarfunge-api-types/src/account.rs | 4 ++ crates/sugarfunge-api-types/src/asset.rs | 6 +++ crates/sugarfunge-api-types/src/bag.rs | 4 ++ crates/sugarfunge-api-types/src/bundle.rs | 3 ++ crates/sugarfunge-api-types/src/challenge.rs | 4 ++ crates/sugarfunge-api-types/src/contract.rs | 1 + crates/sugarfunge-api-types/src/fula.rs | 10 +++++ crates/sugarfunge-api-types/src/lib.rs | 4 ++ crates/sugarfunge-api-types/src/market.rs | 5 +++ crates/sugarfunge-api-types/src/pool.rs | 5 +++ crates/sugarfunge-api-types/src/primitives.rs | 5 +++ crates/sugarfunge-api-types/src/validator.rs | 2 + src/account.rs | 8 ++++ src/args.rs | 1 + src/asset.rs | 17 ++++++++ src/bag.rs | 6 ++- src/bundle.rs | 8 ++++ src/challenge.rs | 18 +++++++- src/config.rs | 1 + src/contract.rs | 13 ++++++ src/fula.rs | 43 +++++++++++++++++++ src/main.rs | 2 + src/market.rs | 6 +++ src/pool.rs | 23 +++++++++- src/subscription.rs | 4 ++ src/util.rs | 1 + src/validator.rs | 4 +- 28 files changed, 208 insertions(+), 4 deletions(-) diff --git a/crates/sugarfunge-api-types/Cargo.toml b/crates/sugarfunge-api-types/Cargo.toml index a72c9ed..2f9157d 100644 --- a/crates/sugarfunge-api-types/Cargo.toml +++ b/crates/sugarfunge-api-types/Cargo.toml @@ -3,6 +3,7 @@ name = "sugarfunge-api-types" version = "0.1.0" edition = "2021" +#SBP-M1 review: remove comment # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] @@ -10,8 +11,11 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive", "full", "bit-vec"] } scale-info = { version = "2.0.0", features = ["bit-vec"] } +#SBP-M1 review: consider updating bevy_derive = "0.10" +#SBP-M1 review: use inline dependencies as above, rather than [dependencoes.X] +#SBP-M1 review: assumed subxt to be somewhat version agnostic, relying on metadata retrieved from the node for extrinsic encoding/decoding. Consider whether this fork is really required. [dependencies.subxt] git = "https://github.com/SugarFunge/subxt.git" branch = "feature/polkadot-v0.9.42" diff --git a/crates/sugarfunge-api-types/src/account.rs b/crates/sugarfunge-api-types/src/account.rs index 9301980..68af9f1 100644 --- a/crates/sugarfunge-api-types/src/account.rs +++ b/crates/sugarfunge-api-types/src/account.rs @@ -1,17 +1,20 @@ use crate::primitives::*; use serde::{Deserialize, Serialize}; +// SBP-M1 review: hardcoded values seem problematic when progressing beyond dev-chain as dev accounts are well known pub const REFUND_SEED: &str = "//Alice"; pub const REFUND_FEE_VALUE: u128 = 20000000000000000; #[derive(Serialize, Deserialize, Debug)] pub struct CreateAccountOutput { + // SBP-M1 review: remove seed pub seed: Seed, pub account: Account, } #[derive(Serialize, Deserialize, Debug)] pub struct FundAccountInput { + // SBP-M1 review: remove seed pub seed: Seed, pub to: Account, pub amount: Balance, @@ -47,6 +50,7 @@ pub struct AccountExistsOutput { #[derive(Serialize, Deserialize, Debug)] pub struct SeededAccountInput { + // SBP-M1 review: remove seed pub seed: Seed, } diff --git a/crates/sugarfunge-api-types/src/asset.rs b/crates/sugarfunge-api-types/src/asset.rs index b526a27..ded99ee 100644 --- a/crates/sugarfunge-api-types/src/asset.rs +++ b/crates/sugarfunge-api-types/src/asset.rs @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Debug)] pub struct CreateClassInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub metadata: serde_json::Value, @@ -34,6 +35,7 @@ pub struct ClassInfoOutput { #[derive(Serialize, Deserialize, Debug)] pub struct CreateInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub asset_id: AssetId, @@ -67,6 +69,7 @@ pub struct AssetInfoOutput { #[derive(Serialize, Deserialize, Debug)] pub struct UpdateMetadataInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub asset_id: AssetId, @@ -83,6 +86,7 @@ pub struct UpdateMetadataOutput { #[derive(Serialize, Deserialize, Debug)] pub struct MintInput { + // SBP-M1 review: remove seed pub seed: Seed, pub to: Account, pub class_id: ClassId, @@ -101,6 +105,7 @@ pub struct MintOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BurnInput { + // SBP-M1 review: remove seed pub seed: Seed, pub from: Account, pub class_id: ClassId, @@ -150,6 +155,7 @@ pub struct AssetBalanceItemOutput { #[derive(Serialize, Deserialize, Debug)] pub struct TransferFromInput { + // SBP-M1 review: remove seed pub seed: Seed, pub from: Account, pub to: Account, diff --git a/crates/sugarfunge-api-types/src/bag.rs b/crates/sugarfunge-api-types/src/bag.rs index ed97049..da3d187 100644 --- a/crates/sugarfunge-api-types/src/bag.rs +++ b/crates/sugarfunge-api-types/src/bag.rs @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Debug)] pub struct RegisterInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub metadata: serde_json::Value, @@ -16,6 +17,7 @@ pub struct RegisterOutput { #[derive(Serialize, Deserialize, Debug)] pub struct CreateInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub owners: Vec, @@ -32,6 +34,7 @@ pub struct CreateOutput { #[derive(Serialize, Deserialize, Debug)] pub struct SweepInput { + // SBP-M1 review: remove seed pub seed: Seed, pub bag: Account, pub to: Account, @@ -46,6 +49,7 @@ pub struct SweepOutput { #[derive(Serialize, Deserialize, Debug)] pub struct DepositInput { + // SBP-M1 review: remove seed pub seed: Seed, pub bag: Account, pub class_ids: Vec, diff --git a/crates/sugarfunge-api-types/src/bundle.rs b/crates/sugarfunge-api-types/src/bundle.rs index 077239f..0e3f372 100644 --- a/crates/sugarfunge-api-types/src/bundle.rs +++ b/crates/sugarfunge-api-types/src/bundle.rs @@ -10,6 +10,7 @@ pub struct BundleSchema { #[derive(Serialize, Deserialize, Debug)] pub struct RegisterBundleInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub asset_id: AssetId, @@ -27,6 +28,7 @@ pub struct RegisterBundleOutput { #[derive(Serialize, Deserialize, Debug)] pub struct MintBundleInput { + // SBP-M1 review: remove seed pub seed: Seed, pub from: Account, pub to: Account, @@ -45,6 +47,7 @@ pub struct MintBundleOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BurnBundleInput { + // SBP-M1 review: remove seed pub seed: Seed, pub from: Account, pub to: Account, diff --git a/crates/sugarfunge-api-types/src/challenge.rs b/crates/sugarfunge-api-types/src/challenge.rs index e6b00d3..26ca653 100644 --- a/crates/sugarfunge-api-types/src/challenge.rs +++ b/crates/sugarfunge-api-types/src/challenge.rs @@ -23,6 +23,7 @@ impl Into for ChallengeState { #[derive(Serialize, Deserialize, Debug)] pub struct GenerateChallengeInput { + // SBP-M1 review: remove seed pub seed: Seed, } @@ -38,6 +39,7 @@ pub struct GenerateChallengeOutput { #[derive(Serialize, Deserialize, Debug)] pub struct VerifyChallengeInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub cids: Vec, @@ -56,6 +58,7 @@ pub struct VerifyChallengeOutput { #[derive(Serialize, Deserialize, Debug)] pub struct MintLaborTokensInput { + // SBP-M1 review: remove seed pub seed: Seed, pub class_id: ClassId, pub asset_id: AssetId, @@ -100,6 +103,7 @@ pub struct VerifyFileSizeOutput { #[derive(Serialize, Deserialize, Debug)] pub struct ProvideFileSizeInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub cids: Vec, diff --git a/crates/sugarfunge-api-types/src/contract.rs b/crates/sugarfunge-api-types/src/contract.rs index 5bf8e14..f85ebd3 100644 --- a/crates/sugarfunge-api-types/src/contract.rs +++ b/crates/sugarfunge-api-types/src/contract.rs @@ -26,6 +26,7 @@ pub struct ContractAllowanceOutput { #[derive(Serialize, Deserialize, Debug)] pub struct ConvertFulaInput { + // SBP-M1 review: remove seed pub seed: Seed, pub wallet_account: String, pub amount: Balance, diff --git a/crates/sugarfunge-api-types/src/fula.rs b/crates/sugarfunge-api-types/src/fula.rs index 4e34731..0a42ccb 100644 --- a/crates/sugarfunge-api-types/src/fula.rs +++ b/crates/sugarfunge-api-types/src/fula.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Debug)] pub struct VerifyManifestsInput { + // SBP-M1 review: remove seed pub seed: Seed, } @@ -19,6 +20,7 @@ pub struct VerifyManifestsOutput { #[derive(Serialize, Deserialize, Debug)] pub struct UploadManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub manifest_metadata: serde_json::Value, pub cid: Cid, @@ -38,6 +40,7 @@ pub struct UploadManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BatchUploadManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub manifest_metadata: Vec, pub cid: Vec, @@ -56,6 +59,7 @@ pub struct BatchUploadManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct UpdateManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub cid: Cid, pub pool_id: PoolId, @@ -78,6 +82,7 @@ pub struct UpdatedManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct StorageManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub cid: Cid, pub pool_id: PoolId, @@ -94,6 +99,7 @@ pub struct StorageManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BatchStorageManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub cid: Vec, @@ -110,6 +116,7 @@ pub struct BatchStorageManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct RemoveManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub cid: Cid, pub pool_id: PoolId, @@ -125,6 +132,7 @@ pub struct RemoveManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BatchRemoveManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: Vec, pub cid: Vec, @@ -140,6 +148,7 @@ pub struct BatchRemoveManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct RemoveStoringManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub cid: Cid, pub pool_id: PoolId, @@ -155,6 +164,7 @@ pub struct RemoveStoringManifestOutput { #[derive(Serialize, Deserialize, Debug)] pub struct BatchRemoveStoringManifestInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub cid: Vec, diff --git a/crates/sugarfunge-api-types/src/lib.rs b/crates/sugarfunge-api-types/src/lib.rs index d6c6113..912b210 100644 --- a/crates/sugarfunge-api-types/src/lib.rs +++ b/crates/sugarfunge-api-types/src/lib.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: add doc comments to all types and fields, which can be used for generation API docs #[subxt::subxt( runtime_metadata_path = "sugarfunge_metadata.scale", derive_for_type( @@ -20,3 +21,6 @@ pub mod market; pub mod pool; pub mod primitives; pub mod validator; + +// SBP-M1 review: this crate uses subxt as a dependency and is itself used by the proof-engine. +// SBP-M1 review: A recommendation is to add a simple API to this crate which facilitates signing of extrinsics on the 'client', rather than within the 'sugarfunge-api' via seeds being passed around. diff --git a/crates/sugarfunge-api-types/src/market.rs b/crates/sugarfunge-api-types/src/market.rs index ad22cf4..741a0d0 100644 --- a/crates/sugarfunge-api-types/src/market.rs +++ b/crates/sugarfunge-api-types/src/market.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: no tests use crate::primitives::*; use serde::{Deserialize, Serialize}; @@ -210,6 +211,7 @@ pub struct Rates { #[derive(Serialize, Deserialize, Debug)] pub struct CreateMarketInput { + // SBP-M1 review: remove seed pub seed: Seed, pub market_id: MarketId, } @@ -222,6 +224,7 @@ pub struct CreateMarketOutput { #[derive(Serialize, Deserialize, Debug)] pub struct CreateMarketRateInput { + // SBP-M1 review: remove seed pub seed: Seed, pub market_id: MarketId, pub market_rate_id: MarketId, @@ -236,6 +239,7 @@ pub struct CreateMarketRateOutput { #[derive(Serialize, Deserialize, Debug)] pub struct DepositAssetsInput { + // SBP-M1 review: remove seed pub seed: Seed, pub market_id: MarketId, pub market_rate_id: MarketId, @@ -254,6 +258,7 @@ pub struct DepositAssetsOutput { #[derive(Serialize, Deserialize, Debug)] pub struct ExchangeAssetsInput { + // SBP-M1 review: remove seed pub seed: Seed, pub market_id: MarketId, pub market_rate_id: MarketId, diff --git a/crates/sugarfunge-api-types/src/pool.rs b/crates/sugarfunge-api-types/src/pool.rs index 50d01b8..8bd6b5f 100644 --- a/crates/sugarfunge-api-types/src/pool.rs +++ b/crates/sugarfunge-api-types/src/pool.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Debug)] pub struct CreatePoolInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_name: Name, pub peer_id: PeerId, @@ -21,6 +22,7 @@ pub struct CreatePoolOutput { #[derive(Serialize, Deserialize, Debug)] pub struct LeavePoolInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, } @@ -35,6 +37,7 @@ pub struct LeavePoolOutput { #[derive(Serialize, Deserialize, Debug)] pub struct JoinPoolInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub peer_id: PeerId, @@ -50,6 +53,7 @@ pub struct JoinPoolOutput { #[derive(Serialize, Deserialize, Debug)] pub struct CancelJoinPoolInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, } @@ -64,6 +68,7 @@ pub struct CancelJoinPoolOutput { #[derive(Serialize, Deserialize, Debug)] pub struct VoteInput { + // SBP-M1 review: remove seed pub seed: Seed, pub pool_id: PoolId, pub account: Account, diff --git a/crates/sugarfunge-api-types/src/primitives.rs b/crates/sugarfunge-api-types/src/primitives.rs index 81c638f..e4635b5 100644 --- a/crates/sugarfunge-api-types/src/primitives.rs +++ b/crates/sugarfunge-api-types/src/primitives.rs @@ -1,3 +1,5 @@ +// SBP-M1 review: group use statements without line breaks +// SBP-M1 review: no tests use std::{ops::Div, str::FromStr}; use serde::{Deserialize, Serialize}; @@ -9,6 +11,8 @@ use sp_core::U256; use bevy_derive::{Deref, DerefMut}; #[derive(Serialize, Deserialize, Clone, Debug, Deref, DerefMut)] +// SBP-M1 review: remove type, seed should not be passed to an API for signing but rather receive a signed extrinsic/transaction for passing to the chain for execution +// SBP-M1 review: look at how https://github.com/paritytech/substrate-api-sidecar implements this at https://paritytech.github.io/substrate-api-sidecar/dist/ pub struct Seed(String); impl From for Seed { @@ -297,6 +301,7 @@ impl From for u16 { } } +// SBP-M1 review: consider replacing the below helper methods with From/Into trait implementations where possible, so transformation can be simply achieved via .into() pub fn transform_vec_account_to_string(in_vec: Vec) -> Vec { in_vec .into_iter() diff --git a/crates/sugarfunge-api-types/src/validator.rs b/crates/sugarfunge-api-types/src/validator.rs index 1338e33..da64039 100644 --- a/crates/sugarfunge-api-types/src/validator.rs +++ b/crates/sugarfunge-api-types/src/validator.rs @@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize}; #[derive(Serialize, Deserialize, Debug)] pub struct AddValidatorInput { + // SBP-M1 review: remove seed pub seed: Seed, pub validator_id: ValidatorId, } @@ -14,6 +15,7 @@ pub struct AddValidatorOutput { #[derive(Serialize, Deserialize, Debug)] pub struct RemoveValidatorInput { + // SBP-M1 review: remove seed pub seed: Seed, pub validator_id: ValidatorId, } diff --git a/src/account.rs b/src/account.rs index 15779ac..9c85e7c 100644 --- a/src/account.rs +++ b/src/account.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpRequest, HttpResponse}; @@ -11,6 +12,7 @@ use sugarfunge_api_types::primitives::*; use sugarfunge_api_types::sugarfunge; /// Generate a unique seed and its associated account +// SBP-M1 review: the seed should be generated on the client (where transactions are signed) and never leave the device. Remove this functionality. pub async fn create(_req: HttpRequest) -> error::Result { let seed = rand::thread_rng().gen::<[u8; 32]>(); let seed = hex::encode(seed); @@ -26,6 +28,7 @@ pub async fn create(_req: HttpRequest) -> error::Result { } /// Compute account from seed +// SBP-M1 review: the seed should never leave the device. Remove this functionality. pub async fn seeded(req: web::Json) -> error::Result { let pair = get_pair_from_seed(&req.seed)?; let account = pair.public().into_account(); @@ -40,7 +43,9 @@ pub async fn fund( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; + // SBP-M1 review: remove commented out code //let signer = sp_core::sr25519::Pair::try_from(pair).unwrap(); let signer = PairSigner::new(pair); let account = subxt::utils::AccountId32::try_from(&req.to).map_err(map_account_err)?; @@ -52,6 +57,7 @@ pub async fn fund( .balances() .transfer(account, amount_input.into()); + // SBP-M1 review: https://github.com/paritytech/subxt/blob/059723e4313d91e8ca0bcd84b0dd7dd66686ca50/subxt/src/tx/tx_client.rs#L429 let result = api .tx() .sign_and_submit_then_watch(&call, &signer, Default::default()) @@ -86,6 +92,7 @@ pub async fn balance( let call = sugarfunge::storage().system().account(&account); + // SBP-M1 review: remove commented out code //let result = api.storage().fetch(&call, None).await; let block = api.blocks().at_latest().await.unwrap(); let data = block.storage().fetch(&call).await.unwrap(); @@ -125,6 +132,7 @@ pub async fn exists( } } +// SBP-M1 review: remove seed pub async fn refund_fees(data: web::Data, seed: &Seed) -> error::Result { let result_fund = fund( data, diff --git a/src/args.rs b/src/args.rs index 079998f..7acfeff 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,4 +1,5 @@ use clap::Parser; +// SBP-M1 review: remove commented out code // use structopt::StructOpt; use url::Url; diff --git a/src/asset.rs b/src/asset.rs index 38d2e11..2793aec 100644 --- a/src/asset.rs +++ b/src/asset.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpResponse}; @@ -16,6 +17,7 @@ pub async fn create_class( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let to = sp_core::sr25519::Public::from_str(req.owner.as_str()).map_err(map_account_err)?; @@ -83,6 +85,7 @@ pub async fn create( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let metadata: Vec = serde_json::to_vec(&req.metadata).unwrap_or_default(); @@ -150,6 +153,7 @@ pub async fn update_metadata( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let metadata = serde_json::to_vec(&req.metadata).unwrap_or_default(); @@ -192,6 +196,7 @@ pub async fn mint( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let to = subxt::utils::AccountId32::try_from(&req.to).map_err(map_account_err)?; @@ -235,6 +240,7 @@ pub async fn burn( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let from = subxt::utils::AccountId32::try_from(&req.from).map_err(map_account_err)?; @@ -319,14 +325,18 @@ pub async fn balances( .asset() .balances_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key balances_root len: {}", query_key.len()); query_key.extend(subxt::ext::sp_core::blake2_128(&account.encode())); + // SBP-M1 review: remove commented out code // println!("query_key account len: {}", query_key.len()); if let Some(class_id) = req.class_id { let class_id: u64 = class_id.into(); query_key.extend(subxt::ext::sp_core::blake2_128(&class_id.encode())); + // SBP-M1 review: remove commented out code // println!("query_key class_id len: {}", query_key.len()); } + // SBP-M1 review: remove commented out code // if let Some(asset_id) = req.asset_id { // let asset_id: u64 = asset_id.into(); // StorageMapKey::new(&asset_id, StorageHasher::Blake2_128Concat).to_bytes(&mut query_key); @@ -340,10 +350,13 @@ pub async fn balances( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); + // SBP-M1 review: remove commented out code // let account_idx = 48; // let account_key = key.0.as_slice()[account_idx..(account_idx + 32)].to_vec(); // let account_id = AccountId32::decode(&mut &account_key[..]); @@ -354,17 +367,20 @@ pub async fn balances( let class_idx = 96; let class_key = key.0.as_slice()[class_idx..(class_idx + 8)].to_vec(); let class_id = u64::decode(&mut &class_key[..]); + // SBP-M1 review: remove commented out code // println!("class_id: {:?}", class_id); let asset_idx = 120; let asset_key = key.0.as_slice()[asset_idx..(asset_idx + 8)].to_vec(); let asset_id = u64::decode(&mut &asset_key[..]); + // SBP-M1 review: remove commented out code // println!("asset_id: {:?}", asset_id); let storage = api.storage().at_latest().await.map_err(map_subxt_err)?; if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { let value = u128::decode(&mut &storage_data[..]); + // SBP-M1 review: remove commented out code // println!( // "Class_Id: {:?} AssetId: {:?} Value: {:?}", // class_id, asset_id, value @@ -388,6 +404,7 @@ pub async fn transfer_from( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let account_from = subxt::utils::AccountId32::try_from(&req.from).map_err(map_account_err)?; diff --git a/src/bag.rs b/src/bag.rs index 641c462..824171e 100644 --- a/src/bag.rs +++ b/src/bag.rs @@ -1,5 +1,5 @@ use std::str::FromStr; - +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpResponse}; @@ -15,6 +15,7 @@ pub async fn register( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let metadata: Vec = serde_json::to_vec(&req.metadata).unwrap_or_default(); @@ -66,6 +67,7 @@ pub async fn create( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let owners = transform_owners_input(transform_vec_account_to_string(req.owners.clone())); @@ -106,6 +108,7 @@ pub async fn sweep( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let bag = sp_core::crypto::AccountId32::try_from(&req.bag).map_err(map_account_err)?; @@ -142,6 +145,7 @@ pub async fn deposit( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let bag = subxt::utils::AccountId32::try_from(&req.bag).map_err(map_account_err)?; diff --git a/src/bundle.rs b/src/bundle.rs index c89a3f7..7a6b7db 100644 --- a/src/bundle.rs +++ b/src/bundle.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::Error; @@ -23,6 +24,7 @@ pub async fn register_bundle( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let schema = ( @@ -84,6 +86,7 @@ pub async fn mint_bundle( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let account_from = subxt::utils::AccountId32::try_from(&req.from).map_err(map_account_err)?; @@ -128,6 +131,7 @@ pub async fn burn_bundle( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let account_from = subxt::utils::AccountId32::try_from(&req.from).map_err(map_account_err)?; @@ -185,16 +189,19 @@ pub async fn get_bundles_id(data: web::Data) -> error::Result) -> error::Result, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -59,6 +60,7 @@ pub async fn verify_challenge( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -103,6 +105,7 @@ pub async fn mint_labor_tokens( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -152,6 +155,7 @@ pub async fn verify_pending_challenge( .challenge_requests_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key account_to len: {}", query_key.len()); let storage = api.storage().at_latest().await.map_err(map_subxt_err)?; @@ -161,12 +165,14 @@ pub async fn verify_pending_challenge( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let account_idx = 48; let account_key = key.0.as_slice()[account_idx..(account_idx + 32)].to_vec(); let account_id = AccountId32::decode(&mut &account_key[..]); let account_id = Account::from(account_id.unwrap()); + // SBP-M1 review: remove commented out code // println!("account_id: {:?}", account_id); if AccountId32::from(Public::from_str(&account_id.as_str()).map_err(map_account_err)?) @@ -193,6 +199,7 @@ pub async fn verify_file_size( .manifests_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key account_to len: {}", query_key.len()); let storage = api.storage().at_latest().await.map_err(map_subxt_err)?; @@ -202,12 +209,14 @@ pub async fn verify_file_size( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let cid_idx = 68; let cid_key = key.0.as_slice()[cid_idx..].to_vec(); let cid_id = String::decode(&mut &cid_key[..]); let cid_id = cid_id.unwrap(); + // SBP-M1 review: remove commented out code // println!("cid_id: {:?}", cid_id); if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { @@ -238,6 +247,7 @@ pub async fn provide_file_size( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -284,6 +294,7 @@ pub async fn get_challenges(data: web::Data) -> error::Result) -> error::Result) -> error::Result) -> error::Result Config { + // SBP-M1 review: typo let panic_message: String = "enviroment variable is not set".to_string(); Config { diff --git a/src/contract.rs b/src/contract.rs index f6e162f..1e3f621 100644 --- a/src/contract.rs +++ b/src/contract.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: nest all use statements use crate::bundle::*; use crate::config; use crate::state::AppState; @@ -143,6 +144,7 @@ pub async fn goerli_convert_to_fula( // Create the bundle schema + // SBP-M1 review: remove commented out code // println!("1. CREATING SCHEMA"); let schema = ( @@ -160,11 +162,13 @@ pub async fn goerli_convert_to_fula( let api = &data.api; // Verify if the Bundle_id exist + // SBP-M1 review: remove commented out code // println!("2. VERIFYING IF THE BUNDLE ID EXIST"); if let Ok(verification) = verify_bundle_exist(&data, bundle_id.encode_hex()).await { // If it doesn't exist, register the bundle if !verification { + // SBP-M1 review: remove commented out code // println!("3. THE BUNDLE ID DOESN'T EXISTS"); let call = sugarfunge::tx().bundle().register_bundle( env.claimed_token_class_id.into(), @@ -182,10 +186,12 @@ pub async fn goerli_convert_to_fula( .wait_for_finalized_success() .await .map_err(map_sf_err)?; + // SBP-M1 review: remove commented out code // println!("4. BUNDLE CREATED"); }; // If exist, continue + // SBP-M1 review: remove commented out code // println!("5. THE BUNDLE ID EXISTS"); // Mint the Bundle with the bundle_id @@ -210,6 +216,7 @@ pub async fn goerli_convert_to_fula( match result { Some(_) => { // If the bundle mint is successful, execute the contract mint + // SBP-M1 review: remove commented out code // println!("6. BUNDLE MINTED SUCCESSFULLY"); let result = goerli_mint_to( req.wallet_account.as_str(), @@ -257,6 +264,7 @@ pub async fn mumbai_convert_to_fula( // Create the bundle schema + // SBP-M1 review: remove commented out code // println!("1. CREATING SCHEMA"); let schema = ( @@ -274,11 +282,13 @@ pub async fn mumbai_convert_to_fula( let api = &data.api; // Verify if the Bundle_id exist + // SBP-M1 review: remove commented out code // println!("2. VERIFYING IF THE BUNDLE ID EXIST"); if let Ok(verification) = verify_bundle_exist(&data, bundle_id.encode_hex()).await { // If it doesn't exist, register the bundle if !verification { + // SBP-M1 review: remove commented out code // println!("3. THE BUNDLE ID DOESN'T EXISTS"); let call = sugarfunge::tx().bundle().register_bundle( env.claimed_token_class_id.into(), @@ -296,10 +306,12 @@ pub async fn mumbai_convert_to_fula( .wait_for_finalized_success() .await .map_err(map_sf_err)?; + // SBP-M1 review: remove commented out code // println!("4. BUNDLE CREATED"); }; // If exist, continue + // SBP-M1 review: remove commented out code // println!("5. THE BUNDLE ID EXISTS"); // Mint the Bundle with the bundle_id @@ -324,6 +336,7 @@ pub async fn mumbai_convert_to_fula( match result { Some(_) => { // If the bundle mint is successful, execute the contract mint + // SBP-M1 review: remove commented out code // println!("6. BUNDLE MINTED SUCCESSFULLY"); let result = mumbai_mint_to( req.wallet_account.as_str(), diff --git a/src/fula.rs b/src/fula.rs index 5933479..1bc495c 100644 --- a/src/fula.rs +++ b/src/fula.rs @@ -1,14 +1,18 @@ +// SBP-M1 review: nest use statements use crate::account; use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpResponse}; +// SBP-M1 review: nest use statements use codec::Decode; use codec::Encode; use serde_json::json; use std::str::FromStr; +// SBP-M1 review: nest use statements use subxt::ext::sp_core::sr25519::Public; use subxt::tx::PairSigner; use subxt::utils::AccountId32; +// SBP-M1 review: nest use statements use sugarfunge_api_types::fula::*; use sugarfunge_api_types::primitives::*; use sugarfunge_api_types::sugarfunge; @@ -19,12 +23,14 @@ use sugarfunge_api_types::sugarfunge::runtime_types::functionland_fula::{ UploaderData as UploaderDataRuntime, }; use sugarfunge_api_types::sugarfunge::runtime_types::sp_core::bounded::bounded_vec::BoundedVec; +// SBP-M1 review: remove commented out code // use sugarfunge_api_types::sugarfunge::runtime_types::sp_runtime::bounded::bounded_vec::BoundedVec; pub async fn upload_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -75,6 +81,7 @@ pub async fn batch_upload_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -121,6 +128,7 @@ pub async fn storage_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let cid: Vec = String::from(&req.cid.clone()).into_bytes(); @@ -163,6 +171,7 @@ pub async fn batch_storage_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -202,6 +211,7 @@ pub async fn remove_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let cid: Vec = String::from(&req.cid.clone()).into_bytes(); @@ -243,6 +253,7 @@ pub async fn batch_remove_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -283,9 +294,11 @@ pub async fn remove_stored_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let cid: Vec = String::from(&req.cid.clone()).into_bytes(); + // SBP-M1 review: remove commented out code // let cid: Vec = serde_json::to_vec(&req.cid.clone()).unwrap_or_default(); let cid = BoundedVec(cid); @@ -326,6 +339,7 @@ pub async fn batch_remove_stored_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -365,6 +379,7 @@ pub async fn verify_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed.clone())?; let signer = PairSigner::new(pair); @@ -402,6 +417,7 @@ pub async fn update_manifest( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -456,12 +472,14 @@ pub async fn get_all_manifests( .fula() .manifests_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key manifests_root len: {}", query_key.len()); if let Some(value) = req.pool_id.clone() { let key_value: u32 = value.into(); query_key.extend(subxt::ext::sp_core::blake2_128(&key_value.encode())); } + // SBP-M1 review: remove commented out code // println!("query_key account_to len: {}", query_key.len()); let storage = api.storage().at_latest().await.map_err(map_subxt_err)?; @@ -471,9 +489,11 @@ pub async fn get_all_manifests( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let mut meet_requirements = true; + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let pool_id_idx = 48; @@ -535,6 +555,7 @@ pub async fn get_available_manifests( .fula() .manifests_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key manifests_root len: {}", query_key.len()); if let Some(value) = req.pool_id.clone() { @@ -549,8 +570,10 @@ pub async fn get_available_manifests( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let pool_id_idx = 48; let pool_id_key = key.0.as_slice()[pool_id_idx..(pool_id_idx + 4)].to_vec(); @@ -589,11 +612,13 @@ pub async fn get_all_manifests_storer_data( .fula() .manifests_storer_data_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key manifests_root len: {}", query_key.len()); if let Some(value) = req.pool_id.clone() { let key_value: u32 = value.into(); query_key.extend(subxt::ext::sp_core::blake2_128(&key_value.encode())); + // SBP-M1 review: remove commented out code // println!("query_key pool_id len: {}", query_key.len()); } @@ -604,27 +629,32 @@ pub async fn get_all_manifests_storer_data( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let mut meet_requirements = true; + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let pool_id_idx = 48; let pool_id_key = key.0.as_slice()[pool_id_idx..(pool_id_idx + 4)].to_vec(); let pool_id_id = u32::decode(&mut &pool_id_key[..]); let pool_id = pool_id_id.unwrap(); + // SBP-M1 review: remove commented out code // println!("pool_id: {:?}", pool_id); let account_idx = 68; let account_key = key.0.as_slice()[account_idx..(account_idx + 32)].to_vec(); let account_id = AccountId32::decode(&mut &account_key[..]); let account_id = Account::from(account_id.unwrap()); + // SBP-M1 review: remove commented out code // println!("account_id: {:?}", account_id); let cid_idx = 116; let cid_key = key.0.as_slice()[cid_idx..].to_vec(); let cid_id = String::decode(&mut &cid_key[..]); let cid_id = cid_id.unwrap(); + // SBP-M1 review: remove commented out code // println!("cid_id: {:?}", cid_id); if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { @@ -659,6 +689,8 @@ pub async fn get_all_manifests_storer_data( })) } +// SBP-M1 review: should not submit an extrinsic to query data, rather use a runtime-api +// SBP-M1 review: subxt examples at https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_raw.rs and https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_static.rs pub async fn get_all_manifests_alter( data: web::Data, req: web::Json, @@ -667,6 +699,8 @@ pub async fn get_all_manifests_alter( let uploader = transform_option_account_value_reverse(req.uploader.clone()).await; let storer = transform_option_account_value_reverse(req.storer.clone()).await; + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. + // SBP-M1 review: hard-coding bad practice, should be configurable as //Alice should not be used outside of development let pair = get_pair_from_seed(&Seed::from(String::from("//Alice")))?; let signer = PairSigner::new(pair); @@ -698,12 +732,16 @@ pub async fn get_all_manifests_alter( } } +// SBP-M1 review: should not submit an extrinsic to query data, rather use a runtime-api +// SBP-M1 review: subxt examples at https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_raw.rs and https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_static.rs pub async fn get_all_available_manifests_alter( data: web::Data, req: web::Json, ) -> error::Result { let pool_id = transform_option_pool_id_value_reverse(req.pool_id); + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. + // SBP-M1 review: hard-coding bad practice, should be configurable as //Alice should not be used outside of development let pair = get_pair_from_seed(&Seed::from(String::from("//Alice")))?; let signer = PairSigner::new(pair); @@ -733,6 +771,8 @@ pub async fn get_all_available_manifests_alter( } } +// SBP-M1 review: should not submit an extrinsic to query data, rather use a runtime-api +// SBP-M1 review: subxt examples at https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_raw.rs and https://github.com/paritytech/subxt/blob/master/subxt/examples/runtime_apis_static.rs pub async fn get_all_manifests_storer_data_alter( data: web::Data, req: web::Json, @@ -741,6 +781,8 @@ pub async fn get_all_manifests_storer_data_alter( let storer = transform_option_account_value_reverse(req.storer.clone()).await; + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. + // SBP-M1 review: hard-coding bad practice, should be configurable as //Alice should not be used outside of development let pair = get_pair_from_seed(&Seed::from(String::from("//Alice")))?; let signer = PairSigner::new(pair); @@ -861,6 +903,7 @@ pub fn get_vec_pool_id_from_node(pool_ids: Vec) -> Vec { } pub fn get_vec_replication_factor_from_input( + // SBP-M1 review: typo repliaction_factors: Vec, ) -> Vec { return repliaction_factors diff --git a/src/main.rs b/src/main.rs index 93f59b6..5020b9a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,5 @@ +// SBP-M1 review: add doc comments to all functions to enable generation of API docs - e.g. https://github.com/juhaku/utoipa +// SBP-M1 review: add tests for all api endpoints use actix_cors::Cors; use actix_web::{ http, middleware, diff --git a/src/market.rs b/src/market.rs index b268e0d..2af9221 100644 --- a/src/market.rs +++ b/src/market.rs @@ -1,3 +1,4 @@ +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpResponse}; @@ -9,6 +10,7 @@ use sugarfunge_api_types::sugarfunge; use sugarfunge_api_types::sugarfunge::runtime_types::sp_core::bounded::bounded_vec::BoundedVec; use sugarfunge_api_types::sugarfunge::runtime_types::sugarfunge_market; +// SBP-M1 review: typo fn extrinsinc_rates( in_rates: &[AssetRate], ) -> BoundedVec> { @@ -40,6 +42,7 @@ pub async fn create_market( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let api = &data.api; @@ -75,6 +78,7 @@ pub async fn create_market_rate( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let api = &data.api; @@ -116,6 +120,7 @@ pub async fn deposit_assets( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let api = &data.api; @@ -157,6 +162,7 @@ pub async fn exchange_assets( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let api = &data.api; diff --git a/src/pool.rs b/src/pool.rs index 04ef503..d29dd8a 100644 --- a/src/pool.rs +++ b/src/pool.rs @@ -1,5 +1,5 @@ use std::str::FromStr; - +// SBP-M1 review: nest all use statements use crate::account; use crate::state::*; use crate::util::*; @@ -17,12 +17,14 @@ use sugarfunge_api_types::sugarfunge::runtime_types::fula_pool::Pool as PoolRunt use sugarfunge_api_types::sugarfunge::runtime_types::fula_pool::PoolRequest as PoolRequestRuntime; use sugarfunge_api_types::sugarfunge::runtime_types::fula_pool::User as UserRuntime; use sugarfunge_api_types::sugarfunge::runtime_types::sp_core::bounded::bounded_vec::BoundedVec; +// SBP-M1 review: remove commented out code // use sugarfunge_api_types::sugarfunge::runtime_types::sp_runtime::bounded::bounded_vec::BoundedVec; pub async fn create_pool( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -66,6 +68,7 @@ pub async fn leave_pool( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -103,6 +106,7 @@ pub async fn join_pool( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -143,6 +147,7 @@ pub async fn cancel_join_pool( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -180,6 +185,7 @@ pub async fn vote( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); @@ -229,6 +235,7 @@ pub async fn get_all_pools( let mut result_array = Vec::new(); let query_key = sugarfunge::storage().pool().pools_root().to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key pool_root len: {}", query_key.len()); let storage = api.storage().at_latest().await.map_err(map_subxt_err)?; @@ -238,15 +245,18 @@ pub async fn get_all_pools( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let mut meet_requirements = true; + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let pool_id_idx = 48; let pool_id_key = key.0.as_slice()[pool_id_idx..(pool_id_idx + 4)].to_vec(); let pool_id_id = u32::decode(&mut &pool_id_key[..]); let pool_id = pool_id_id.unwrap(); + // SBP-M1 review: remove commented out code // println!("pool_id: {:?}", pool_id); if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { @@ -300,11 +310,13 @@ pub async fn get_all_pool_requests( .pool() .pool_requests_root() .to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key pool_root len: {}", query_key.len()); if let Some(value) = req.pool_id.clone() { let key_value: u32 = value.into(); query_key.extend(subxt::ext::sp_core::blake2_128(&key_value.encode())); + // SBP-M1 review: remove commented out code // println!("query_key pool_id len: {}", query_key.len()); } @@ -315,21 +327,25 @@ pub async fn get_all_pool_requests( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let mut meet_requirements = true; + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let pool_id_idx = 48; let pool_id_key = key.0.as_slice()[pool_id_idx..(pool_id_idx + 4)].to_vec(); let pool_id_id = u32::decode(&mut &pool_id_key[..]); let pool_id = pool_id_id.unwrap(); + // SBP-M1 review: remove commented out code // println!("pool_id: {:?}", pool_id); let account_idx = 68; let account_key = key.0.as_slice()[account_idx..(account_idx + 32)].to_vec(); let account_id = AccountId32::decode(&mut &account_key[..]); let account_id = Account::from(account_id.unwrap()); + // SBP-M1 review: remove commented out code // println!("account_id: {:?}", account_id); if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { @@ -381,8 +397,10 @@ pub async fn get_all_pool_users( let mut result_array = Vec::new(); let query_key = sugarfunge::storage().pool().users_root().to_root_bytes(); + // SBP-M1 review: remove commented out code // println!("query_key pool_root len: {}", query_key.len()); + // SBP-M1 review: remove commented out code // if let Some(account_value) = req.account.clone() { // let account = AccountId32::try_from(&account_value).map_err(map_account_err)?; // StorageMapKey::new(account, StorageHasher::Blake2_128Concat).to_bytes(&mut query_key); @@ -396,15 +414,18 @@ pub async fn get_all_pool_users( .await .map_err(map_subxt_err)?; + // SBP-M1 review: remove commented out code // println!("Obtained keys:"); for key in keys.iter() { let mut meet_requirements = true; + // SBP-M1 review: remove commented out code // println!("Key: len: {} 0x{}", key.0.len(), hex::encode(&key)); let account_idx = 48; let account_key = key.0.as_slice()[account_idx..(account_idx + 32)].to_vec(); let account_id = AccountId32::decode(&mut &account_key[..]); let account_id = Account::from(account_id.unwrap()); + // SBP-M1 review: remove commented out code // println!("account_id: {:?}", account_id); if let Some(storage_data) = storage.fetch_raw(&key.0).await.map_err(map_subxt_err)? { diff --git a/src/subscription.rs b/src/subscription.rs index 38b93a7..deaafdd 100644 --- a/src/subscription.rs +++ b/src/subscription.rs @@ -4,14 +4,17 @@ use actix_web::{web, Error, HttpRequest, HttpResponse}; use actix_web_actors::ws; use crossbeam::channel; use futures::StreamExt; +// SBP-M1 review: nest all use statements use std::collections::HashMap; use std::time::{Duration, Instant}; use sugarfunge_api_types::sugarfunge; /// How often heartbeat pings are sent +// SBP-M1 review: consider making configurable via cli arg const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(5); /// How long before lack of client response causes a timeout +// SBP-M1 review: consider making configurable via cli arg const CLIENT_TIMEOUT: Duration = Duration::from_secs(10); /// websocket connection is long running connection @@ -242,6 +245,7 @@ impl StreamHandler> for SubcriptionServic } } +// SBP-M1 review: typo /// WebSocket handshake and start `SubcriptionServiceWS` actor. pub async fn ws( data: web::Data, diff --git a/src/util.rs b/src/util.rs index 4607719..987bb7f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -48,6 +48,7 @@ pub fn map_account_err(e: sp_core::crypto::PublicError) -> actix_web::Error { error::ErrorBadRequest(req_error) } +// SBP-M1 review: remove, signing should be done on the client. pub fn get_pair_from_seed(seed: &Seed) -> error::Result { sp_core::sr25519::Pair::from_string(seed.as_str(), None).map_err(|e| { let req_error = RequestError { diff --git a/src/validator.rs b/src/validator.rs index 42700eb..5016f09 100644 --- a/src/validator.rs +++ b/src/validator.rs @@ -1,5 +1,5 @@ use std::str::FromStr; - +// SBP-M1 review: nest all use statements use crate::state::*; use crate::util::*; use actix_web::{error, web, HttpResponse}; @@ -13,6 +13,7 @@ pub async fn add_validator( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let validator_id = @@ -54,6 +55,7 @@ pub async fn remove_validator( data: web::Data, req: web::Json, ) -> error::Result { + // SBP-M1 review: seed should never leave the client. This extrinsic should be created on the client, signed and then the resulting bytes either submitted directly to chain, or relayed via this API. Remove this functionality. let pair = get_pair_from_seed(&req.seed)?; let signer = PairSigner::new(pair); let validator_id =