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

Add basic accounting capabilities #85

Open
ghost opened this issue Sep 3, 2020 · 15 comments · May be fixed by #125
Open

Add basic accounting capabilities #85

ghost opened this issue Sep 3, 2020 · 15 comments · May be fixed by #125
Assignees
Labels
enhancement New feature or request P2 mid priority tbd to be discussed
Milestone

Comments

@ghost
Copy link

ghost commented Sep 3, 2020

This issue is about adding basic accounting capabilities to arby for the end user. Details below.

@ghost ghost self-assigned this Sep 3, 2020
@ghost ghost added enhancement New feature or request tbd to be discussed labels Sep 3, 2020
@kilrau kilrau added the P2 mid priority label Sep 4, 2020
@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

We just discussed adding a basic /trades endpoint which takes a date range as filter and accepts above proposed granularity, e.g. as /trades?startDate=20200101&endDate=20201231 returning data of the executed CEX trades, along with the uuid of the related OpenDEX trade + arbitrage profit.

Data fields this should return:

  • cexOrderId (CEX order id, NOT tradeIDs)
  • side (CEX)
  • amount (e.g. 0.1 ETH, CEX)
  • price (e.g. 0.002 BTC, CEX)
  • time (unix timestam, CEX)
  • profit (CEX - OpenDEX)
  • opendexOrderId (order uuid from xud)

@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

This feature is needed for the "Closed CEX Trades" view in the Arby/MM Bot secion in xud-ui, which only shows closed arby trades on the CEX side + how much profit was made with the arbitrage to the OpenDEX trade:
image

It also composes some of this info into a graph:
image

@kilrau kilrau added this to the 1.3.1 milestone Dec 11, 2020
@ghost
Copy link
Author

ghost commented Dec 11, 2020

What does order side mean under closed trades? It's always both - either buy on CEX and sell on DEX or vice versa.

@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

Very good point! How about now?

@ghost
Copy link
Author

ghost commented Dec 11, 2020

For the chart we'll need to data in the following format:

/profit?startDate=xyz&endDate=zyx&granularity=day

{
  result: [
    { label: '1st of Jan', profit: 0.1},
    { label: '2nd of Jan', profit: 0.025},
    ...
  ]
}

Thinking it might make sense to have a separate endpoint for the closed trades info since it's on a separate screen and does not need granularity.

@ghost
Copy link
Author

ghost commented Dec 11, 2020

Very good point! How about now?

That works.

Same question for the amount. Is it always the non-BTC asset?

@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

Thinking it might make sense to have a separate endpoint for the closed trades info since it's on a separate screen and does not need granularity.

It might in future...

@ghost
Copy link
Author

ghost commented Dec 11, 2020

Thinking it might make sense to have a separate endpoint for the closed trades info since it's on a separate screen and does not need granularity.

It might in future...

It just needs the date range though? startDate and endDate?

Or limit (show last 100 trades).

@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

Same question for the amount. Is it always the non-BTC asset?

Oh damn, that took a while. So we discussed at some point to expose CEX data only here on that mm bot page along with the profit info, since all OpenDEX trade data can be looked up in Tradehistory and we don't want to have that double.

Updated.

@kilrau
Copy link
Contributor

kilrau commented Dec 11, 2020

Thinking it might make sense to have a separate endpoint for the closed trades info since it's on a separate screen and does not need granularity.

It might in future...

It just needs the date range though? startDate and endDate?

Or limit (show last 100 trades).

True. Adjusted to date range only in #85 (comment)

@kilrau
Copy link
Contributor

kilrau commented Dec 15, 2020

I just onboarded @rsercano to this @erkarl - any questions/implementation proposal - let's track it here

@kilrau kilrau assigned rsercano and unassigned ghost Dec 15, 2020
@rsercano
Copy link

rsercano commented Dec 19, 2020

Hi, this's complicated for a single issue imo, and I guess either we need to split it into PRs, or sub issues somehow. So I was thinking to split into below PRs.

  • Introducing sequelize and a new db type arby-order.ts
  • Getting order from CEX cctx & saving into db async after converting to arby-order.ts. (Open to suggestions about async process, if it can be done on rx-js)
  • Introducing http webserver to arby.
  • Implementing new /trades endpoint as explained above.

or maybe two by combining implementing & introducing parts

wdyt? @erkarl @kilrau

@ghost
Copy link
Author

ghost commented Dec 19, 2020

Introducing sequelize and a new db type arby-order.ts

Let's start with adding sequelize support and define what we're going to save to the database.

@rsercano
Copy link

Edited my comment to be brief, @erkarl started to implement db model

@kilrau
Copy link
Contributor

kilrau commented Dec 21, 2020

As just discussed with @erkarl and @rsercano :

  • we can't link each Binance trade with an OpenDEX trade directly (e.g. arby might accumulate xud trades if amount is too small to execute on Binance and later issue one binance order covering multiple xud trades)
  • instead we'll use the ccxt order output and save it to a binance trades db as is
  • profit will be only be calculatable for a given time frame: sum up (price x amount) binance & xud, subtract binance from xud = profit

Removed Wireframe adjusted too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2 mid priority tbd to be discussed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants