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/Create Cross-Exchange MM configuration schema #240

Merged
merged 34 commits into from
May 25, 2022

Conversation

mhrvth
Copy link

@mhrvth mhrvth commented May 18, 2022

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

Adds a Maker-Taker Base Config Map class
Defines a config map schema for the XEMM strategy
The XEMM strategy takes configuration directly from a config map

Tests performed by the developer:

Ran the unit tests

@mhrvth mhrvth requested review from petioptrv and a team May 18, 2022 19:58
@mhrvth mhrvth self-assigned this May 18, 2022
@mhrvth
Copy link
Author

mhrvth commented May 18, 2022

[sc-26285]

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #26285: Create Cross-Exchange MM configuration schema.

@RC-13
Copy link

RC-13 commented May 19, 2022

Imported a strategy that was created from another branch and got the following error while trying to configure some parameters, the parameter values were removed due to the change in the parameter name.
Note: despite getting the error the strategy still starts.
xemm-import-validation-config-map-failed

2022-05-19 07:28:30,200 - 5976 - root - ERROR - Error writing configs: 'CrossExchangeMarketMakingConfigMap' object has no attribute 'trading_pair_taker'
Traceback (most recent call last):
  File "/Users/clintg/testing/xemm/hummingbot/client/config/config_helpers.py", line 614, in save_to_yml
    cm_yml_str = cm.generate_yml_output_str_with_comments()
  File "/Users/clintg/testing/xemm/hummingbot/client/config/config_helpers.py", line 193, in generate_yml_output_str_with_comments
    original_fragments = yaml.safe_dump(self._dict_in_conf_order(), sort_keys=False).split("\n")
  File "/Users/clintg/testing/xemm/hummingbot/client/config/config_helpers.py", line 224, in _dict_in_conf_order
    value = getattr(self, attr)
  File "/Users/clintg/testing/xemm/hummingbot/client/config/config_helpers.py", line 98, in __getattr__
    value = getattr(self._hb_config, item)
AttributeError: 'CrossExchangeMarketMakingConfigMap' object has no attribute 'trading_pair_taker'
Screen.Recording.2022-05-19.at.7.28.22.AM.mov

@mhrvth
Copy link
Author

mhrvth commented May 19, 2022

@RC-13 Hi Clinton, I have reverted the variable names, so you should be able to load an old config now.

@petioptrv
Copy link

Imported a strategy that was created from another branch and got the following error while trying to configure some

For this, @mhrvth, you can rebase onto this PR, and add a migration step for this strategy in hummingbot/client/config/conf_migration.py

@gerald-hb
Copy link

gerald-hb commented May 19, 2022

latest changes looks good to me. I no longer encounter issue with importing and editing strategies that came from a different branch. I can also run them without any issues.

Test Summary
1.Clone and install featured branch:white_check_mark:
2.Create cross Exchange strategy:white_check_mark:
3.Use oracle rate with different base/quote asset from maker and taker pair:white_check_mark:
4.Status check:white_check_mark:
5.Getting filled orders in order ✅
6.History shows filled trade details with correct trading Fee:white_check_mark:
7.Also test in Paper trade and works ✅

mhrvth and others added 4 commits May 19, 2022 14:06
Co-authored-by: Petio Petrov <petioptrv@icloud.com>
Co-authored-by: Petio Petrov <petioptrv@icloud.com>
…e_market_making.pyx

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
@PtrckM
Copy link

PtrckM commented May 20, 2022

Test Update:

  • running multiple bots overnight for monitoring purposes.

@mhrvth mhrvth requested a review from petioptrv May 20, 2022 16:51
@gerald-hb
Copy link

Latest commits looks good to me. No notable error found.

Test summary
-Clone and install featured branch
-Create cross Exchange strategy and tested new parameters
-Order refresh mode
-active_order_refresh: ok
-passive_order_refresh: ok
-what is the threshold of profitability to cancel a trade: ok
-how often limit orders to expire: ok
-Convertion rate mode
-rate oracle source: ok
-fixed convertion: ok
-Orders are getting cancelled/refreshed accordingly
-filled orders are working in order (maker(buy/sell)-taker (alternate)
-Paper trading: ok

Looking forward to test other requested changes.

mhrvth and others added 8 commits May 23, 2022 13:48
…e_market_making.pyx

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
…e_market_making_config_map_pydantic.py

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
…e_market_making_config_map_pydantic.py

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
…e_market_making_config_map_pydantic.py

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
…e_market_making_config_map_pydantic.py

Co-authored-by: Petio Petrov <petioptrv@icloud.com>
@mhrvth mhrvth requested a review from petioptrv May 24, 2022 06:18
@petioptrv
Copy link

Finally, if we stick to all code being in conf_migration.py, then the CI failure on code coverage will be solved. Otherwise, please create a folder hummingbot/client/config/conf_migration, and edit .coveragerc:7 from

        hummingbot/client/config/conf_migration.py

to

        hummingbot/client/config/conf_migration/*

hummingbot/client/config/conf_migration.py Outdated Show resolved Hide resolved
hummingbot/client/config/conf_migration.py Show resolved Hide resolved
hummingbot/client/config/conf_migration.py Outdated Show resolved Hide resolved
mhrvth and others added 2 commits May 24, 2022 18:09
Co-authored-by: Petio Petrov <petioptrv@icloud.com>
Copy link

@petioptrv petioptrv left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Copy link

@Deepali6997 Deepali6997 left a comment

Choose a reason for hiding this comment

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

changes LGTM

Test Summary

  • Cloned and install feature branch
  • Migrating strategy from dev branch : ok
  • Running import command: strategy can be seen : ok
  • Import paper trade strategy and live strategy : ok
  • Migration successful : ok

quick re-test of following

  • Create cross Exchange strategy
  • Status : ok
  • Status —live : ok
  • Order_refresh_mode
  • active_order_refresh
  • passive_order_refresh
  • what is the threshold of profitability to cancel a trade
  • how often limit orders to expire
  • Conversion rate mode
  • rate_oracle_conversion_rate: fetches from the exchange
  • fixed_conversion_rate: given by the user (taker to maker base and taker to maker quote)
  • working on paper trade too

@mhrvth mhrvth merged commit edd7860 into epic/config_management_refactoring May 25, 2022
@mhrvth mhrvth deleted the feat/xemm_config_map branch May 25, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants