Feat/collapse payments csv by lissavxo · Pull Request #942 · PayButton/paybutton-server

lissavxo · 2025-02-10T15:52:35Z

Related to #934

Description

Change the CSV logic to collapse payments with value smaller than $1 USD in the same day.
Added two new columns to the file Collapsed and Notes.
If the line contains payment Collapsed the column will be true and in Notes we will display "<button-name> - <payments-collapsed-count> transactions"

Test plan

Test with one address that has a bunch of payments smaller than $1 USD se if the behavior of collapsing is working
Also test with a list of payments smaller than $1 USD and a larger payment between those payments, the behavior should be 3 lines 2 collapsed and 1 not collapsed following the order of the date.

Klakurka · 2025-02-10T16:04:07Z

Tests failing.

Klakurka

Let's remove the 'Collapsed' column from the CSV.
Let's put all of the transaction Ids into the tx id column, comma separated (assuming that's possible).
How about adding some tests around making sure the amounts and values add up correctly (as in the same amount and value totals as when not collapsed).

Klakurka · 2025-02-10T16:36:32Z

constants/index.ts

+
 export const DEFAULT_PAYBUTTON_CSV_FILE_DELIMITER = ','
 export const MAX_RECORDS_PER_FILE = 10000
+export const DEFAULT_COLLAPSE_THRESHOLD_FILE_USD = 1


How about changing this from USD to DEFAULT_CSV_COLLAPSE_THRESHOLD and in a follow-up branch, we can make this configurable in the account settings? The threshold will use whatever currency they have set, not just USD.

lissavxo · 2025-02-10T19:10:21Z

Let's remove the 'Collapsed' column from the CSV.

Let's put all of the transaction Ids into the tx id column, comma separated (assuming that's possible).

How about adding some tests around making sure the amounts and values add up correctly (as in the same amount and value totals as when not collapsed).

The CSV uses comma to separate the columns so separating the tx Ids with commas will mess up the file, can we do semicolon (;) ?

Klakurka · 2025-02-11T20:37:44Z

Let's remove the 'Collapsed' column from the CSV.

Let's put all of the transaction Ids into the tx id column, comma separated (assuming that's possible).

How about adding some tests around making sure the amounts and values add up correctly (as in the same amount and value totals as when not collapsed).

The CSV uses comma to separate the columns so separating the tx Ids with commas will mess up the file, can we do semicolon (;) ?

Sure we can go with that.

Klakurka

I know some may not want this but for us, we definitely need this to be done in both the Payments and Transactions CSVs. We'll eventually make this configurable.
Let's do the same thingh we're doing with txids with addresses (put them all in but semicolon-separated).

Klakurka

Something seems to be breaking the formatting:
The 'Amount' column is being rounded. We want to keep the decimal places (2 for XEC, 8 for BCH).
I'm testing w/ EL addresses.

lissavxo · 2025-02-14T17:54:31Z

Something seems to be breaking the formatting:

The 'Amount' column is being rounded. We want to keep the decimal places (2 for XEC, 8 for BCH).
I'm testing w/ EL addresses.

this is due to opening CSV in excel, changed the delimiter to semicolon (;) it will make it readable in excel

Klakurka

Same issue again

Klakurka

Still an issue:

Looks like the problem is that there's a max length on each row (or cell).

I think I'm ok with just omitting the tx ids if there's multiple. Businesses are unlikely going to need to reference the tx id on small payments. The important ones will be the big ones. If there's more than one tx (collapsed row), just say 'Multiple'.

constants/index.ts

Klakurka

I went through and did a more thorough check of the transactions and it appears that we're missing a bunch.

Here's some dates where we should have transactions and we somehow don't in the CSV (for the 5 eCash Land addresses; 6733 txs total):

2024-02-13
2024-03-06
2024-05-23
2024-05-30
2024-06-11
2024-08-23
2024-10-17
2024-10-21
2024-11-19
2025-01-09

Klakurka

The tx count lines up now, but the amounts and values don't.

