Implement Batch Payments Feature #155

eshanvaid · 2023-06-14T20:43:53Z

This PR introduces the batch payment feature to our CLI. This feature allows the user to execute multiple payments simultaneously using a CSV file containing the details of the recipients.

The command syntax is as follows:
batch --csv <file-path>

The CSV file needs to adhere to this format: the columns should be - username, {cents or sats}, memo(optional).

Example:

username	cents	memo
test_user_a	3	happy easter
test_user_b	8

The implementation consists of four key steps, which are carried out by their corresponding functions:

check_file_exists - This function checks if the input file exists.
validate_csv - This function validates the CSV file. It checks whether the headers and rows are in the correct format, and whether the usernames in the rows are valid.
check_sufficient_balance - This function checks if the total amount specified in the CSV file is less than or equal to the user's current wallet balance.
execute_batch_payment - This function performs the payment transactions. The mode of payment (cents or sats) is determined based on the corresponding header in the CSV file.

Related issue: #148

As this is a significant feature addition, thorough testing has been performed to ensure its correctness and reliability.

eshanvaid · 2023-06-14T20:48:10Z

Note that this branch was created from add-balance-command. Since that branch has not been merged into the master yet, all the commits from add-balance-command are currently visible in this PR as well. Once PR #152 is merged, I will rebase this branch with the latest master which will then show only the changes relevant to this feature

nicolasburtey · 2023-06-18T08:03:59Z

src/client/batch.rs

+    let mut reader = csv::ReaderBuilder::new().delimiter(b',').from_path(file)?;
+    let headers = reader.headers()?.clone();
+
+    if &headers[0] != "username"


maybe we could make this feature proof and go with lightning address? or maybe lightning address has not been implemented yet from the cli?

We currently haven't implemented Lightning Address in the CLI, but it's on our to-do list. Once we've got that (probably within this week), we'll tweak the batch payments to work with it.

nicolasburtey · 2023-06-18T08:05:06Z

src/client/batch.rs

-    pub fn is_empty(&self) -> bool {
-        self.payments.is_empty()
-    }
+    let wallet_type = if headers.get(1) == Some("cents") {


are we assuming that it we sent sats, it's from the btc wallet, and if we send cents, it's from the usd wallet?

I don't think we should make this assumption. we can send "100 dollar worth of bitcoin" from the bitcoin wallet.

I think typically people right not send "cents" or "cents equivalent", so we could start with this one.

Batch payments is built upon intraledger payments, which does not support sending cents from a Bitcoin wallet or sats from a USD wallet. Due to this, our current implementation assumes that if the amount is in cents, it is sent from the USD wallet and vice versa. Once we incorporate the capability for conversions in intraledger payments, we will extend the batch payments feature to allow this.

nicolasburtey · 2023-06-18T08:06:16Z

src/client/batch.rs


-        Ok(())
+pub fn check_sufficient_balance(


we should also check the sending limits.

nicolasburtey · 2023-06-18T08:07:19Z

tests/batch.rs

@@ -1,92 +0,0 @@
-use galoy_cli::batch::Batch;


why is the tests deleted?

the tests you're referring to were associated with the previous iteration of batch payments, which are no longer relevant.
Once this PR is approved and the implementation is finalized, I'll be introducing new e2e tests for batch in a subsequent PR

Once this PR is approved and the implementation is finalized, I'll be introducing new e2e tests for batch in a subsequent PR

imo, unless exception for good reasons, tests should come with PRs, not after. otherwise it's too easy to move on to other things and never do the tests.

bodymindarts · 2023-06-18T18:39:31Z

src/client/batch.rs

-            client,
-            price,
-        }
+// Utility function to read and validate the CSV file


This comment is redundant as it adds no additional information than the name of the function. Delete it - or alternatively if you want to publish this as a library turn in into a doc style comment (///) and add something useful.

bodymindarts · 2023-06-18T18:40:10Z

src/client/batch.rs

+    file: &str,
+) -> anyhow::Result<(Vec<csv::StringRecord>, Wallet)> {
+    let mut reader = csv::ReaderBuilder::new().delimiter(b',').from_path(file)?;
+    let headers = reader.headers()?.clone();


bodymindarts · 2023-06-18T18:40:49Z

src/client/batch.rs

+    let mut reader = csv::ReaderBuilder::new().delimiter(b',').from_path(file)?;
+    let headers = reader.headers()?.clone();
+
+    if &headers[0] != "username"


Should not be using magic values... all hard coded values must be declared as constants.

bodymindarts · 2023-06-18T18:41:25Z

src/client/batch.rs

-    pub fn is_empty(&self) -> bool {
-        self.payments.is_empty()
-    }
+    let wallet_type = if headers.get(1) == Some("cents") {


This should be handled via FromStr / parse

bodymindarts · 2023-06-18T18:42:38Z

src/client/batch.rs

-            let payment_btc: Decimal = payment.usd / self.price;
-            payment.sats = Some(payment_btc * dec!(100_000_000));
-        }
+        let amount = record.get(1).ok_or(anyhow::anyhow!("Amount is missing"))?;


I find the syntax:

record.get(1).context("Amount is missing")?;

more concise.
Need to use anyhow::Context for it to work.

bodymindarts · 2023-06-18T18:44:34Z

src/client/batch.rs

-                "payment to {username} of sats {amount}, usd {usd}: {:?}",
-                res
-            );
+        match wallet_type {


How are you dealing with partial failures? What if the first 5 out of 10 payments complete but then there is an error - how is this reported to avoid a double payment on re-try?

bodymindarts · 2023-06-18T18:45:11Z

src/client/mod.rs


 pub mod server;

+impl From<&WalletCurrency> for Wallet {


I like putting From implementations into their own file eg src/client/convert.rs.

bodymindarts · 2023-06-18T18:47:20Z

src/client/mod.rs

-        batch.execute().context("can't make payment successfully")?;
-
-        Ok(())
+    pub fn batch_payment(self, file: String) -> anyhow::Result<String> {


Having batch as a submodule of client is IMO significantly bloating its responsibility. Typically a client is a thin wrapper around some network calls. More complex coordination that requires multiple round trips needs to be put in a use-case / app layer that uses the client as a dependency.

bodymindarts · 2023-06-18T18:50:48Z

src/main.rs

@@ -15,6 +15,8 @@ use std::fs::{self};
 mod constants;
 mod token;

+use galoy_cli::types::*;


main.rs is completely bloated. It should just call into something that gets exposed via lib - which is where the use cases should be defined.

IMO this project is getting large enough that a purely design - improvement / refactoring iteration should be executed to improve things before implementing further complex use cases.

At this stage it should still be fairly simple to clean things up - later that might not be the case anymore.

bodymindarts · 2023-06-18T18:52:02Z

src/types.rs

@@ -0,0 +1,16 @@
+use rust_decimal::Decimal;


Having a types.rs file is a code smell that indicates a lack of domain oriented design-thinking.

eshanvaid added 7 commits June 11, 2023 01:52

refactor: add types to library crate

6db69e0

feat(balance): add balance command

c4a575d

feat: allow input of wallet-ids

4a65bd3

refactor: implement From for Wallet conversion

3292bb9

refactor: rename wallet to wallet_type

626873f

refactor(fetch_balance): return type to json

b9a2227

feat: implement batch payments

141c8dc

eshanvaid added the enhancement New feature or request label Jun 14, 2023

eshanvaid requested review from nicolasburtey and sandipndev June 14, 2023 20:43

eshanvaid added 2 commits June 15, 2023 02:24

fix: code check test

d06e3af

refactor: remove previous batch tests

978d13c

nicolasburtey reviewed Jun 18, 2023

View reviewed changes

bodymindarts reviewed Jun 18, 2023

View reviewed changes

sandipndev closed this Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Batch Payments Feature #155

Implement Batch Payments Feature #155

eshanvaid commented Jun 14, 2023

eshanvaid commented Jun 14, 2023 •

edited

Loading

nicolasburtey Jun 18, 2023

eshanvaid Jun 18, 2023

nicolasburtey Jun 18, 2023

nicolasburtey Jun 18, 2023

eshanvaid Jun 18, 2023

nicolasburtey Jun 18, 2023

nicolasburtey Jun 18, 2023

eshanvaid Jun 18, 2023

nicolasburtey Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

bodymindarts Jun 18, 2023

Implement Batch Payments Feature #155

Implement Batch Payments Feature #155

Conversation

eshanvaid commented Jun 14, 2023

eshanvaid commented Jun 14, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

eshanvaid commented Jun 14, 2023 •

edited

Loading