-
Notifications
You must be signed in to change notification settings - Fork 20
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: adding sequelize along with new db type 'Order' (#85) #125
base: master
Are you sure you want to change the base?
Conversation
41e0da6
to
2e533c1
Compare
2e533c1
to
aa7efaf
Compare
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.
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:
And for profit calculation it just should be a simple minus operation, doesn't it? @kilrau @erkarl |
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 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? |
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) |
After the call with @erkarl and @kilrau we'll save the order as is with linked |
aa7efaf
to
ab1d7d6
Compare
As an outcome to our discussion,
Just as in the ccxt. Next step for this PR:
|
ab1d7d6
to
c76974c
Compare
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.
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.
feba5c2
to
1fad13a
Compare
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. |
1fad13a
to
56655ec
Compare
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. |
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. |
75f0af7
to
9a77ed3
Compare
fyi. @erkarl I'll create a new branch over this one and will start HTTP implementation |
src/arby.ts
Outdated
}).subscribe({ | ||
error: error => { | ||
if (error.message) { | ||
console.log(`Error: ${error.message}`); | ||
} else { | ||
console.log(error); | ||
} | ||
process.exit(1); | ||
closeDB$().subscribe({ |
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.
Whenever you need to subscribe within a subscribe callback you should probably be using a transformation operation instead (such as mergeMap
).
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 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
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.
Line that I suspect is not working: https://github.com/ExchangeUnion/market-maker-bot/pull/125/files#diff-77a45f12e136f324d3c28fe24cdf3a531c7582103e96498a48462eaf7414a509R181
|
||
const arbyOrder = convertToArbyOrder(order); | ||
const arbyTrades = convertToArbyTrades(order.trades, order.id); | ||
// TODO async |
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.
TODO?
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 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?
Hello,
this PR implements the following changes:
ArbyOrder
.env-example
(TEST_MODE
instead ofLIVE_CEX
)arby.ts
db
and used for sequelizeNext steps:
ArbyOrder
model inmergeMap
'sproject
here: and giving it toinitCEX$
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)