feat(payouts): implement KVRouterStore #3889

kashif-m · 2024-02-29T08:50:27Z

Type of Change

Description

This PR includes below changes

implement KVRouterStore for storing payout data in either redis + postgres or postgres only
moving required columns for dashboard analytics to a single table
usage and consumption of payouts feature throughout crates

Explained in issue - #3652

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

By verifying existing payout functionalities are working as expected using KV

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed the submitted code
I added unit tests for my changes where possible
I added a CHANGELOG entry if applicable

refactor(payouts): update payouts feature flag dependency and consumption throughout crates

crates/diesel_models/src/query/payout_attempt.rs

crates/router/src/core/payment_methods/cards.rs

sahkal · 2024-03-01T09:32:18Z

crates/router/src/core/payouts/validator.rs

 pub async fn validate_uniqueness_of_payout_id_against_merchant_id(
    db: &dyn StorageInterface,
    payout_id: &str,
    merchant_id: &str,
+    storage_scheme: storage::enums::MerchantStorageScheme,
 ) -> RouterResult<Option<storage::Payouts>> {
    let payout = db
-        .find_payout_by_merchant_id_payout_id(merchant_id, payout_id)
+        .find_payout_by_merchant_id_payout_id(merchant_id, payout_id, storage_scheme)
        .await;
    match payout {
        Err(err) => {
-            if err.current_context().is_db_not_found() {
-                // Empty vec should be returned by query in case of no results, this check exists just
-                // to be on the safer side. Fixed this, now vector is not returned but should check the flow in detail later.
-                Ok(None)
-            } else {
-                Err(err
+            let erc = err.current_context();
+            match erc {
+                StorageError::ValueNotFound(_) => Ok(None),
+                _ => Err(err
                    .change_context(errors::ApiErrorResponse::InternalServerError)
-                    .attach_printable("Failed while finding payout_attempt, database error"))
+                    .attach_printable("Failed while finding payout_attempt, database error")),
            }
        }


I don't think this is the correct way of implementing this function, can be confusing for some instance

Re-looking at this function, it looks good to me.

It checks whether or not the resource exists in DB, and returns an optional result

If Err is returned, it was due to external DB issues.
If Ok(None) is returned, DB call was successful but payout was not found
If Ok(Some) is returned, DB call was successful and payout with given ID already exists.

Please have a look at fn generic_find_by_id_optional

you can have an function named something like get_optional_payout_by_merchant_id_payout_id in PayoutInterface which would be calling generic_find_by_id_optional

sahkal · 2024-03-01T09:34:23Z

crates/router/src/db.rs

+    + refund::RefundInterface
+    + reverse_lookup::ReverseLookupInterface
+    + cards_info::CardsInfoInterface
+    + merchant_key_store::MerchantKeyStoreInterface
+    + MasterKeyInterface
+    + payment_link::PaymentLinkInterface
+    + RedisConnInterface
+    + RequestIdStore
+    + business_profile::BusinessProfileInterface
+    + OrganizationInterface
+    + routing_algorithm::RoutingAlgorithmInterface
+    + gsm::GsmInterface
+    + user::UserInterface
+    + user_role::UserRoleInterface
+    + authorization::AuthorizationInterface
+    + user::sample_data::BatchSampleDataInterface
+    + health_check::HealthCheckDbInterface
+    + role::RoleInterface
+    + 'static
+{
+    fn get_scheduler_db(&self) -> Box<dyn scheduler::SchedulerInterface>;
+}
+
+#[cfg(feature = "payouts")]
+#[async_trait::async_trait]
+pub trait StorageInterface:
+    Send
+    + Sync
+    + dyn_clone::DynClone
+    + address::AddressInterface
+    + api_keys::ApiKeyInterface
+    + blocklist_lookup::BlocklistLookupInterface
+    + configs::ConfigInterface
+    + capture::CaptureInterface
+    + customers::CustomerInterface
+    + dashboard_metadata::DashboardMetadataInterface
+    + dispute::DisputeInterface
+    + ephemeral_key::EphemeralKeyInterface
+    + events::EventInterface
+    + file::FileMetadataInterface
+    + FraudCheckInterface
+    + locker_mock_up::LockerMockUpInterface
+    + mandate::MandateInterface
+    + merchant_account::MerchantAccountInterface
+    + merchant_connector_account::ConnectorAccessToken
+    + merchant_connector_account::MerchantConnectorAccountInterface
+    + PaymentAttemptInterface
+    + PaymentIntentInterface
+    + payment_method::PaymentMethodInterface
+    + blocklist::BlocklistInterface
+    + blocklist_fingerprint::BlocklistFingerprintInterface


@jarnura and @SanchithHegde can we verify this change, its being duplicated for only payouts interface

I have added an empty PayoutsInterface and PayoutAttemptInterface for instances where payouts feature is not enabled. Similar to how it's done for pub trait Connector:

sahkal · 2024-03-01T09:35:45Z

crates/router/src/db/kafka_store.rs

        self.diesel_store
-            .update_payout_by_merchant_id_payout_id(merchant_id, payout_id, payout)
+            .update_payout(this, payout_update, storage_scheme)
            .await
    }



can u pass instrument on every db function

sahkal · 2024-03-01T09:40:18Z

crates/storage_impl/src/lib.rs

 }
+
+#[cfg(feature = "payouts")]
+impl UniqueConstraints for diesel_models::Payouts {
+    fn unique_constraints(&self) -> Vec<String> {
+        vec![format!("po_{}_{}", self.merchant_id, self.payout_id)]
+    }
+    fn table_name(&self) -> &str {
+        "Payouts"
+    }
+}
+
+#[cfg(feature = "payouts")]
+impl UniqueConstraints for diesel_models::PayoutAttempt {
+    fn unique_constraints(&self) -> Vec<String> {
+        vec![format!(
+            "poa_{}_{}",
+            self.merchant_id, self.payout_attempt_id


@dracarys18 can check this

sahkal · 2024-03-01T09:44:31Z

crates/storage_impl/src/payouts/payout_attempt.rs

+            }
+        }
+    }
+


can you add intruments in such funcs

sahkal

Ci checks are failing along with merge conflicts can you fix all

kashif-m · 2024-03-18T06:52:11Z

crates/router/src/types/storage/payment_method.rs

@@ -76,6 +77,19 @@ impl PaymentTokenData {
    pub fn wallet_token(payment_method_id: String) -> Self {
        Self::WalletToken(WalletTokenData { payment_method_id })
    }
+
+    pub fn get_token(&self) -> &String {


@Chethan-rao - can you review this?

In case of cards, get_token function doesn't really does what is says. Because cards have an object data as opposed to bank_transfer which has only one token field. I feel its better to remove this and generate token for bank transfer by explicitly matching on it

kashif-m · 2024-03-18T06:52:39Z

crates/router/src/core/payment_methods/cards.rs

                None,
                PaymentTokenData::temporary_generic(generate_id(consts::ID_LENGTH, "token")),
            ),
        };

+        #[cfg(feature = "payouts")]


@Chethan-rao It's consumed here

crates/storage_impl/src/payouts/payout_attempt.rs

ThisIsMani

Dashboard specific changes looks good.

kashif-m force-pushed the payout_kv_interface branch from f9d186b to d5494ad Compare February 29, 2024 08:53

kashif-m self-assigned this Feb 29, 2024

kashif-m linked an issue Feb 29, 2024 that may be closed by this pull request

[FEATURE] Analytics for payouts #3652

Closed

2 tasks

feat(payouts): implement KVRouterStore

8e81bed

refactor(payouts): update payouts feature flag dependency and consumption throughout crates

kashif-m force-pushed the payout_kv_interface branch from d4755d3 to 8e81bed Compare February 29, 2024 09:03

kashif-m marked this pull request as ready for review February 29, 2024 09:13

kashif-m requested review from a team as code owners February 29, 2024 09:13

kashif-m force-pushed the payout_kv_interface branch from d6295ea to 8e81bed Compare February 29, 2024 09:14

hyperswitch-bot bot and others added 3 commits February 29, 2024 09:16

docs(openapi): re-generate OpenAPI specification

494c2e4

Merge remote-tracking branch 'origin/main' into payout_kv_interface

9c00637

fix(payouts): missing import

0814880

sahkal added the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Mar 1, 2024

sahkal added this to the February 2024 Release milestone Mar 1, 2024

sahkal added A-routing Area: Routing A-core Area: Core flows labels Mar 1, 2024

sahkal requested changes Mar 1, 2024

View reviewed changes

kashif-m added 2 commits March 5, 2024 11:33

Merge remote-tracking branch 'origin/main' into payout_kv_interface

6f79fc1

refactor(payouts): resolve comments

86da711

kashif-m requested a review from sahkal March 5, 2024 06:44

sahkal requested changes Mar 5, 2024

View reviewed changes

kashif-m added 2 commits March 6, 2024 11:06

Merge remote-tracking branch 'origin/main' into payout_kv_interface

7ff65e5

chore: clippy fixes

f7ba3d4

kashif-m requested a review from sahkal March 6, 2024 05:51

kashif-m added 2 commits March 6, 2024 11:52

refactor: add default implementation for payouts StorageInterface

a0814a1

chore: cargo hack fixes

f911788

SanchithHegde removed this from the February 2024 Release milestone Mar 6, 2024

feat(payouts): add optional find query for payouts table

af6a261

kashif-m force-pushed the payout_kv_interface branch from ce878c1 to af6a261 Compare March 18, 2024 06:28

Merge remote-tracking branch 'origin/main' into payout_kv_interface

a9a6792

kashif-m requested a review from hrithikesh026 March 18, 2024 06:40

refactor: get_token function

9c2b35c

kashif-m commented Mar 18, 2024

View reviewed changes

hrithikesh026 previously approved these changes Mar 18, 2024

View reviewed changes

kashif-m requested a review from sahkal March 18, 2024 07:44

sahkal previously approved these changes Mar 18, 2024

View reviewed changes

refactor: remove get_token

33ecc69

kashif-m dismissed stale reviews from sahkal and hrithikesh026 via 33ecc69 March 18, 2024 08:13

kashif-m and others added 2 commits March 18, 2024 14:05

refactor: specify type annotations

4e44564

chore: run formatter

ac5da4b

akshay-97 reviewed Mar 18, 2024

View reviewed changes

crates/storage_impl/src/payouts/payout_attempt.rs Show resolved Hide resolved

refactor: payment methods list and payout_attempt KV lookup consumption

73d0d29

kashif-m force-pushed the payout_kv_interface branch from e4b6d58 to 73d0d29 Compare March 18, 2024 17:36

Merge remote-tracking branch 'origin/main' into payout_kv_interface

dec9b67

akshay-97 approved these changes Mar 19, 2024

View reviewed changes

Chethan-rao approved these changes Mar 19, 2024

View reviewed changes

hrithikesh026 approved these changes Mar 19, 2024

View reviewed changes

ThisIsMani approved these changes Mar 19, 2024

View reviewed changes

sahkal approved these changes Mar 19, 2024

View reviewed changes

srujanchikke approved these changes Mar 19, 2024

View reviewed changes

Gnanasundari24 added this pull request to the merge queue Mar 19, 2024

Merged via the queue into main with commit 944089d Mar 19, 2024
13 of 15 checks passed

Gnanasundari24 deleted the payout_kv_interface branch March 19, 2024 10:18

pixincreate removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(payouts): implement KVRouterStore #3889

feat(payouts): implement KVRouterStore #3889

kashif-m commented Feb 29, 2024 •

edited

Loading

sahkal Mar 1, 2024

kashif-m Mar 5, 2024

hrithikesh026 Mar 15, 2024

hrithikesh026 Mar 15, 2024

sahkal Mar 1, 2024

kashif-m Mar 5, 2024

sahkal Mar 1, 2024

sahkal Mar 1, 2024

sahkal Mar 1, 2024

sahkal left a comment

kashif-m Mar 18, 2024

Chethan-rao Mar 18, 2024

kashif-m Mar 18, 2024

ThisIsMani left a comment

feat(payouts): implement KVRouterStore #3889

feat(payouts): implement KVRouterStore #3889

Conversation

kashif-m commented Feb 29, 2024 • edited Loading

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahkal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMani left a comment

Choose a reason for hiding this comment

kashif-m commented Feb 29, 2024 •

edited

Loading