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 gate.io trades import #279

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

Conversation

Zahrun
Copy link
Contributor

@Zahrun Zahrun commented Dec 11, 2022

No description provided.

@vercel
Copy link

vercel bot commented Dec 11, 2022

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

Name Status Preview Updated
cryptotithe ❌ Failed (Inspect) Dec 17, 2022 at 1:34PM (UTC)

Copy link
Owner

@starsoccer starsoccer left a comment

Choose a reason for hiding this comment

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

If you have an example CSV that would be useful in order to comment on how we may be able to better do some things here

if (data.length < 1) {
return internalFormat;
}
let feeTrade = 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.

Why are we setting all of these to 0?

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 was getting filled trade is read before assigned at line 53 tradeToAdd.boughtCurrency = filledTrade.Currency; so I assign it to something first. It should be assigned in the switch structure but that does not compile.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay so just to confirm Im understanding this right basically the reason your doing this is because 2 lines = 1 trade. One line is for the bought amount, then another for the sold amount right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trades are on three lines and "Points Purchase" are on two lines, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the lines of trades is the fee and the fee currency.
I had configured to pay the fee in POINT, which is not the bought currency nor the sold currency.
I thought that transactionFee was for that but you told me it’s not. So how to reflect this trade with 3 currencies?

Copy link
Contributor Author

@Zahrun Zahrun Dec 15, 2022

Choose a reason for hiding this comment

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

Can’t we use transactionFee and transactionFeeCurrency also for tradeFee and tradeFeeCurrency?
The ideas are quite similar. To detect if it is a transactionFee or a tradeFee, we can test if one of the soldCurrency or boughtCurrency are fiat.

Copy link
Owner

Choose a reason for hiding this comment

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

They are similiar but serve different purposes. For instance transactionFee is more intended for on chain exchanges. So like trading USDC to some other token and paying for the transaction with ETH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay then let's implement tradeFee. See a strart in #281

Copy link
Owner

Choose a reason for hiding this comment

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

I am going to make a discussion on it first as I dont want to implement this until some of the wasm stuff Ive done is merged in as that will cause breaking changing

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 pushed a better management of trades over multiples lines. And a basic support for tradeFee, at least for import

src/parsers/trades/gate.io/index.ts Outdated Show resolved Hide resolved
}
}
tradeToAdd.ID = createID(tradeToAdd);
tradeToAdd.exchangeID = tradeToAdd.ID;
Copy link
Owner

Choose a reason for hiding this comment

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

I believe you should be able to move this up to the top. Also ideally we should set this before we create the ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So ID would be set to createID after the initial partial trade setting?
I modified the code from the binance parser and followed that structure.
How come exchangeID = trade.ID?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, and good point the binance parser should be updated. Its outdated and now not making IDs properly.

Ideally when we fix it we should bump the version and make part of the migration process to recompute all trade ids as they will all change then

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 also plan to update the Binance parser. I have exported the data from Binance with the "all statements" feature.
All kind of operations are there.
In that case, should we import trades, transactions, and income all at once?
Or should I make a parser for each type, all of them taking only the concerned type of operations?

Bump the version you mean from "version":"0.8.1", and in migration recalculate all IDs, ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@starsoccer Please if you can tell your opinion about this:
"I also plan to update the Binance parser. I have exported the data from Binance with the "all statements" feature.
It contains operations of all different kinds.
In that case, should we import trades, transactions, and income all at once?
Or should I make a parser for each type, all of them taking only the concerned type of operations?"
It is not the case only with Binance, also with other platforms.

I would say import all at once, but I’m also ok with implementing it as separate, since it is the current structure.

See all part.csv

Copy link
Owner

Choose a reason for hiding this comment

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

I cant seem to open that file. Is that a zip or csv?

And regarding the binance question, I am fine with it all being in one parser, but we may need to do some tweaking to the current import design because I believe it asks for type first(transaction, income, or trade)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try this one (it’s the Binance one)

all part.csv

I will write it as an all-in-one importer then

@Zahrun
Copy link
Contributor Author

Zahrun commented Dec 11, 2022

Please see an example file
mypurselog_2022_12_11.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