Bug: Implement HashiCorp Vault support in the Card Vault




diff --git a/crates/common_utils/src/custom_serde.rs b/crates/common_utils/src/custom_serde.rs
index d7bd9721507..4aa132c4555 100644
--- a/crates/common_utils/src/custom_serde.rs
+++ b/crates/common_utils/src/custom_serde.rs
@@ -64,6 +64,9 @@ pub mod iso8601 {
         where
             S: Serializer,
         {
+            // FIXME(kos): There is no need to repeat the whole
+            // serialization/deserialization logic here. Just reuse
+            // it from the module above.
             date_time
                 .map(|date_time| date_time.assume_utc().format(&Iso8601::<FORMAT_CONFIG>))
                 .transpose()
diff --git a/crates/common_utils/src/ext_traits.rs b/crates/common_utils/src/ext_traits.rs
index 83770e6ca8c..12651910a79 100644
--- a/crates/common_utils/src/ext_traits.rs
+++ b/crates/common_utils/src/ext_traits.rs
@@ -211,6 +211,8 @@ impl<T> ValueExt<T> for serde_json::Value {
     where
         T: serde::de::DeserializeOwned,
     {
+        // FIXME(kos): The syntax `{self:?}` could be used here without any
+        // impact on semantics.
         let debug = format!(
             "Unable to parse {type_name} from serde_json::Value: {:?}",
             &self
diff --git a/crates/common_utils/src/pii.rs b/crates/common_utils/src/pii.rs
index 97b395b26d7..c219ff55398 100644
--- a/crates/common_utils/src/pii.rs
+++ b/crates/common_utils/src/pii.rs
@@ -56,12 +56,32 @@ where
 #[derive(Debug)]
 pub struct Email;
 
+// FIXME(kos): Here should be not generic T, but newtype Email.
 impl<T> Strategy<T> for Email
 where
     T: AsRef<str>,
 {
     fn fmt(val: &T, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let val_str: &str = val.as_ref();
+        // FIXME(kos): We should assume constructed value object is already validated.
+        // Email validation is quite a heavy operation. Doing it on
+        // each formatting is quite a subtle performance penalty,
+        // while being... unnecessary?
+        // Another problem, that validation here is
+        // violation of the "Separation of concerns" design
+        // principle. Formatting is not a validation in any way.
+        // https://en.wikipedia.org/wiki/Separation_of_concerns
+        // Consider to provide a newtype for email strings.
+        // https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html
+        // This way you do the validation only once, when creating a
+        // value of the type, and then you may fearlessly reuse it
+        // as the type system protects you.
+        // https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate
+        // Thus, as the result, you will be able to remove any
+        // validation code from the formatting, as the compiler will
+        // guarantee that you would have valid values here.
+        // The same is true for other formatting strategies in this
+        // module too, as they're effectively validators too.
         let is_valid = validate_email(val_str);
 
         if is_valid.is_err() {
@@ -80,11 +100,14 @@ where
 #[derive(Debug)]
 pub struct IpAddress;
 
+// FIXME(kos): Here should be not generic T, but std::net::IpAddr.
 impl<T> Strategy<T> for IpAddress
 where
     T: AsRef<str>,
 {
     fn fmt(val: &T, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // TODO(kos): Consider to not use `String`s for IP addresses.
+        // There is a `std::net::IpAddr` for that.
         let val_str: &str = val.as_ref();
         let segments: Vec<&str> = val_str.split('.').collect();
 
diff --git a/crates/masking/src/bytes.rs b/crates/masking/src/bytes.rs
index e828f8a78e0..e1b415e36ee 100644
--- a/crates/masking/src/bytes.rs
+++ b/crates/masking/src/bytes.rs
@@ -77,6 +77,27 @@ impl<'de> Deserialize<'de> for SecretBytesMut {
             where
                 V: de::SeqAccess<'de>,
             {
+                // FIXME(kos): Should be improved. Current logic:
+                // 1. If there is some `size_hint`, then we
+                //     pre-allocate 4096 at max, and later, when
+                //     filling the bytes will re-allocate the memory
+                //     if `size_hint` > 4096.
+                // 2. If there is no `size_hint`, then we
+                //     pre-allocate 0 bytes, and re-allocate normally
+                //     when filling the bytes.
+                // So, what's no benefits of these moves?
+                // It seems more feasible to have:
+                // ```rust
+                // let len = seq.size_hint().unwrap_or(0);
+                // ```
+                // This way:
+                // 1. If there is some `size_hint`, then we
+                //     pre-allocate the exact size and fill it with
+                //     the bytes without re-allocations.
+                // 2. If there is no `size_hint`, then we
+                //     pre-allocate 4096 bytes, and re-allocate only
+                //     if the real size is bigger than 4096 bytes.
+
                 // 4096 is cargo culted from upstream
                 let len = core::cmp::min(seq.size_hint().unwrap_or(0), 4096);
                 let mut bytes = BytesMut::with_capacity(len);
diff --git a/crates/masking/src/strong_secret.rs b/crates/masking/src/strong_secret.rs
index fd1335f6993..2f591e51305 100644
--- a/crates/masking/src/strong_secret.rs
+++ b/crates/masking/src/strong_secret.rs
@@ -9,6 +9,13 @@ use zeroize::{self, Zeroize as ZeroizableSecret};
 
 use crate::{strategy::Strategy, PeekInterface};
 
+// FIXME(kos) : It would be convenient to implement Deref<Target=Thing> for Secret<Thing>
+// This would allow getting rid of `peek()`.
+
+// FIXME(kos): Documentation should explain how it differs from the `Secret`
+// type and why it's called "strong". Save the reader's time and
+// free him from wondering and parsing the `impl`s of this type to
+// understand the difference.
 ///
 /// Secret thing.
 ///
@@ -62,6 +69,13 @@ where
     Self: PeekInterface<Secret>,
     Secret: ZeroizableSecret + StrongEq,
 {
+    // FIXME(kos): Usual comparison is not safe in cryptography as is lazy
+    // (fail-fast) and makes the code potentially vulnerable to
+    // timing attacks:
+    // https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
+    // Use crates like `subtle` for constant-time comparison of
+    // secret values:
+    // https://docs.rs/subtle
     fn eq(&self, other: &Self) -> bool {
         StrongEq::strong_eq(self.peek(), other.peek())
     }
diff --git a/crates/redis_interface/src/commands.rs b/crates/redis_interface/src/commands.rs
index 13eeee5043a..831d714321b 100644
--- a/crates/redis_interface/src/commands.rs
+++ b/crates/redis_interface/src/commands.rs
@@ -101,6 +101,8 @@ impl super::RedisConnectionPool {
         type_name: &str,
     ) -> CustomResult<T, errors::RedisError>
     where
+        // FIXME(kos): Just `Send` and `Sync` should be enough. No need in full path
+        // naming.
         T: serde::de::DeserializeOwned,
     {
         let value_bytes = self.get_key::<Vec<u8>>(key).await?;
diff --git a/crates/redis_interface/src/types.rs b/crates/redis_interface/src/types.rs
index 129c587298e..64b9044f16e 100644
--- a/crates/redis_interface/src/types.rs
+++ b/crates/redis_interface/src/types.rs
@@ -41,6 +41,9 @@ impl From<RedisEntryId> for fred::types::XID {
             } => XID::Manual(fred::bytes_utils::format_bytes!(
                 "{milliseconds}-{sequence_number}"
             )),
+            // FIXME(kos): Why do these extra allocations here if we just need
+            // `&[u8]`? Either use `Cow` or `out.write_arg()`
+            // directly im match arms.
             RedisEntryId::AutoGeneratedID => XID::Auto,
             RedisEntryId::AfterLastID => XID::Max,
             RedisEntryId::UndeliveredEntryID => XID::NewInGroup,
diff --git a/crates/router/src/configs/settings.rs b/crates/router/src/configs/settings.rs
index 03d67edfa54..69047780e0b 100644
--- a/crates/router/src/configs/settings.rs
+++ b/crates/router/src/configs/settings.rs
@@ -91,7 +91,22 @@ pub struct Server {
 
 #[derive(Debug, Deserialize, Clone)]
 pub struct Database {
+    // TODO(kos): Consider using `Box<str>` here (and in similar places), as we
+    // don't need any `String` mutation capabilities for this field
+    // (after parsing it will only be read, but never mutated).
     pub username: String,
+    // FIXME(kos): Any kind of secrets, we control, should be wrapped into
+    // Secret, which will:
+    // 1. Protect any accidental secrets leaks to logs, traces or
+    //     any other `Debug` prints.
+    // 2. Correctly zeroize the memory of secrets on `Drop` to avoid
+    //     accidental secrets leaks when process is crashed and core
+    //     dumped (by accessing the core dump file), process memory
+    //     is swapped, etc.
+    // There is a similar, extended, but custom made functionality
+    // in the `masking` crate. So, the team is aware of the problem.
+    // But why then the `masking::Secret` is not used here and in
+    // similar places?
     pub password: String,
     pub host: String,
     pub port: u16,
@@ -168,6 +183,12 @@ impl Settings {
                 include_str!("defaults.toml"),
                 FileFormat::Toml,
             ))
+            // FIXME(kos): `.required(true)` seems to be unnecessarily strict
+            // here. It's totally common scenario to run a process
+            // and provide it with environment variables only,
+            // without any config files. So, requiring a config file
+            // always to exist will be only a stumble factor for
+            // users.
             .add_source(File::from(config_path).required(true))
             .add_source(
                 Environment::with_prefix("ROUTER")
@@ -180,6 +201,16 @@ impl Settings {
             .build()?;
 
         serde_path_to_error::deserialize(config).map_err(|error| {
+            // FIXME(kos): We can improve it.
+            // Usually, the logging system is initialized after the
+            // config parsing is done, as the config itself would
+            // likely have settings for logging setup. That's why
+            // doing logging here before the parsed config is
+            // returned is kinda strange thing.
+            // We can simply return the error.
+            // Let the caller decide what to do with this
+            // error. Hardcoded printing side effects here may be
+            // undesired by the caller.
             logger::error!(%error, "Unable to deserialize application configuration");
             eprintln!("Unable to deserialize application configuration: {error}");
             BachError::from(error.into_inner())
diff --git a/crates/router/src/connector/adyen/transformers.rs b/crates/router/src/connector/adyen/transformers.rs
index a4944f1c4f4..3817b8dd8a9 100644
--- a/crates/router/src/connector/adyen/transformers.rs
+++ b/crates/router/src/connector/adyen/transformers.rs
@@ -156,6 +156,7 @@ pub struct AdyenCard {
     number: Option<Secret<String>>,
     expiry_month: Option<Secret<String>>,
     expiry_year: Option<Secret<String>>,
+    // FIXME(kos): CVC is a secret too!
     cvc: Option<String>,
 }
 
@@ -306,6 +307,9 @@ impl TryFrom<&types::PaymentsAuthorizeRouterData> for AdyenPaymentRequest {
                     expiry_year: ccard.map(|x| x.card_exp_year.peek().clone().into()),
                     // TODO: CVV/CVC shouldn't be saved in our db
                     // Will need to implement tokenization that allows us to make payments without cvv
+                    // FIXME(kos): And this could be easily prevented if it was
+                    // represented as a newtype for value object which doesn't implement
+                    // `ToSql`.
                     cvc: ccard.map(|x| x.card_cvc.peek().into()),
                 };
 
diff --git a/crates/router/src/connector/authorizedotnet/transformers.rs b/crates/router/src/connector/authorizedotnet/transformers.rs
index 9e3815a767a..79b6a2858e2 100644
--- a/crates/router/src/connector/authorizedotnet/transformers.rs
+++ b/crates/router/src/connector/authorizedotnet/transformers.rs
@@ -41,6 +41,7 @@ impl TryFrom<&types::ConnectorAuthType> for MerchantAuthentication {
 #[derive(Serialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 struct CreditCardDetails {
+    // FIXME(kos): Why this is not a `Secret` as in other places?
     card_number: String,
     expiration_date: String,
     card_code: String,
@@ -134,6 +135,13 @@ impl TryFrom<&types::PaymentsAuthorizeRouterData> for CreateTransactionRequest {
     fn try_from(item: &types::PaymentsAuthorizeRouterData) -> Result<Self, Self::Error> {
         let payment_details = match item.request.payment_method_data {
             api::PaymentMethod::Card(ref ccard) => {
+                // FIXME(kos): The secrets here (and in similar places below)
+                // are cloned into the unprotected memory, and,
+                // thus, not properly zeroized on `Drop`.
+                // Furthermore, `.clone()` produces a redundant
+                // allocation here. It shouldn't be used, as
+                // the `format!()` macro bellow may easily accept a
+                // reference.
                 let expiry_month = ccard.card_exp_month.peek().clone();
                 let expiry_year = ccard.card_exp_year.peek().clone();
 
@@ -194,6 +202,20 @@ impl TryFrom<&types::PaymentsCancelRouterData> for CancelTransactionRequest {
 
 #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)]
 pub enum AuthorizedotnetPaymentStatus {
+    // FIXME(kos): Instead of such renaming, it would be more properly to use
+    // `#[repr(u8)]` here:
+    // ```rust
+    // #[derive(Serialize_repr, Deserialize_repr)]
+    // #[repr(u8)]
+    // pub enum AuthorizedotnetPaymentStatus {
+    //     Approved = 1,
+    //     Declined = 2,
+    //     Error = 3,
+    //     #[default]
+    //     HeldForReview = 4,
+    // }
+    // ```
+    // https://serde.rs/enum-number.html
     #[serde(rename = "1")]
     Approved,
     #[serde(rename = "2")]
@@ -287,6 +309,21 @@ impl<F, T>
             .transaction_response
             .errors
             .and_then(|errors| {
+                // FIXME(kos): Two redundant allocations here are introduced due
+                // to `.clone()` usage.
+                // We do own `errors` vector here, not borrow it, so
+                // we may directly consume its first element and its
+                // values without any cloning:
+                // ```rust
+                // errors
+                //     .into_iter()
+                //     .next()
+                //     .map(|err| types::ErrorResponse {
+                //         code: err.error_code,
+                //         message: err.error_text,
+                //         reason: None,
+                //     })
+                // ```
                 errors.first().map(|error| types::ErrorResponse {
                     code: error.error_code.clone(),
                     message: error.error_text.clone(),
diff --git a/crates/router/src/connector/checkout.rs b/crates/router/src/connector/checkout.rs
index 1e0cbd6df5a..74ad303dd9d 100644
--- a/crates/router/src/connector/checkout.rs
+++ b/crates/router/src/connector/checkout.rs
@@ -40,6 +40,19 @@ impl api::ConnectorCommon for Checkout {
         &self,
         auth_type: &types::ConnectorAuthType,
     ) -> CustomResult<Vec<(String, String)>, errors::ConnectorError> {
+        // FIXME(kos): Just every implementation of this `get_auth_header()`
+        // method in this codebase uses a statically predefined
+        // value for the first element of the returned tuple. So,
+        // why do redundant allocations with `.to_string()` then?
+        // Why not just return `&'static str` or, if we do really
+        // want to allow dynamic values, at least
+        // `Cow<'static, str>`?
+        // The same is valid for
+        // `ConnectorIntegration::get_headers()` (and similar ones)
+        // too.
+        // Or even use framework's header:
+        // https://docs.rs/http/0.2.8/http/header/struct.HeaderName.html
+
         let auth: checkout::CheckoutAuthType = auth_type
             .try_into()
             .change_context(errors::ConnectorError::FailedToObtainAuthType)?;
diff --git a/crates/router/src/core/admin.rs b/crates/router/src/core/admin.rs
index d14489ba692..02efe3a3600 100644
--- a/crates/router/src/core/admin.rs
+++ b/crates/router/src/core/admin.rs
@@ -17,7 +17,18 @@ use crate::{
 
 #[inline]
 fn create_merchant_api_key() -> String {
+    // FIXME(kos): Why duplicate `&Uuid::new_v4().simple().to_string()` line
+    // three times? Better DRY it out into a separate value and
+    // reuse:
+    // ```rust
+    // let id = Uuid::new_v4().simple();
+    // ```
     match env::which() {
+        // FIXME(kos): Using `.to_string()` here just does a redundant
+        // allocation without giving any benefit.
+        // Just use `Uuid::new_v4().simple()` directly, which
+        // implements `Display` anyway, so can be written directly
+        // into the formatter without intermediate allocations.
         Env::Development => format!("dev_{}", &Uuid::new_v4().simple().to_string()),
         Env::Production => format!("prd_{}", &Uuid::new_v4().simple().to_string()),
         Env::Sandbox => format!("snd_{}", &Uuid::new_v4().simple().to_string()),
@@ -59,6 +70,8 @@ pub async fn create_merchant_account(
         routing_algorithm: req.routing_algorithm.map(ForeignInto::foreign_into),
         custom_routing_rules: Some(custom_routing_rules),
         sub_merchants_enabled: req.sub_merchants_enabled,
+        // FIXME(kos) : possible race condition here. what if a concurrent user updates
+        // parent merchant between this operation and the following?
         parent_merchant_id: get_parent_merchant(
             db,
             &req.sub_merchants_enabled,
@@ -133,6 +146,8 @@ pub async fn merchant_account_update(
     merchant_id: &String,
     req: api::CreateMerchantAccount,
 ) -> RouterResponse<api::MerchantAccountResponse> {
+    // FIXME(kos) : possible race condition here. what if a concurrent user updates
+    // merchant account between this read and the last write?
     let merchant_account = db
         .find_merchant_account_by_merchant_id(merchant_id)
         .await
@@ -191,6 +206,8 @@ pub async fn merchant_account_update(
         sub_merchants_enabled: req
             .sub_merchants_enabled
             .or(merchant_account.sub_merchants_enabled),
+        // FIXME(kos) : possible race condition here. what if a concurrent user updates
+        // parent merchant between this operation and the following?
         parent_merchant_id: get_parent_merchant(
             db,
             &req.sub_merchants_enabled
diff --git a/crates/router/src/core/errors/api_error_response.rs b/crates/router/src/core/errors/api_error_response.rs
index 88dbb652cb7..0cc1c365194 100644
--- a/crates/router/src/core/errors/api_error_response.rs
+++ b/crates/router/src/core/errors/api_error_response.rs
@@ -128,6 +128,18 @@ pub enum ApiErrorResponse {
 
 impl ::core::fmt::Display for ApiErrorResponse {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // FIXME(kos): `serde_json::to_string(self)` here makes a redundant
+        // allocation, which may be omitted by writing into the
+        // formatter directly via `serde_json::to_writer()`:
+        // ```rust
+        // write!(f, r#"{{"error":"#)?;
+        // serde_json::to_writer(f, self)
+        //     .map(|_| fmt::Error::default())?;
+        // write!(f, "}")
+        // ```
+        // FIXME(kos): `.unwrap_or_else()` logic here is just incorrect.
+        // Better map the error and return it as in the example
+        // above.
         write!(
             f,
             r#"{{"error":{}}}"#,
@@ -140,6 +152,10 @@ impl actix_web::ResponseError for ApiErrorResponse {
     fn status_code(&self) -> reqwest::StatusCode {
         use reqwest::StatusCode;
 
+        // FIXME(kos): Use `Self::Unauthorized` syntax here (and in similar
+        // places), as more ergonomic.
+        // `#![warn(clippy::use_self)]` on crate level should help
+        // with this.
         match self {
             ApiErrorResponse::Unauthorized
             | ApiErrorResponse::BadCredentials
diff --git a/crates/router/src/core/payment_methods/cards.rs b/crates/router/src/core/payment_methods/cards.rs
index 862c075346f..4707e9d8ebd 100644
--- a/crates/router/src/core/payment_methods/cards.rs
+++ b/crates/router/src/core/payment_methods/cards.rs
@@ -665,9 +665,18 @@ pub async fn delete_payment_method(
             error.to_not_found_response(errors::ApiErrorResponse::PaymentMethodNotFound)
         })?;
 
+    // FIXME!(kos): What if the application process would be killed after
+    // successful execution of
+    // `delete_payment_method_by_merchant_id_payment_method_id()`,
+    // but before executing `delete_card()`?
+    // Would be the card ever deleted?
+    // Seems like the system has weak consistency guarantees.
+    // https://docs.diesel.rs/diesel/connection/trait.Connection.html#method.transaction
     if pm.payment_method == enums::PaymentMethodType::Card {
         let response = delete_card(state, &pm.merchant_id, &pm.payment_method_id).await?;
         if response.status == "success" {
+            // FIXME(kos): Why writing to STDOUT in blocking and unstructured
+            // manner? Use `log::info!()` instead.
             print!("Card From locker deleted Successfully")
         } else {
             print!("Error: Deleting Card From Locker")
diff --git a/crates/router/src/core/payments.rs b/crates/router/src/core/payments.rs
index 73d4077917d..9f4c385f8a0 100644
--- a/crates/router/src/core/payments.rs
+++ b/crates/router/src/core/payments.rs
@@ -457,6 +457,10 @@ where
     pub payment_intent: storage::PaymentIntent,
     pub payment_attempt: storage::PaymentAttempt,
     pub connector_response: storage::ConnectorResponse,
+    // FIXME(kos) : It is incorrect to use i32 as payment amount. Use decimal instead.
+    // first, there are situations where 2**31 would overflow.
+    // second, payment shouldn't have negative amounts.
+    // third, decimal place may be not shared.
     pub amount: api::Amount,
     pub currency: enums::Currency,
     pub mandate_id: Option<String>,
diff --git a/crates/router/src/core/refunds.rs b/crates/router/src/core/refunds.rs
index f61de78d2af..8444ac23a23 100644
--- a/crates/router/src/core/refunds.rs
+++ b/crates/router/src/core/refunds.rs
@@ -332,7 +332,10 @@ pub async fn refund_update_core(
 }
 
 // ********************************************** VALIDATIONS **********************************************
-
+// FIXME(kos) : this function doesn't follow ACID principle, since the validation
+// and the update is performed using multiple queries not in a single transaction.
+// For example, two concurrent requests can pass validation on refund amount,
+// and bypass maximum refund amount limitation. Thus, atomicity and consistency is broken.
 #[instrument(skip_all)]
 pub async fn validate_and_create_refund(
     state: &AppState,
@@ -366,6 +369,8 @@ pub async fn validate_and_create_refund(
         .attach_printable("invalid merchant_id in request")),
     )?;
 
+    // FIXME(kos) : There's a race condition here. it's possible for two clients to check that
+    // refund doesn't exist and try to create one at the same time.
     let refund = match validator::validate_uniqueness_of_refund_id_against_merchant_id(
         db,
         &payment_intent.payment_id,
diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs
index de83d4e94ff..95ffd26c984 100644
--- a/crates/router/src/lib.rs
+++ b/crates/router/src/lib.rs
@@ -1,6 +1,20 @@
+// FIXME(kos): Here and in any other crate roots it's better to declare
+// `#![forbid(unsafe_code)]` as we do in `batch_pii` crate.
+// This will ease a life of readers and auditors a lot, and will
+// require quite a reasoning for those who will intend to contribute
+// any `unsafe` code.
+
+// TODO(kos): Consider always adding `#![forbid(non_ascii_idents)]` to crate
+// roots, unless there is a real need in some exotic or math notation
+// in the code. This prevents any fuckups with accidental unicode
+// characters similarity.
+
 // FIXME: I strongly advise to add this worning.
 // #![warn(missing_docs)]
 
+// FIXME(kos): Codebase has many such problems. Use it.
+// #![warn(clippy::use_self)]
+
 // FIXME: I recommend to add these wornings too, although there is no harm if these wanrings will stay disabled.
 // #![warn(rust_2018_idioms)]
 // #![warn(missing_debug_implementations)]
diff --git a/crates/router/src/scheduler/consumer.rs b/crates/router/src/scheduler/consumer.rs
index 92cf5723569..abb954b4c01 100644
--- a/crates/router/src/scheduler/consumer.rs
+++ b/crates/router/src/scheduler/consumer.rs
@@ -85,6 +85,8 @@ pub async fn consumer_operations(
     settings: &settings::SchedulerSettings,
 ) -> CustomResult<(), errors::ProcessTrackerError> {
     let stream_name = settings.stream.clone();
+    // FIXME(kos): Why `String` when `consumer_group_create()` accepts
+    // `&str`?
     let group_name = settings.consumer_group.clone();
     let consumer_name = format!("consumer_{}", Uuid::new_v4());
 
diff --git a/crates/router/src/scheduler/producer.rs b/crates/router/src/scheduler/producer.rs
index a190cf9104e..bcdef9da2e5 100644
--- a/crates/router/src/scheduler/producer.rs
+++ b/crates/router/src/scheduler/producer.rs
@@ -49,7 +49,16 @@ pub async fn start_producer(
         options.looper_interval.milliseconds,
     ));
 
+    // TODO(kos): The loop is not panic safe. It won't be automatically restored
+    // if a panic happens. So the external machinery should take care
+    // to restart the process if this happens.
+    // Either catch panic with catch_unwind or restart process externally.
+    // https://docs.rs/futures/0.3.25/futures/future/trait.FutureExt.html#method.catch_unwind
     loop {
+        // FIXME(kos) : If the scheduler is going to sleep for some time,
+        // then process events, the time between start of event processing
+        // will be off by the time taken to process events.
+        // Is it necessary to have precise interval between event processing?
         interval.tick().await;
 
         let is_ready = options.readiness.is_ready;
diff --git a/crates/router/src/scheduler/types/batch.rs b/crates/router/src/scheduler/types/batch.rs
index bd81d093a95..15db3de5004 100644
--- a/crates/router/src/scheduler/types/batch.rs
+++ b/crates/router/src/scheduler/types/batch.rs
@@ -49,6 +49,19 @@ impl ProcessTrackerBatch {
     pub fn from_redis_stream_entry(
         entry: HashMap<String, Option<String>>,
     ) -> CustomResult<Self, errors::ProcessTrackerError> {
+        // FIXME(kos) : the MissingRequiredField could include the missing field name.
+        //   .change_context(errors::ProcessTrackerError::MissingRequiredField("id"))?;
+        // FIXME(kos) : the code is quite repetitive, is there a way to make it shorter, like with a macro?
+        // macro_rules! get_field {
+        //     ($entry:expr, $field:expr) => {
+        //         $entry
+        //             .get($field)
+        //             .get_required_value($field)
+        //             .change_context(errors::ProcessTrackerError::MissingRequiredField($field))?
+        //     }
+        // }
+        // let id = get_field!(entry, "id");
+
         let mut entry = entry;
         let id = entry
             .remove("id")
diff --git a/crates/router/src/services/api.rs b/crates/router/src/services/api.rs
index f69149b62b7..9721304ca7e 100644
--- a/crates/router/src/services/api.rs
+++ b/crates/router/src/services/api.rs
@@ -362,6 +362,10 @@ pub enum ApiAuthentication<'a> {
     Connector(ConnectorAuthentication<'a>),
 }
 
+// FIXME(kos) : Is it correct to call this MerchantAuthentication, considering
+// there's AdminApiKey as a variant?
+// The name implies that authentication is only performed for Merchants,
+// however, AdminApiKey implies it's an admin user.
 #[derive(Clone, Debug)]
 pub enum MerchantAuthentication<'a> {
     ApiKey,
@@ -536,6 +540,10 @@ pub async fn authenticate_merchant<'a>(
         }
 
         MerchantAuthentication::AdminApiKey => {
+            // FIXME(kos) : It looks like admin credentials are hard-coded below.
+            // It is a securiry problem to store credentials in the code.
+            // e.g. if request.password != "hunter2" { return Err(...) }
+
             let admin_api_key =
                 get_api_key(request).change_context(errors::ApiErrorResponse::Unauthorized)?;
             if admin_api_key != "test_admin" {
diff --git a/crates/router/src/services/api/request.rs b/crates/router/src/services/api/request.rs
index 04cb0413c42..88c3956fd4b 100644
--- a/crates/router/src/services/api/request.rs
+++ b/crates/router/src/services/api/request.rs
@@ -10,6 +10,7 @@ use crate::{
     logger,
 };
 
+// FIXME(kos) : Why not use reqwest::RequestBuilder + make an extension?
 pub(crate) type Headers = Vec<(String, String)>;
 
 #[derive(
diff --git a/crates/router/src/types.rs b/crates/router/src/types.rs
index 58a029b1350..5eab93a9634 100644
--- a/crates/router/src/types.rs
+++ b/crates/router/src/types.rs
@@ -253,6 +253,7 @@ pub struct ResponseRouterData<Flow, R, Request, Response> {
 // Different patterns of authentication.
 #[derive(Debug, Clone, serde::Deserialize)]
 #[serde(tag = "auth_type")]
+// FIXME(kos): Shouldn't these keys be `Secret`?
 pub enum ConnectorAuthType {
     HeaderKey {
         api_key: String,
diff --git a/crates/router/src/utils.rs b/crates/router/src/utils.rs
index ef0b24a69ec..ac742d4ecab 100644
--- a/crates/router/src/utils.rs
+++ b/crates/router/src/utils.rs
@@ -34,6 +34,14 @@ pub mod error_parser {
     // Display is a requirement defined by the actix crate for implementing ResponseError trait
     impl Display for CustomJsonError {
         fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            // FIXME(kos): This code contains redundant heap allocation `.to_string()`
+            // allocations, because we can write directly into the
+            // formatter:
+            // ```rust
+            // write!(f, "{}", serde_json::json!({
+            //     "error": &self.err
+            // }))
+            // ```
             f.write_str(
                 serde_json::to_string(&serde_json::json!({
                     "error": self.err.to_string()
diff --git a/crates/router_derive/src/lib.rs b/crates/router_derive/src/lib.rs
index 8e6240d3487..872da0fdbec 100644
--- a/crates/router_derive/src/lib.rs
+++ b/crates/router_derive/src/lib.rs
@@ -158,6 +158,7 @@ pub fn setter(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
         let method_ident = syn::Ident::new(&method_name, name.span());
         let ty = &f.ty;
         if check_if_auth_based_attr_is_present(f, "auth_based") {
+            // FIXME(kos) : `is_merchant_flow` seems to be too use-case specific, maybe `is_authenticated`?
             quote::quote! {
                 pub fn #method_ident(&mut self, val:#ty, is_merchant_flow: bool)->&mut Self{
                     if is_merchant_flow {
diff --git a/crates/router_derive/src/macros.rs b/crates/router_derive/src/macros.rs
index 8ef88d13c65..5a5a466a7f7 100644
--- a/crates/router_derive/src/macros.rs
+++ b/crates/router_derive/src/macros.rs
@@ -19,6 +19,14 @@ pub(crate) fn debug_as_display_inner(ast: &DeriveInput) -> syn::Result<TokenStre
 
     Ok(quote! {
         impl #impl_generics ::core::fmt::Display for #name #ty_generics #where_clause {
+                // FIXME(kos): Redundant allocation on heap.
+                // This code produces the redundant `String`
+                // allocation as the result of `format!()` macro
+                // call. Omit it by writing into the formatter
+                // directly with `write!()` macro:
+                // ```rust
+                // ::core::write!(f, "{:?}", self)
+                // ```
             fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::result::Result<(), ::core::fmt::Error> {
                 f.write_str(&format!("{:?}", self))
             }
diff --git a/crates/router_derive/src/macros/api_error.rs b/crates/router_derive/src/macros/api_error.rs
index cdf14bbfdcc..61ed3661dfa 100644
--- a/crates/router_derive/src/macros/api_error.rs
+++ b/crates/router_derive/src/macros/api_error.rs
@@ -173,6 +173,8 @@ fn implement_serialize(
 
         let error_type_enum = type_properties.error_type_enum.as_ref().unwrap();
         let response_definition = if msg_unused_fields.is_empty() {
+            // FIXME(kos): Redundant structure.
+            // It's possible to drop it and get better peformance and simpler code.
             quote! {
                 #[derive(Clone, Debug, serde::Serialize)]
                 struct ErrorResponse {
diff --git a/crates/router_derive/src/macros/api_error/helpers.rs b/crates/router_derive/src/macros/api_error/helpers.rs
index b2997090785..14f85c78b2c 100644
--- a/crates/router_derive/src/macros/api_error/helpers.rs
+++ b/crates/router_derive/src/macros/api_error/helpers.rs
@@ -222,6 +222,12 @@ pub(super) fn check_missing_attributes(
 /// Get all the fields not used in the error message.
 pub(super) fn get_unused_fields(fields: &Fields, message: &str) -> syn::Result<Vec<Field>> {
     let fields = match fields {
+        // FIXME(kos): Restructure it as:
+        // ```rust
+        // Fields::Unit | Fields::Unnamed(_) => vec![],
+        // ```
+        // `#![warn(clippy::match_same_arms)]` on crate level should
+        // help with this.
         syn::Fields::Unit => Vec::new(),
         syn::Fields::Unnamed(_) => Vec::new(),
         syn::Fields::Named(fields) => fields.named.iter().cloned().collect(),
diff --git a/crates/router_derive/src/macros/diesel.rs b/crates/router_derive/src/macros/diesel.rs
index 932d45b7f4d..a7a4b4c9cc2 100644
--- a/crates/router_derive/src/macros/diesel.rs
+++ b/crates/router_derive/src/macros/diesel.rs
@@ -20,6 +20,11 @@ pub(crate) fn diesel_enum_derive_inner(ast: &DeriveInput) -> syn::Result<TokenSt
         #[diesel(postgres_type(name = #type_name))]
         pub struct #struct_name;
 
+        // FIXME(kos): Consider to mark any new code/items, generated by
+        // procedural macros, with the `#[automatically_derived]`
+        // attribute. This makes code style linters to omit the
+        // generated code and doesn't report redundant warnings.
+        // Example: https://github.com/cucumber-rs/cucumber/blob/v0.15.1/codegen/src/world.rs
         impl #impl_generics ::diesel::serialize::ToSql<#struct_name, ::diesel::pg::Pg> for #name #ty_generics
         #where_clause
         {
diff --git a/crates/router_derive/src/macros/operation.rs b/crates/router_derive/src/macros/operation.rs
index e234635aabc..89c9454e984 100644
--- a/crates/router_derive/src/macros/operation.rs
+++ b/crates/router_derive/src/macros/operation.rs
@@ -89,6 +89,10 @@ impl From<String> for Conversion {
             "update_tracker" => Self::UpdateTracker,
             "post_tracker" => Self::PostUpdateTracker,
             "all" => Self::All,
+            // FIXME(kos) : proc macro should not panic.  When proc macro panics,
+            // it will be the only error message returned by the rust compiler.
+            // Instead, in case of errors, proc macro should emit compile_error!()
+            // to communicate error to the user. This will allow other errors to be displayed.
             _ => panic!("Invalid conversion identifier {}", s),
         }
     }
@@ -157,6 +161,7 @@ impl Conversion {
         }
     }
 
+    // FIXME(kos) : why is it necessary to have a separate impl for this
     fn to_ref_function(&self, ident: Derives) -> TokenStream {
         let req_type = Self::get_req_type(ident);
         match self {
diff --git a/crates/router_env/src/logger/config.rs b/crates/router_env/src/logger/config.rs
index 114fad09335..b95533b4385 100644
--- a/crates/router_env/src/logger/config.rs
+++ b/crates/router_env/src/logger/config.rs
@@ -48,6 +48,25 @@ pub struct LogFile {
     // pub rotation: u16,
 }
 
+// FIXME(kos): It could be done in a better way. As far as
+// `serde` provides `#[serde(deserialize_with = ...)]` attribute:
+// ```rust
+// #[derive(Debug, Deserialize, Clone)]
+// pub struct LogFile {
+//     // ...
+//     /// What gets into log files.
+//     `#[serde(deserialize_with = "deserialize_level")]`
+//     pub level: tracing::Level,
+//     // ...
+// }
+// fn deserialize_level<'de, D: Deserializer<'de>>(
+//     d: D,
+// ) -> Result<tracing::Level, D::Error> {
+//     tracing::Level::from_str(&*<Cow<'_, str>>::deserialize(d)?)
+//         .map_err(D::Error::custom)
+// }
+// ```
+// See more: https://serde.rs/field-attrs.html#deserialize_with
 /// Describes the level of verbosity of a span or event.
 #[derive(Debug, Clone)]
 pub struct Level(tracing::Level);
@@ -96,6 +115,9 @@ pub struct LogTelemetry {
 #[derive(Default, Debug, Deserialize, Clone, PartialEq, Eq)]
 #[serde(rename_all = "lowercase")]
 pub enum LogFormat {
+    // FIXME(kos): `Default` is quite a generic naming here, giving no idea about
+    // what the format is. Better name it either `Pretty`, or
+    // `Compact` (the terminology used by `slog` crate).
     /// Default pretty log format
     Default,
     /// JSON based structured logging
diff --git a/crates/router_env/src/logger/formatter.rs b/crates/router_env/src/logger/formatter.rs
index 11d9abc66bc..1ceaf79d4c1 100644
--- a/crates/router_env/src/logger/formatter.rs
+++ b/crates/router_env/src/logger/formatter.rs
@@ -111,6 +111,9 @@ pub enum RecordType {
 impl fmt::Display for RecordType {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let repr = match self {
+            // FIXME(kos): Use `Self::EnterSpan` syntax here.
+            // `#![warn(clippy::use_self)]` on crate level should
+            // help with this.
             RecordType::EnterSpan => "START",
             RecordType::ExitSpan => "END",
             RecordType::Event => "EVENT",
@@ -204,6 +207,10 @@ where
         map_serializer.serialize_entry(ENV, &self.env)?;
         map_serializer.serialize_entry(VERSION, &self.version)?;
         map_serializer.serialize_entry(BUILD, &self.build)?;
+        // FIXME(kos): Use here, below, and in similar places, the
+        // `format_args!()` macro instead, to omit redundant
+        // allocations produced by the `format!()` macro (as it
+        // returns a new `String`).
         map_serializer.serialize_entry(LEVEL, &format!("{}", metadata.level()))?;
         map_serializer.serialize_entry(TARGET, metadata.target())?;
         map_serializer.serialize_entry(SERVICE, &self.service)?;
diff --git a/crates/router_env/src/logger/macros.rs b/crates/router_env/src/logger/macros.rs
index c3398e98c64..f0443db33df 100644
--- a/crates/router_env/src/logger/macros.rs
+++ b/crates/router_env/src/logger/macros.rs
@@ -1,3 +1,5 @@
+// FIXME(kos): We can remove the file now.
+
 //!
 //! Macros.
 //!
diff --git a/crates/storage_models/src/refund.rs b/crates/storage_models/src/refund.rs
index adf189665a4..b4bf6f8ba6b 100644
--- a/crates/storage_models/src/refund.rs
+++ b/crates/storage_models/src/refund.rs
@@ -17,8 +17,12 @@ pub struct Refund {
     pub pg_refund_id: Option<String>,
     pub external_reference_id: Option<String>,
     pub refund_type: storage_enums::RefundType,
+    // FIXME(kos): It is incorrect to use i32 as payment amount. Use decimal instead.
+    // same as PaymentData::amount
     pub total_amount: i32,
     pub currency: storage_enums::Currency,
+    // FIXME(kos): It is incorrect to use i32 as payment amount. Use decimal instead.
+    // same as PaymentData::amount
     pub refund_amount: i32,
     pub refund_status: storage_enums::RefundStatus,
     pub sent_to_gateway: bool,

Thorough code review