-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this 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
src/parsers/trades/gate.io/index.ts
Outdated
if (data.length < 1) { | ||
return internalFormat; | ||
} | ||
let feeTrade = data[0]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
} | ||
} | ||
tradeToAdd.ID = createID(tradeToAdd); | ||
tradeToAdd.exchangeID = tradeToAdd.ID; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see an example file |
No description provided.