For example, 2024-10-19 should be 2068.67 XEC, not 2069 XEC (seemingly rounded).

Klakurka

Exchange rates are sometimes rounding to 8 decimal places when they should be 14 (which is what the CD API returns).

Klakurka

Looks good - in production, I'm going to spend some extra energy making sure everything is as expected before closing the task.

chedieck

Requesting:

bug fix for 8 decimal places on fiat value if dealing with a bch address,
avoiding possible float precision errors,
naming improvements,
other refactors

constants/index.ts

chedieck · 2025-02-26T10:25:20Z

utils/files.ts

+
+const getPaybuttonTransactionsFileData = (transactions: TransactionsWithPaybuttonsAndPrices[], currency: SupportedQuotesType, timezone: string): TransactionFileData[] => {
+  const paymentsFileData: TransactionFileData[] = []
+  transactions.forEach(element => {


element is a bad name for what you know is a transaction. Any array has elements, this is the variable-naming equivalent of typing something as any. Just call it tx or transaction.

chedieck · 2025-02-26T10:40:00Z

utils/files.ts

+    const { amount, hash, address, timestamp } = element
+    const value = getTransactionValueInCurrency(element, currency)
+    const date = moment.tz(timestamp * 1000, timezone)
+    const rate = value / amount.toNumber()


I couldn't find any float precision error myself, but I don't feel confortable knowing it can happen.

We do have this price rate in the DB, no need to recalculate it from the value / amount, since we literally calculated the value from this price.

You can use something like

const price = tx.prices.find(p => p.price.quoteId = QUOTE_IDS[currency.toUpperCase()])!.price.value

chedieck · 2025-02-26T10:40:53Z

utils/files.ts

+    const { timestamp, hash, address, amount } = tx
+    const values = getTransactionValue(tx)
+    const value = Number(values[currency])
+    const rate = value / Number(amount)


Same thing I commented below about float precision errors

chedieck · 2025-02-26T10:45:55Z

utils/files.ts

+  collapseThreshold: number
+): TransactionFileData[] => {
+  const treatedPayments: TransactionFileData[] = []
+  const tempGroups: Record<string, TransactionsWithPaybuttonsAndPrices[]> = {}


tempTxGroups is more clear

chedieck · 2025-02-26T10:46:07Z

utils/files.ts

+  let totalPaymentsTreated = 0
+
+  const pushTempGroup = (groupKey: string): void => {
+    const tempGroup = tempGroups[groupKey]


tempTxGroup here

chedieck · 2025-02-26T10:49:51Z

utils/files.ts

+    }
+    const totalAmount = tempGroup.reduce((sum, p) => sum + Number(p.amount), 0)
+    const totalValue = tempGroup.reduce((sum, p) => sum + Number(getTransactionValue(p)[currency]), 0)
+    const rate = totalValue / totalAmount


Same thing I commented below about float precision error, with the difference that here we have many txs. These txs, however, all belong to the same UTC day, so they should have the same price. What you could do is

const uniquePrices = new Set( tempTxGroup .map(tx => tx.prices.find(p => p.price.quoteId === QUOTE_IDS[currency.toUpperCase()])!.price.value) );

and then

if (uniquePrices.length !== 1) { // Should never come here throw new Error(RESPONSE_MESSAGES.500_INVALID_PRICES_FOR_TX_ON_CSV_CREATION(uniquePrices.length)) } const rate = uniquePrices[0]

chedieck · 2025-02-26T10:53:32Z

utils/files.ts

+    amount: amount.toFixed(DECIMALS[networkTicker]),
    date: date.format(PRICE_API_DATE_FORMAT),
-    value: value.toFixed(2),
+    value: value.toFixed(DECIMALS[networkTicker]),


This is wrong, will lead to USD/CAD values having 8 decimal digits if the networkId is 2, for BCH.

Should be DECIMALS.FIAT.

chedieck · 2025-02-26T10:57:23Z

pages/api/paybutton/download/transactions/[paybuttonId].ts

+    let networkIdArray = Object.values(NETWORK_IDS)
+    if (networkTicker !== undefined) {
+      const slug = Object.keys(NETWORK_TICKERS).find(key => NETWORK_TICKERS[key] === networkTicker)
+      const networkId = getNetworkIdFromSlug(slug ?? NETWORK_TICKERS.ecash)


Why are you defaulting to ecash? Maybe if slug is invalid, we should just throw an error, we already have INVALID_NETWORK_SLUG_400.

Also could have const slug = NETWORK_SLUGS_FROM_IDS[NETWORK_IDS[ticker]]

Also, this same codeblock appears right above downloadTxsFile, on both the only two places this function is ever called. Why not put it inside it, then?

Also, this same codeblock appears right above downloadTxsFile, on both the only two places this function is ever called. Why not put it inside it, then?

Because the networkIdArray is used to fetch the txs something we are doing outside downloadTxsFile function

chedieck · 2025-02-26T21:09:59Z

pages/api/paybutton/download/transactions/[paybuttonId].ts

    if (networkTicker !== undefined) {
-      const slug = Object.keys(NETWORK_TICKERS).find(key => NETWORK_TICKERS[key] === networkTicker)
-      const networkId = getNetworkIdFromSlug(slug ?? NETWORK_TICKERS.ecash)
+      const slug =  NETWORK_SLUGS_FROM_IDS[NETWORK_IDS[networkTicker]]


Just noticed: why get the slug from the id if you're gonna get the id from the slug? if the goal is to get the id, just forget the slug, check if the ticker is valid (we have a variable with the valid network tickers) and then get the id with NETWORK_IDS[networkTicker]

chedieck · 2025-02-26T21:10:07Z

pages/api/payments/download/index.ts

    if (networkTicker !== undefined) {
-      const slug = Object.keys(NETWORK_TICKERS).find(key => NETWORK_TICKERS[key] === networkTicker)
-      const networkId = getNetworkIdFromSlug(slug ?? NETWORK_TICKERS.ecash)
+      const slug =  NETWORK_SLUGS_FROM_IDS[NETWORK_IDS[networkTicker]]


Same thing here

chedieck · 2025-02-26T21:15:24Z

constants/index.ts

  FAILED_TO_FETCH_PRICE_FROM_API_500: (day: string, ticker: string) => { return { statusCode: 500, message: `Failed to fetch ${ticker} price for day ${day}` } },
  MISSING_WS_AUTH_KEY_400: { statusCode: 400, message: 'Missing WS_AUTH_KEY environment variable' },
  MISSING_PRICE_FOR_TRANSACTION_400: { statusCode: 400, message: 'Missing price for transaction.' },
+  INVALID_PRICES_FOR_TX_ON_CSV_CREATION_500: (pricesLenght: number) => {  return  {statusCode: 500, message: `Missing price for transaction in CSV creation. ${pricesLenght}` }},


The variable name and message are not right:

To be more precise, is the number of prices that is wrong, not the prices themselves. But more than that, this happens if there is a number != 1 of prices, so there could be missing prices or there could be more prices than expected. This error says "Missing price", which could not always be the case, and just throw a random number at the end without explain what it is. A better approach here would be:

INVALID_PRICES_AMOUNT_FOR_TX_ON_CSV_CREATION_500: (pricesLenght: number) => { return { statusCode: 500, message: `Wrong number of prices for transactions group in CSV creation. Expected 1, got ${pricesLenght}`. }},

chedieck

Approving since this has been requested already for some time, but I think tests should improve in the future to also include the verification of the correctness of dividing same-utc-day lines into different lines across different timezones.

Klakurka

I'll approve but I agree on the tests needing to be better on follow-ups.

lissavxo requested a review from Klakurka February 10, 2025 15:52

Klakurka requested changes Feb 10, 2025

View reviewed changes

Klakurka requested a review from chedieck February 10, 2025 16:36

lissavxo requested a review from Klakurka February 12, 2025 19:16

Klakurka requested changes Feb 12, 2025

View reviewed changes

lissavxo requested a review from Klakurka February 13, 2025 22:27

Klakurka requested changes Feb 13, 2025

View reviewed changes

lissavxo requested a review from Klakurka February 14, 2025 18:03

Klakurka requested changes Feb 14, 2025

View reviewed changes

Klakurka requested changes Feb 19, 2025

View reviewed changes

lissavxo requested a review from Klakurka February 19, 2025 15:10

Klakurka requested changes Feb 19, 2025

View reviewed changes

constants/index.ts Outdated Show resolved Hide resolved

lissavxo requested a review from Klakurka February 19, 2025 20:51

lissavxo force-pushed the feat/collapse-payments-csv branch from 440ce76 to bc12ff9 Compare February 19, 2025 20:56

Klakurka approved these changes Feb 19, 2025

View reviewed changes

Klakurka requested changes Feb 20, 2025

View reviewed changes

lissavxo force-pushed the feat/collapse-payments-csv branch from bc12ff9 to 93a681b Compare February 21, 2025 20:46

Klakurka requested changes Feb 21, 2025

View reviewed changes

lissavxo force-pushed the feat/collapse-payments-csv branch from cd9e4e4 to e3f6f34 Compare February 22, 2025 01:00

Klakurka requested changes Feb 22, 2025

View reviewed changes

lissavxo requested a review from Klakurka February 25, 2025 22:50

lissavxo added 5 commits February 25, 2025 21:11

feat: collapse payments by day

a9c380f

feat: csv collapse payments orderded

0178755

feat: const DEFAULT_COLLAPSE_THRESHOLD_FILE_USD

74dd2fb

fix: file types

c08e615

feat: clean code

864f5f7

lissavxo added 17 commits February 25, 2025 21:11

fix: collapse threshold var

8f7c322

refactor: download csv method

7e69132

refactor: download csv button transactions

6fd29ee

tests: collapse payments update

22be9df

feat: collapse by default

9bc0c1a

tests: fix transaction service

4ac7131

feat: concatenate addresses

afc23ce

feat: change csv delimiter to ;

09b1c6f

tests: fix

30473a2

feat: change csv delimiters

2dcf289

feat: remove addresses repetition csv

634b6a6

feat: remove txIds concatenation

e226369

test: remove concat txid case

631e496

fix: multiple values line

2d8e8a9

refactor: collapseSmallPayments logic

483c0f9

fix: amount decimals csv

54e443e

feat: separate utc tx csv

933ce99

lissavxo force-pushed the feat/collapse-payments-csv branch from 0dfa4c7 to 933ce99 Compare February 26, 2025 00:11

Klakurka previously approved these changes Feb 26, 2025

View reviewed changes

chedieck requested changes Feb 26, 2025

View reviewed changes

refactor: clean up code

2309403

lissavxo dismissed Klakurka’s stale review via 2309403 February 26, 2025 19:40

tests: remove old test

2754bd0

chedieck requested changes Feb 26, 2025

View reviewed changes

chedieck added 2 commits February 26, 2025 18:29

fix: no getting unecessary slug

1cbc1a3

fix: wrong assignment & error message for wrong amount of prices

eb1cede

chedieck self-requested a review February 26, 2025 21:37

chedieck approved these changes Feb 26, 2025

View reviewed changes

Klakurka approved these changes Feb 26, 2025

View reviewed changes

Klakurka merged commit bf6238b into master Feb 26, 2025
2 checks passed

Conversation

lissavxo commented Feb 10, 2025

Description

Test plan

Uh oh!

Klakurka commented Feb 10, 2025

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lissavxo commented Feb 10, 2025

Uh oh!

Klakurka commented Feb 11, 2025

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lissavxo commented Feb 14, 2025

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Uh oh!

chedieck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chedieck left a comment

Choose a reason for hiding this comment

Uh oh!

Klakurka left a comment

Choose a reason for hiding this comment

Klakurka left a comment •

edited

Loading

Klakurka left a comment •

edited

Loading