feat: adding sequelize along with new db type 'Order' (#85) #125

rsercano · 2020-12-20T08:14:18Z

Hello,

this PR implements the following changes:

Added a new db type along with db connection ArbyOrder
Fixed a small issue on .env-example (TEST_MODE instead of LIVE_CEX)
Changed initialization flow to add db initialization into arby.ts
Created a new log context db and used for sequelize

Next steps:

Using ArbyOrder model in mergeMap's project here: and giving it to initCEX$
Optionally adding tests for initDB$ (it's up to you if you want me to add or not @kilrau @erkarl)

I guess other steps (querying cex to fetch order and save) would be another PR since it would make this more than it seems. But it's up to you @erkarl if you want me to continue on this before merging, I'm okay with it too.

Also I'm still open to suggestions about how to fill openDexOrderId while executing a cex order @erkarl (but that's next topic)

ghost

Thanks for getting started on this one.

The database structure you're suggesting to implement is a bit unclear for me. Left some comments/questions.

How are you planning to query BTC profit earned/accumulated between X and Y timestamps?

src/db/arby-order.ts

rsercano · 2020-12-20T13:17:02Z

Thanks for getting started on this one.

The database structure you're suggesting to implement is a bit unclear for me. Left some comments/questions.

How are you planning to query BTC profit earned/accumulated between X and Y timestamps?

Hi @erkarl thanks for prompt review!

In fact as far as I understood from @kilrau this profit is per trade there's no calculation between timestamps, and since we'll save this into database whenever a CEX order happens, I guess that's the case, what I had in my mind for the rest:

CEX order executed ->
-- Query cex order to find price/quantity/timestamp etc..
-- Find corresponding dex order to save dexOrderId.

And for profit calculation it just should be a simple minus operation, doesn't it? @kilrau @erkarl

ghost

One DEX buy order can be linked to multiple CEX orders, and vice-versa.

For example, if there were many DEX orders fills with very small quantity. In that case, the bot will not issue order on CEX until a configurable threshold is met.

What do you think about starting with saving all order fills (both from CEX and DEX side) to the database and timestamp it. That way we'll get the most raw data and can derive whatever metrics we want for visualization purposes and whatever timeframe.

src/db/arby-order.ts

rsercano · 2020-12-20T14:00:47Z

One DEX buy order can be linked to multiple CEX orders, and vice-versa.

For example, if there were many DEX orders fills with very small quantity. In that case, the bot will not issue order on CEX until a configurable threshold is met.

What do you think about starting with saving all order fills (both from CEX and DEX side) to the database and timestamp it. That way we'll get the most raw data and can derive whatever metrics we want for visualization purposes and whatever timeframe.

I see, so in this way we'll save whatever order is executed and how will we link them?

ghost · 2020-12-20T14:18:26Z

and how will we link them?

There's no hard link. The query will always include start timestamp and end timestamp.

"Show me all the trades in the last 24h". We can then calculate the amounts sold/bought during the period and derive the PnL (profit and loss)

rsercano · 2020-12-20T14:43:08Z

oh I see @erkarl if that's okay by you guys, okay for me too ofc. @kilrau @erkarl

rsercano · 2020-12-21T14:39:42Z

export interface Order {
        id: string;
        datetime: string;
        timestamp: number;
        lastTradeTimestamp: number;
        status: 'open' | 'closed' | 'canceled';
        symbol: string;
        type: string;
        side: 'buy' | 'sell';
        price: number;
        average?: number;
        amount: number;
        filled: number;
        remaining: number;
        cost: number;
        trades: Trade[];
        fee: Fee;
        info: any;
    }

After the call with @erkarl and @kilrau we'll save the order as is with linked Trades in the database as a first step

rsercano · 2020-12-23T08:37:31Z

As an outcome to our discussion,

I changed ArbyOrder name to CexOrder
Created two new db types CexFee & CexTrade
CexFee has one to one relation with both CexTrade & CexOrder
CexOrder has one to many relation with CexTrade

Just as in the ccxt.

Next step for this PR:

Saving cex order as soon as it's executed.

ghost

Thanks for moving forward with this.

In general I think we can lose the Cex prefix for database fields CexOrder -> Order, CexTrade -> Trade, CexFee -> Fee.

I'm also a bit unsure about Fee in a separate table. Going to think about it for a while.

src/db/db.ts

src/db/cex-fee.ts

rsercano · 2020-12-25T07:14:55Z

So as per your comment @erkarl I suppose while saving fee into Order, I guess it's easier to do when we don't separate Fee as you said, I'm going to change it and put fee fields into order.

rsercano · 2020-12-25T09:39:54Z

Hi @erkarl what I've added saveOrder$ observable to executeOrder of Cex, could you please check? To be honest I couldn't test this %100 since it's not being used on test mode.

Also I added a TODO to orderRepository to use schedulers of rxjs to make save operation async, wdyt @erkarl ?

ghost · 2020-12-29T09:09:31Z

To be honest I couldn't test this %100 since it's not being used on test mode.

You definitely should be able to test this manually.

rsercano · 2020-12-29T09:17:43Z

To be honest I couldn't test this %100 since it's not being used on test mode.

You definitely should be able to test this manually.

Uhm, can you point me to right direction I literally never saw a log message about centralized order, I saw you are logging either way it's test or not, but I never ended up in the logs with one.

src/db/order-repository.ts

src/db/db.ts

rsercano · 2021-01-02T10:15:50Z

Can you please check this once more @erkarl there was an issue because I was calling closeDB$ in cleanup but that's not right since DB close operation has to happen during shutdown and has to happen once not on cleanup.

rsercano · 2021-01-04T08:58:26Z

fyi. @erkarl I'll create a new branch over this one and will start HTTP implementation

ghost · 2021-01-04T13:36:59Z

fyi. @erkarl I'll create a new branch over this one and will start HTTP implementation

@rsercano did you test this PR with real environment?

rsercano · 2021-01-04T13:38:22Z

fyi. @erkarl I'll create a new branch over this one and will start HTTP implementation

@rsercano did you test this PR with real environment?

I tested by calling saveOrder$ subscribe with a dummy order, not with a real environment unfortunately.

ghost · 2021-01-04T18:06:40Z

src/arby.ts

  }).subscribe({
    error: error => {
      if (error.message) {
        console.log(`Error: ${error.message}`);
      } else {
        console.log(error);
      }
-      process.exit(1);
+      closeDB$().subscribe({


Whenever you need to subscribe within a subscribe callback you should probably be using a transformation operation instead (such as mergeMap).

I changed this by adding a pipe to startArby observable and then calling closeDB observable within mergeMap, but I'm really unsure if it worked, because if I put console.log statements inside to mergeMap (that I call closeDB), they're never getting called, and never printing, can you please check @erkarl

I followed here: https://rxjs-dev.firebaseapp.com/api/operators/mergeMap

Line that I suspect is not working: https://github.com/ExchangeUnion/market-maker-bot/pull/125/files#diff-77a45f12e136f324d3c28fe24cdf3a531c7582103e96498a48462eaf7414a509R181

ghost · 2021-01-04T18:10:42Z

src/db/order-repository.ts

+
+  const arbyOrder = convertToArbyOrder(order);
+  const arbyTrades = convertToArbyTrades(order.trades, order.id);
+  // TODO async


Yes actually saving into database is not a big deal, but may interrupt the responsiveness of arby, so I added a TODO there to take care later and make save operation async with schedulers of rxjs, wdyt?

src/trade/cleanup.spec.ts

rsercano requested review from a user, kilrau and raladev December 20, 2020 08:14

rsercano linked an issue Dec 20, 2020 that may be closed by this pull request

Add basic accounting capabilities #85

Open

rsercano force-pushed the feat/adding-sequelize branch from 41e0da6 to 2e533c1 Compare December 20, 2020 08:15

rsercano self-assigned this Dec 20, 2020

rsercano added db enhancement New feature or request labels Dec 20, 2020

rsercano force-pushed the feat/adding-sequelize branch from 2e533c1 to aa7efaf Compare December 20, 2020 08:45

ghost reviewed Dec 20, 2020

View reviewed changes

src/db/arby-order.ts Outdated Show resolved Hide resolved

src/db/arby-order.ts Outdated Show resolved Hide resolved

src/db/arby-order.ts Outdated Show resolved Hide resolved

ghost reviewed Dec 20, 2020

View reviewed changes

src/db/arby-order.ts Outdated Show resolved Hide resolved

rsercano force-pushed the feat/adding-sequelize branch from aa7efaf to ab1d7d6 Compare December 23, 2020 08:35

rsercano force-pushed the feat/adding-sequelize branch from ab1d7d6 to c76974c Compare December 23, 2020 08:38

rsercano requested a review from a user December 23, 2020 08:40

rsercano changed the title ~~feat: adding sequelize along with new db type 'ArbyOrder' (#85)~~ feat: adding sequelize along with new db type 'CexOrder' (#85) Dec 23, 2020

ghost reviewed Dec 23, 2020

View reviewed changes

src/db/db.ts Show resolved Hide resolved

src/db/db.ts Outdated Show resolved Hide resolved

src/db/cex-fee.ts Outdated Show resolved Hide resolved

feat: adding sequelize along with new db type 'CexOrder' (#85)

079a754

rsercano force-pushed the feat/adding-sequelize branch 2 times, most recently from feba5c2 to 1fad13a Compare December 25, 2020 06:18

rsercano changed the title ~~feat: adding sequelize along with new db type 'CexOrder' (#85)~~ feat: adding sequelize along with new db type 'Order' (#85) Dec 25, 2020

rebased

56655ec

rsercano force-pushed the feat/adding-sequelize branch from 1fad13a to 56655ec Compare December 25, 2020 07:23

saveOrder$ added to executeOrder of cex

c98a140

rsercano requested a review from a user December 25, 2020 09:39

rsercano added 2 commits December 27, 2020 11:08

added graceful shutdown for db

0f910c3

fixed tests

950c965

ghost reviewed Dec 29, 2020

View reviewed changes

src/db/order-repository.ts Outdated Show resolved Hide resolved

src/db/db.ts Show resolved Hide resolved

removed order repository

86d25ea

rsercano requested a review from a user January 2, 2021 10:15

moved db close operation to start subscribe and fixed saveOrder

9a77ed3

rsercano force-pushed the feat/adding-sequelize branch from 75f0af7 to 9a77ed3 Compare January 2, 2021 10:16

ghost suggested changes Jan 4, 2021

View reviewed changes

fixed reviews

6b16997

rsercano mentioned this pull request Jan 9, 2021

Adding http to server to serve saved CEX orders #133

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding sequelize along with new db type 'Order' (#85) #125

feat: adding sequelize along with new db type 'Order' (#85) #125

rsercano commented Dec 20, 2020 •

edited

Loading

ghost left a comment

rsercano commented Dec 20, 2020

ghost left a comment

rsercano commented Dec 20, 2020

ghost commented Dec 20, 2020

rsercano commented Dec 20, 2020 •

edited

Loading

rsercano commented Dec 21, 2020 •

edited

Loading

rsercano commented Dec 23, 2020 •

edited

Loading

ghost left a comment

rsercano commented Dec 25, 2020

rsercano commented Dec 25, 2020

ghost commented Dec 29, 2020

rsercano commented Dec 29, 2020

rsercano commented Jan 2, 2021 •

edited

Loading

rsercano commented Jan 4, 2021

ghost commented Jan 4, 2021

rsercano commented Jan 4, 2021

ghost Jan 4, 2021

rsercano Jan 9, 2021

rsercano Jan 9, 2021

ghost Jan 4, 2021

rsercano Jan 9, 2021

feat: adding sequelize along with new db type 'Order' (#85) #125

Are you sure you want to change the base?

feat: adding sequelize along with new db type 'Order' (#85) #125

Conversation

rsercano commented Dec 20, 2020 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

rsercano commented Dec 20, 2020

ghost left a comment

Choose a reason for hiding this comment

rsercano commented Dec 20, 2020

ghost commented Dec 20, 2020

rsercano commented Dec 20, 2020 • edited Loading

rsercano commented Dec 21, 2020 • edited Loading

rsercano commented Dec 23, 2020 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

rsercano commented Dec 25, 2020

rsercano commented Dec 25, 2020

ghost commented Dec 29, 2020

rsercano commented Dec 29, 2020

rsercano commented Jan 2, 2021 • edited Loading

rsercano commented Jan 4, 2021

ghost commented Jan 4, 2021

rsercano commented Jan 4, 2021

ghost Jan 4, 2021

Choose a reason for hiding this comment

rsercano Jan 9, 2021

Choose a reason for hiding this comment

rsercano Jan 9, 2021

Choose a reason for hiding this comment

ghost Jan 4, 2021

Choose a reason for hiding this comment

rsercano Jan 9, 2021

Choose a reason for hiding this comment

rsercano commented Dec 20, 2020 •

edited

Loading

rsercano commented Dec 20, 2020 •

edited

Loading

rsercano commented Dec 21, 2020 •

edited

Loading

rsercano commented Dec 23, 2020 •

edited

Loading

rsercano commented Jan 2, 2021 •

edited

Loading