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

Resolve ops in edge cases #136

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

jhoogstraat
Copy link
Contributor

@jhoogstraat jhoogstraat commented May 16, 2022

This is the pr for issue #135.

It's not finished yet, but I wanted to allow for feedback early on.

Implemented edge cases:

  • Two pairs at a particular utc_time via a bridge coin
  • Orders not being fulfilled at once, leading to Buy/Sell ops later on (can this even happen? Buy/Sell come in pairs, no?)
  • Commissions / Withdrawals / Deposits / Airdrop / CoinLendInterest not relevant for trade linking
  • BNB small asset exchange at multiple utc_times (buy/sell 1 second apart for me)
  • Fees paid (partly) in BNB in the double_buy_sell_pair case

Currently, the is_binance_bnb_small_asset_transfer case catches almost everything that was not handled by the other cases.
I tried my best to make sure that only ops that really are small asset exchanges get handled (assert that the buy coin is BNB).

@jhoogstraat
Copy link
Contributor Author

jhoogstraat commented May 16, 2022

I merged the two methods for resolving fees and trades into one.
This reduces duplication by quite a bit.
It also enables us to resolve fees easily when there are two buy/sell-pairs at any given utc_time.
I know that separating the two methods is good practice, but I think in this case it adds too much duplication and complexity.

I am not sure how to account for fees paid in bnb when there is more than one buy op... I added a BUG to the code at the relevant line. How can we detect which bnb-fee op belongs to which buy op?

@provinzio
Copy link
Owner

I merged the two methods for resolving fees and trades into one.

Make sure that resolving fees and resolving trades can be something different. E.g. the fee could also belong to a deposit

I am not sure how to account for fees paid in bnb when there is more than one buy op... I added a BUG to the code at the relevant line. How can we detect which bnb-fee op belongs to which buy op?

Split the BNB fee according to the "value" of the sell. We can use the price in eur to estimate that

@jhoogstraat
Copy link
Contributor Author

jhoogstraat commented May 17, 2022

Make sure that resolving fees and resolving trades can be something different. E.g. the fee could also belong to a deposit

True. Maybe have fees done in multiple passes? What do you prefer / think is better?

Split the BNB fee according to the "value" of the sell. We can use the price in eur to estimate that

Like calculating the expected fee and find a combination of fee ops matches the calculated fee best? Fees can be quite different depending on volume on Binance. Don't know about other platforms.

@provinzio
Copy link
Owner

provinzio commented May 17, 2022

True. Maybe have fees done in multiple passes? What do you prefer / think is better?

Matching of deposit/withdrawals, coin lend/staking start and end, trading pairs and finally matching everything with fees should be possible in the same loop

In that case we might have to group and match everything before the "merge of equal operations"

@jhoogstraat
Copy link
Contributor Author

Im am first going to concentrate on trades with fees. We can add lending/staking, etc. later.

@jhoogstraat jhoogstraat marked this pull request as ready for review May 18, 2022 11:45
@provinzio provinzio added the Book label Jun 4, 2022
@provinzio
Copy link
Owner

provinzio commented Jun 4, 2022

Hey @jhoogstraat,

I went through the code and did some formatting. Didn't wanted to bother you with that. I merged the newest main into the branch, fixed the linting errors and added some more assert checks.

Changes look good to me. I'd love when you could proof read my commits. Is the PR ready to be merged? Looking forward to your reply. For now, I'll mark this as draft to keep an overview. :)

@provinzio provinzio marked this pull request as draft June 4, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants