Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support liquid trades import #281

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Zahrun
Copy link
Contributor

@Zahrun Zahrun commented Dec 13, 2022

No description provided.

@vercel
Copy link

vercel bot commented Dec 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cryptotithe ❌ Failed (Inspect) Dec 15, 2022 at 3:56PM (UTC)

export default async function processData(importDetails: IImport): Promise<ITrade[]> {
const data: ILiquid[] = await getCSVData(importDetails.data) as ILiquid[];
const internalFormat: ITrade[] = [];
if (data.length < 1) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on moving line 30-34 into its own small function for all the parsers?

I feel like we just keep repeating this for all of them. Doesnt have to be in this PR but just curious what your thoughts are on it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ that link is not working for me for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s because I changed the whole liquid code in between

if (data.length < 1) {
return internalFormat;
}
let splitTrade = data[0];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm this is because again 1 trade = 2 line items?

What about if we did a for loop here(instead of for of) and then just incremented 2 each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is because 1 trade = 2 lines.
This is also actually the reason for the if statement line 32-34. So that would be necessary for exchanges which split trades over multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new solution is much better. This liquid file has some very strange and inconsistent looking data as well, worse than Revolut.

src/types/locations.ts Outdated Show resolved Hide resolved
@Zahrun
Copy link
Contributor Author

Zahrun commented Dec 14, 2022

In case you want to check the data
CryptoTransactions.csv

exchange : EXCHANGES.Liquid,
exchangeID : execution,
};
if (execution === '') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant we just do !!execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know that notation. Here I’m testing if the executionID is '', when the cell is empty in the csv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to replace the condition with !!execution, but the result is not the same, it ignores every line

const data: ILiquid[] = await getCSVData(importDetails.data) as ILiquid[];
const internalFormat: ITrade[] = [];
const sorted = data.reduce(groupByExecutionID, {});
for (const execution of Object.keys(sorted)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beliieve we can do a for in loop here instead which will expose the key and the value rather then doing for of on the keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced with in loop

break;
}
case 6: {
internalFormat.push(addFeeTrade(tradeToAdd, trades[4], trades[5]));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt there be a regular trade here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you look at the data in the csv, the lines cancel themselves. Only the fee stays there. I’m not sure what it means

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I changed the code so that even if it cancels itself, it shows up as a trade. So we actually import the data.

}
case 8: {
let secondTrade = tradeToAdd;
internalFormat.push(addFeeTrade(tradeToAdd, trades[4], trades[5]));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really get this what happens to trades 0 - 3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know why these trades over 6 or 8 lines exist but they all cancel themselves. Maybe a cancelled order or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll send a request to Liquid support to check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, I changed the code so that even if it cancels itself, it shows up as a trade. So we actually import the data.

tradeToAdd.boughtCurrency = 'USDT';
tradeToAdd.soldCurrency = feeTrade.currency;
tradeToAdd.amountSold = amount;
tradeToAdd.rate = amount / 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we dividing by zero here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m paying fee but not receiving anything for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if we add tradeFee and tradeFeeCurrency, that would help. I’ll make a commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I replaced this with a trade of amount 0, rate 1, and it has a fee that is the actual amount that is paid.

@starsoccer
Copy link
Owner

In case you want to check the data CryptoTransactions.csv

I cant seem to open this file either

@Zahrun
Copy link
Contributor Author

Zahrun commented Dec 14, 2022

In case you want to check the data CryptoTransactions.csv

I cant seem to open this file either

Seems fine to me, downloaded from the same link

@starsoccer
Copy link
Owner

In case you want to check the data CryptoTransactions.csv

I cant seem to open this file either

Seems fine to me, downloaded from the same link

Is this a CSV or something else?

@Zahrun
Copy link
Contributor Author

Zahrun commented Dec 14, 2022

In case you want to check the data CryptoTransactions.csv

I cant seem to open this file either

Seems fine to me, downloaded from the same link

Is this a CSV or something else?

Simple UTF-8 csv
image

@Zahrun
Copy link
Contributor Author

Zahrun commented Dec 15, 2022

So there are two files in the statements, one for Crypto and one for Fiat. I changed the import process so that it can manage the two different header hashes and also it asks for a second file. Please try these files:
CryptoTransactions.csv
FiatTransactions.csv

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants