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

feat(launcher): arby integration #495

Merged
merged 12 commits into from
Jun 16, 2020
Merged

feat(launcher): arby integration #495

merged 12 commits into from
Jun 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jun 2, 2020

How to use:
bash xud.sh -b feat/arby

Down the environment:
down

Edit ~/.xud-docker/simnet/simnet.conf:

[arby]
binance-api-key = "123"
binance-api-secret = "321"
margin = "0.04"

Start the environment again and restart arby.
docker logs -f simnet_arby_1

Arby should now create buy/sell orders.

Known issues:

  • add arby disabled = false/true to $DOCKER_DIR/$NETWORK/$NETWORK.conf - will be done in a separate PR by @reliveyy
  • do not require restart arby when starting the environment - will be solved by v2
  • only ETH/BTC orders are added at the moment - will be solved by v2

@kilrau kilrau requested review from kilrau and reliveyy June 2, 2020 13:58
@reliveyy reliveyy changed the title Add arby support feat(launcher): arby integration Jun 2, 2020
@ghost ghost force-pushed the feat/arby branch 2 times, most recently from 06ceec3 to 50922b6 Compare June 5, 2020 15:05
@ghost ghost marked this pull request as ready for review June 5, 2020 15:06
@@ -396,6 +396,25 @@ def update_raiden(self, parsed):
node = self.nodes["raiden"]
self.update_ports(node, parsed)

def update_arby(self, parsed):
"""Update arby related configurations from parsed TOML arby section
:param parsed: Parsed xud TOML section
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xud -> arby

@kilrau kilrau requested a review from raladev June 8, 2020 08:17
@kilrau kilrau added the P1 top priority label Jun 8, 2020
@kilrau kilrau assigned ghost Jun 8, 2020
@kilrau
Copy link
Contributor

kilrau commented Jun 8, 2020

This is ready for testing by us @raladev

To be tested:

  • arby starts/stops
  • connects to binance

Not to be tested for now:

  • actual arbitrage/trading behavior, v2 will change quite a bunch of that. Do you think it's realistic upgrading to v2 latest ~Wednesdayish? @erkarl

@kilrau
Copy link
Contributor

kilrau commented Jun 8, 2020

add arby disabled = false/true to $DOCKER_DIR/$NETWORK/$NETWORK.conf - will be done in a separate PR by @reliveyy

Looks like this is still missing. Are you throwing up that PR? @reliveyy

@kilrau
Copy link
Contributor

kilrau commented Jun 8, 2020

Also as of latest master it's now: sudo cp sample-simnet.conf simnet.conf, then edit simnet.conf

@reliveyy
Copy link
Collaborator

reliveyy commented Jun 9, 2020

Are you throwing up that PR?

@kilrau #507

@kilrau
Copy link
Contributor

kilrau commented Jun 9, 2020

While I like the personal note, maybe simply Ready here to be in line with the other status fields?

simnet > status
┌─────────┬──────────────────────────────────────────────────┐
│ SERVICE │ STATUS                                           │
├─────────┼──────────────────────────────────────────────────┤
│ lndbtc  │ Ready                                            │
├─────────┼──────────────────────────────────────────────────┤
│ lndltc  │ Ready                                            │
├─────────┼──────────────────────────────────────────────────┤
│ connext │ Ready                                            │
├─────────┼──────────────────────────────────────────────────┤
│ arby    │ Hello, Arby.                                     │
├─────────┼──────────────────────────────────────────────────┤
│ xud     │ Ready                                            │
└─────────┴──────────────────────────────────────────────────┘

While status reports fine and container is up, arby logs show it exited, not sure why but I am sure v2 will solve this:

simnet > logs arby
2020-06-09 12:01:07,948 INFO Set uid to user 0 succeeded
2020-06-09 12:01:07,955 INFO supervisord started with pid 1
2020-06-09 12:01:08,958 INFO spawned: 'arby' with pid 7
2020-06-09 12:01:09,962 INFO success: arby entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

> [email protected] start:arby /app
> npm run compile && node dist/arby.js


> [email protected] compile /app
> npm run lint && tsc && cross-os postcompile


> [email protected] lint /app
> eslint . --ext .js,.jsx,.ts,.tsx

2020-06-09 12:01:23.2323 [GLOBAL] info: Starting. Hello, Arby.
2020-06-09 12:01:23.2323 [TradeManager] info: starting broker manager...
Arby boot sequence complete.
2020-06-09 12:01:23,894 INFO exited: arby (exit status 0; expected)

@raladev
Copy link
Contributor

raladev commented Jun 10, 2020

im not tested it fully, but it seems:

  • status command output should be standardized - change 'Hello, its arby' to Ready + add status about exited container.
  • if invalid creds are passed, status command should contain message about that, also logs of arby should contain error about bad creds and maybe container should finish work with another exit code (not 0).

@raladev
Copy link
Contributor

raladev commented Jun 12, 2020

Notes from first run session:

  • My xud node was locked after installation, but arby container printed "OpenDEX orders updated". Also because of locked node, arby said that my balance is 0 for both currencies. It seems like lock condition should be added.
    Screenshot from 2020-06-12 19-26-06
    UPD: should be retested, not reproduced during next installation attempt.

  • 'OpenDex orders updated' is not informative. I want to know how much orders are cancelled/added, new price, new qty, pair. It seems it would be better change update to cancel/add commands.
    Im tried to find them using stream orders but timestamp and data in logs matching is unreal.
    Screenshot from 2020-06-12 18-06-16
    IMO it is MUST HAVE feature for incident investigation. This tool plays with money and i want to clearly understand each step of its behavior.

  • Same for Binance order that was sent + also i want to see info about binance fill.

  • Each trade leads to price stream closing and reopening. idk, maybe it is legit behaviour, but does not look so.

  • Manual trade for another pair (not ETH/BTC) leads to order sending to binance.
    Screenshot from 2020-06-12 18-22-39

  • I manually closed BTC channel, but one of my orders is still staying in the orderbook.
    Screenshot from 2020-06-12 18-27-11

  • Maybe it would be better to do not print same price twice or move printing all prices to lower debug level.

  • bestBid and bestAsk monitoring looks like more correct solution then last trade monitoring, but for now trade monitoring is enough.

  • Current behavior can be destructive if price will move significantly after binance order placement, if I understood correctly. OpenDex update was stopped by placed but not filled order, so xud node will contain orders with tasty prices.

- is it true that each Opendex trade leads to order canceling untill Binance fill?
- - I haven't implemented this logic yet. Right now it just stops updating orders until Binance order is in progress. It should probably keep updating them or remove all orders until centralized exchange part is done.
  • Performance of xud node should be tested more accurate with 3 runned xud-arby envs

@michael1011 michael1011 mentioned this pull request Jun 14, 2020
3 tasks
Copy link
Contributor

@kilrau kilrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's fix the order update interval to 5s, at the moment orders are getting replaced that fast that our matching routine can't keep up:
simnet > buy 1 eth/btc mkt
matched 1 ETH @ 0.024507483 with peer ConsiderMoral order 039a7cc0-aef8-11ea-9144-37681cbd4116, attempting swap...
failed to swap 1 ETH due to OrderNotFound with peer order 039a7cc0-aef8-11ea-9144-37681cbd4116, continuing with matching routine...
matched 1 ETH @ 0.024505481 with peer ConsiderMoral order 03c3fdc0-aef8-11ea-9144-37681cbd4116, attempting swap...
failed to swap 1 ETH due to UnknownError with peer order 03c3fdc0-aef8-11ea-9144-37681cbd4116, continuing with matching routine...
matched 1 ETH @ 0.024503479 with peer ConsiderMoral order 087ec6b0-aef8-11ea-9144-37681cbd4116, attempting swap...
successfully swapped 1 ETH with peer order 087ec6b0-aef8-11ea-9144-37681cbd4116
  • Another thing: you are always using replace_order_id when issuing new orders, right? I am confused by local xud/arby logs saying removed order and creating new orders, but but there were no remova p2p packets. In that case, I'd remove this arby log statement since it's confusing: [OpenDEX] trace: Removed all open orders for ETH/BTC

@kilrau
Copy link
Contributor

kilrau commented Jun 15, 2020

On testnet this is not connecting to me xud, could you check what changes are needed to make arby work with the testnet setup (xud port 18885 etc) @reliveyy

And please give it a rebase while you are at it.

Karl Ranna and others added 7 commits June 16, 2020 00:13
* add disabled option

* add disabled in template simnet.conf

* fix config option missing

* fix container missing when disabled

* add disabled container status

* comment out simnet.conf arby options

* fix syntax

* add arby to testnet and mainnet too
@reliveyy
Copy link
Collaborator

In order to make arby disable/enable more semantic, I add a new option --arby to enable arby and keep the old option --arby.disabled as it is to disable arby. @kilrau @raladev

@kilrau
Copy link
Contributor

kilrau commented Jun 15, 2020

I add a new option --arby to enable arby

arby will be enabled on default once it's stable. Disabling by default now is only to make things controllable since arby is in alpha stage. So please no --arby option. You can also leave the sample-conf value with disabled = false.

@reliveyy reliveyy requested review from reliveyy and kilrau and removed request for reliveyy June 15, 2020 17:57
Copy link
Contributor

@kilrau kilrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general: works

Tested on simnet & testnet with xud -b feat/arby --arby.disabled=false -> worked.

Ran with

[arby]
binance-api-key = "123"
binance-api-secret = "321"
margin = "0.04"
disabled = false

-> arby was removed:

- Container testnet_arby_1: disabled
A new version is available. Would you like to upgrade (Warning: this may restart your environment and cancel all open orders)? [Y/n] 
Removing testnet_arby_1...

So something is wrong in how we interpret the .conf file [arby] disabled option.

@kilrau kilrau self-requested a review June 15, 2020 20:21
Copy link
Contributor

@kilrau kilrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again, works!

@raladev
Copy link
Contributor

raladev commented Jun 16, 2020

  • bash xud.sh -b feat/arby --arby.binance-api-key abc --arby.binance-api-secret 123 --arby.margin 0.001 does not work for now, it seems like arby is disabled always.

@kilrau
Copy link
Contributor

kilrau commented Jun 16, 2020

That is intended behavior @raladev as we discussed above we default to arby.disabled = true until arby is stable. So you will need to run xud.sh -b feat/arby --arby.disabled false --arby.binance-api-key abc --arby.binance-api-secret 123 --arby.margin 0.001 or this network config:

[arby]
binance-api-key = "123"
binance-api-secret = "321"
margin = "0.04"
disabled = false

@raladev
Copy link
Contributor

raladev commented Jun 16, 2020

That is intended behavior @raladev as we discussed above we default to arby.disabled = true until arby is stable. So you will need to run xud.sh -b feat/arby --arby.disabled false --arby.binance-api-key abc --arby.binance-api-secret 123 --arby.margin 0.001 or this network config:

[arby]
binance-api-key = "123"
binance-api-secret = "321"
margin = "0.04"
disabled = false

It seems inconvenient for command line parameters, but understood.
For me expected result for cmd is: passing these 3 params makes arby enabled without enabling manually.

@raladev raladev merged commit ce229bc into master Jun 16, 2020
@raladev raladev deleted the feat/arby branch June 16, 2020 09:29
@kilrau
Copy link
Contributor

kilrau commented Jun 16, 2020

For me expected result for cmd is: passing these 3 params makes arby enabled without enabling manually.

Arby defaulting to disabled = true is temporary and after we change this, passing these 3 parameters (just like you did) indeed is enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 top priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants