Skip to content

What is the preferred way to add futures/swap support #536

Open

Description

@szmcdull

a) Add another virtual exchange that works only for futures, yet another one for swap.
b) Add a marketType parameter to all methods of the existing exchange.

The first one is closer to the native APIs provided by most exchanges.
The second one is more user friendly in my opinion.

Activity

  1. vslee commented on Apr 16, 2020

    @vslee
    Collaborator

    Are you talking about OKEx?

  2. szmcdull commented on Apr 16, 2020

    @szmcdull
    ContributorAuthor

    OKEx, Huobi, Binance, Deribit, KuCoin etc.

  3. vslee commented on Apr 16, 2020

    @vslee
    Collaborator

    For OKEx, can you determine whether it's futures / swap from the symbol name? I haven't looked into he others.

  4. szmcdull commented on Apr 20, 2020

    @szmcdull
    ContributorAuthor

    Maybe futures/swap can be determined by symbol name. But margin/spot share the same symbol names and depths.

  5. vslee commented on Apr 20, 2020

    @vslee
    Collaborator

    In that case, I think option (b) would be better and less confusing.

  6. jjxtra commented on Apr 20, 2020

    @jjxtra
    Collaborator

    Which methods would need the parameter?

  7. vslee commented on Apr 22, 2020

    @vslee
    Collaborator

    Actually, a better option would be to create a property in ExchangeAPI:

    		protected bool isMarginTradingEnabled = false;
    
    		public virtual bool IsMarginTradingEnabled
    		{
    			get { return isMarginTradingEnabled; }
    			set { throw new NotImplementedException(
    				"Margin trading is not implemented for this exchange. Please submit a PR if you would like to implement it."); }
    		}

    Any exchanges that do implement it can override the property to set the field isMarginTradingEnabled. This way, no new classes need to be created, and no method signatures need to be changed.

  8. jjxtra commented on Apr 22, 2020

    @jjxtra
    Collaborator

    Would returning false be better, that way people don't have unexpected exceptions? I suppose if we document the setter with a NotImplementedException that may be sufficient?

  9. vslee commented on Apr 22, 2020

    @vslee
    Collaborator

    isMarginTradingEnabled is by default set to false (I updated the code above to make it clearer). I think it is better to throw an execption, than to have ppl set it to true and thinking that they are using margin trading when they are not. This would be set in the initialization part of ppl's code anyways, so it's not like it would interrupt their trading in the middle of the day.

  10. vslee commented on Apr 22, 2020

    @vslee
    Collaborator

    Also, the exception would only be thrown if someone tries to use margin trading where not implemented. All existing user code would still work fine w/o any changes.

  11. jjxtra commented on Apr 22, 2020

    @jjxtra
    Collaborator

    What would a derived class look like trying to set the bool to true? Would it need it's own bool?

  12. vslee commented on Apr 22, 2020

    @vslee
    Collaborator

    The derived class would have this:

    		public override bool IsMarginTradingEnabled
    		{
    			get { return isMarginTradingEnabled; }
    			set { isMarginTradingEnabled = value; }
    		}
  13. jjxtra commented on Apr 22, 2020

    @jjxtra
    Collaborator

    I would propose the following, where derived classes opt in and override IsMarginTradingPossible to true if desired.

    ///<summary>Indicates if margin trading is possible. Derived classes can override and return true. The default is false.</summary>
    public virtual bool IsMarginTradingPossible => false;
    
    private bool isMarginTradingEnabled = false;
    
    ///<summary>Gets whether margin trading has been turned on. If IsMarginTradingPossible is false, any attempt to set this value will throw a NotImplementedException.</summary>
    public bool IsMarginTradingEnabled
    {
        get { return isMarginTradingEnabled; }
        set
        {
            if (!IsMarginTradingPossible)
            {
                throw new NotImplementedException(
    	    "Margin trading is not implemented for this exchange. Please submit a PR if you would like to implement it, derived classes can override IsMarginTradingPossible to return true .");
            }
            isMarginTradingEnabled = value;
        }
    }
  14. vslee commented on Apr 22, 2020

    @vslee
    Collaborator

    Yes, I like that

  15. jjxtra commented on Apr 22, 2020

    @jjxtra
    Collaborator

    Awesome, sounds like a plan for whoever tackles this beast :)

  16. szmcdull commented on Apr 24, 2020

    @szmcdull
    ContributorAuthor

    How is isMarginEnabled related to futures/swap?

  17. vslee commented on Apr 24, 2020

    @vslee
    Collaborator

    isMarginEnabled is just for whether the user wants to use margin vs spot. Since the symbol names for futures/swap are different, those can be added to the existing methods without adding any parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions