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/bamboo relay coordinator #500

Merged
merged 44 commits into from Jul 30, 2019

Conversation

@Arctek
Copy link
Contributor

commented Jul 4, 2019

Added support for the coordinator to the Bamboo Relay connector.

Some points:

  • The connector supports both coordinated and uncoordinated modes, by a configuration toggle
  • The connector has a pre-emptive cancel function (and config toggle) when using the coordinated version, as orders with < 30 seconds expiry aren't shown in the UI on Bamboo Relay
  • Added in flight cancels list
  • When placing limit orders its possible for the strategy to attempt to cancel them before the API has responded, so I added a in flight pending limit orders dictionary to address this
  • Placing market orders now uses fillOrder or batchFillOrder calls on the smart contracts, as this is better for gas consumption
  • Added an API failure timeout (3 seconds), to prevent the strategy from spamming the server endpoints on every tick
  • Added support for the testnets to the connector (as needed this for testing anyway), but I had to modify a few other spots in the hummingbot code to do this (erc20_events_watcher.py/erc20_token.py)

For testing I've been running the integration tests on ropsten, and placing buy/sell DAI orders on the book (so something is sitting there).

Anyway this is a big PR, let me know what you think/I should change.

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Can one of the admins verify this patch?

@carlolm

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

ok to test

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/145/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/162/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/166/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/179/


@classmethod
def http_client(cls) -> aiohttp.ClientSession:
if cls._client is None:
def http_client(self) -> aiohttp.ClientSession:

This comment has been minimized.

Copy link
@chowryan

chowryan Jul 9, 2019

Contributor

our convention is to use cls for @classmethod


@classmethod
async def get_all_token_info(cls) -> Dict[str, any]:
async def get_all_token_info(self) -> Dict[str, any]:

This comment has been minimized.

Copy link
@chowryan

chowryan Jul 9, 2019

Contributor

our convention is to use cls for @classmethod

@@ -110,8 +125,14 @@ def order_book_class(self) -> BambooRelayOrderBook:
return BambooRelayOrderBook

@staticmethod
async def get_snapshot(client: aiohttp.ClientSession, trading_pair: str) -> Dict[str, any]:
async with client.get(f"{REST_BASE_URL}/markets/{trading_pair}/book") as response:
async def get_snapshot(client: aiohttp.ClientSession,

This comment has been minimized.

Copy link
@chowryan

chowryan Jul 9, 2019

Contributor

static methods should not require self. pass in the api_prefix from the function that calls this function

@@ -134,7 +155,7 @@ def order_book_class(self) -> BambooRelayOrderBook:

for trading_pair in trading_pairs:
try:
snapshot: Dict[str, any] = await self.get_snapshot(client, trading_pair)
snapshot: Dict[str, any] = await self.get_snapshot(client, trading_pair, self)

This comment has been minimized.

Copy link
@chowryan

chowryan Jul 9, 2019

Contributor

pass in api_prefix instead of self here


def __init__(self,
wallet: Web3Wallet,
ethereum_rpc_url: str,
chain: EthereumChain = EthereumChain.MAIN_NET,

This comment has been minimized.

Copy link
@chowryan

chowryan Jul 9, 2019

Contributor

the initialized wallet instance knows which chain its on. check line 147 on web3_wallet_backend.py.

This comment has been minimized.

Copy link
@Arctek

Arctek Jul 10, 2019

Author Contributor

I exposed it on Web3Wallet as a property, given that makes the most sense?
Else it has to be wallet.current_backend.chain - which should be the same value anyway?

Changes based on PR feedback: cls in class methods, removing self in …
…static methods, removed the chain being passed in neeedlessly
@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/211/

@carlolm

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

This had a compile error:

Error compiling Cython file:
------------------------------------------------------------
...
        self._approval_tx_polling_task = None
        self._wallet = wallet
        self._use_coordinator = use_coordinator
        self._pre_emptive_soft_cancels = pre_emptive_soft_cancels
        self._latest_salt = -1
        if wallet.chain is EthereumChain.MAIN_NET:
                          ^
------------------------------------------------------------

hummingbot/market/bamboo_relay/bamboo_relay_market.pyx:345:27: undeclared name not builtin: EthereumChain
Traceback (most recent call last):
  File "/root/miniconda/envs/hummingbot/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1244, in cythonize_one_helper
    return cythonize_one(*m)
  File "/root/miniconda/envs/hummingbot/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1220, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: hummingbot/market/bamboo_relay/bamboo_relay_market.pyx
multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/root/miniconda/envs/hummingbot/lib/python3.6/multiprocessing/pool.py", line 119, in worker
    result = (True, func(*args, **kwds))
  File "/root/miniconda/envs/hummingbot/lib/python3.6/multiprocessing/pool.py", line 44, in mapstar
    return list(map(*args))
  File "/root/miniconda/envs/hummingbot/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1244, in cythonize_one_helper
    return cythonize_one(*m)
  File "/root/miniconda/envs/hummingbot/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1220, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: hummingbot/market/bamboo_relay/bamboo_relay_market.pyx
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "setup.py", line 143, in <module>
    main()
  File "setup.py", line 132, in main
    ], language="c++", language_level=3, nthreads=cpu_count),
  File "/root/miniconda/envs/hummingbot/lib/python3.6/site-packages/Cython/Build/Dependencies.py", line 1088, in cythonize
    result.get(99999)  # seconds
  File "/root/miniconda/envs/hummingbot/lib/python3.6/multiprocessing/pool.py", line 670, in get
    raise self._value
Cython.Compiler.Errors.CompileError: hummingbot/market/bamboo_relay/bamboo_relay_market.pyx
@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/212/

@Arctek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Ah sorry, I forget that commits to a PR will automatically re-run tests.
I'll fix it up locally first.

@Arctek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Hey Mike,

  1. This is because of the tracking states json that's stored in the database - as that field is new. i.e. if you delete the database under the data dir it will run (hummingbot_trades.sqlite).
    When users do a hummingbot upgrade is this database re-created?
    Or should I add error handling in the order restoration code, i.e. from_json?
    This will be an issue in future too I think if any connector goes and adds new fields that wont exist on trades from a previous version.

  2. Just added this in the docs.

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/368/

@Arctek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Ok - it should work now with that older database gracefully and just convert missing/changed fields.

In future there will probably need to be SQL migrations to update saved InFlightOrders and add/update fields, to prevent it becoming too messy.

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/369/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/371/

@mifeng

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Tested all 4 strategies and it appears to work well. I've approved but I think someone from engineering should review the code before we merge.

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/379/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/381/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/409/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/411/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/415/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/420/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/434/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/447/

@jenkins-coinalpha

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins-hb.coinalpha.com/ghprb/job/hb-test_pull-request/454/

@mifeng

mifeng approved these changes Jul 30, 2019

Copy link
Member

left a comment

Tested and LGTM

@mifeng mifeng merged commit 3652ba7 into CoinAlpha:development Jul 30, 2019

1 check passed

ci/jenkins: stable-tests Your tests passed on Jenkins! No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.