Bugfix: Submit/Cancel order scripting errors + Ordermanager GetOrder & CancelOrder improvements (#584)

* Fixes Submit and Cancel order functions for scripting. Adds tests to ensure that wrapper functions actually execute as expected. Adds more coverage. Fixes a few other tests

* Fixes issues

* Simplifies tests

* Update cancel order to properly cancel an order. Adds example script. Adds sweet tests.

* Fixes bad test setup for cancelling orders

* Adds new order manager GetOrderInfo function to call the wrapper function and then store it in the order manager

* Addresses order concerns

* LOWERS THE CASE OF ALL EXCHANGE NAMES BECAUSE ORDER MANAGER NEVER CARED ABOUT CASING OR YOUR FAMILY

* Removes asset and currency requirement from cancelling orders via validation test

* Fixes old cancel order assumptions that had requirements for currency and asset

* Moves all the logs and events to the dedicated Cancel function instead of exclusively doing it in CancelAllOrders

* Adds more detail logging. Fixes lbank fee test

* Addresses comms manager issues

* Removes go routine for pushing events. Better to let them fail

engine/comms_relayer.go

@@ -72,7 +72,11 @@ func (c *commsManager) PushEvent(evt base.Event) {
 	if !c.Started() {
 		return
 	}
-	c.relayMsg <- evt
+	select {
+	case c.relayMsg <- evt:
+	default:
+		log.Errorf(log.CommunicationMgr, "Failed to send, no receiver when pushing event [%v]", evt)
+	}
 }
 
 func (c *commsManager) run() {

engine/fake_exchange_test.go

@@ -133,7 +133,10 @@ func (h *FakePassingExchange) CancelAllOrders(_ *order.Cancel) (order.CancelAllR
 	return order.CancelAllResponse{}, nil
 }
 func (h *FakePassingExchange) GetOrderInfo(_ string, _ currency.Pair, _ asset.Item) (order.Detail, error) {
-	return order.Detail{}, nil
+	return order.Detail{
+		Exchange: fakePassExchange,
+		ID:       "fakeOrder",
+	}, nil
 }
 func (h *FakePassingExchange) GetDepositAddress(_ currency.Code, _ string) (string, error) {
 	return "", nil

engine/orders.go

@@ -3,12 +3,15 @@ package engine
 import (
 	"errors"
 	"fmt"
+	"strings"
 	"sync/atomic"
 	"time"
 
 	"github.com/gofrs/uuid"
 	"github.com/thrasher-corp/gocryptotrader/common"
 	"github.com/thrasher-corp/gocryptotrader/communications/base"
+	"github.com/thrasher-corp/gocryptotrader/currency"
+	"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
 	"github.com/thrasher-corp/gocryptotrader/exchanges/order"
 	"github.com/thrasher-corp/gocryptotrader/log"
 )
@@ -33,7 +36,7 @@ func (o *orderStore) get() map[string][]*order.Detail {
 func (o *orderStore) GetByExchangeAndID(exchange, id string) (*order.Detail, error) {
 	o.m.RLock()
 	defer o.m.RUnlock()
-	r, ok := o.Orders[exchange]
+	r, ok := o.Orders[strings.ToLower(exchange)]
 	if !ok {
 		return nil, ErrExchangeNotFound
 	}
@@ -50,7 +53,7 @@ func (o *orderStore) GetByExchangeAndID(exchange, id string) (*order.Detail, err
 func (o *orderStore) GetByExchange(exchange string) ([]*order.Detail, error) {
 	o.m.RLock()
 	defer o.m.RUnlock()
-	r, ok := o.Orders[exchange]
+	r, ok := o.Orders[strings.ToLower(exchange)]
 	if !ok {
 		return nil, ErrExchangeNotFound
 	}
@@ -78,7 +81,7 @@ func (o *orderStore) exists(det *order.Detail) bool {
 	}
 	o.m.RLock()
 	defer o.m.RUnlock()
-	r, ok := o.Orders[det.Exchange]
+	r, ok := o.Orders[strings.ToLower(det.Exchange)]
 	if !ok {
 		return false
 	}
@@ -116,9 +119,9 @@ func (o *orderStore) Add(det *order.Detail) error {
 	}
 	o.m.Lock()
 	defer o.m.Unlock()
-	orders := o.Orders[det.Exchange]
+	orders := o.Orders[strings.ToLower(det.Exchange)]
 	orders = append(orders, det)
-	o.Orders[det.Exchange] = orders
+	o.Orders[strings.ToLower(det.Exchange)] = orders
 
 	return nil
 }
@@ -203,8 +206,6 @@ func (o *orderManager) CancelAllOrders(exchangeNames []string) {
 		}
 
 		for y := range v {
-			log.Debugf(log.OrderMgr, "Order manager: Cancelling order ID %v [%v]",
-				v[y].ID, v[y])
 			err := o.Cancel(&order.Cancel{
 				Exchange:      k,
 				ID:            v[y].ID,
@@ -218,20 +219,8 @@ func (o *orderManager) CancelAllOrders(exchangeNames []string) {
 			})
 			if err != nil {
 				log.Error(log.OrderMgr, err)
-				Bot.CommsManager.PushEvent(base.Event{
-					Type:    "order",
-					Message: err.Error(),
-				})
 				continue
 			}
-
-			msg := fmt.Sprintf("Order manager: Exchange %s order ID=%v cancelled.",
-				k, v[y].ID)
-			log.Debugln(log.OrderMgr, msg)
-			Bot.CommsManager.PushEvent(base.Event{
-				Type:    "order",
-				Message: msg,
-			})
 		}
 	}
 }
@@ -239,41 +228,91 @@ func (o *orderManager) CancelAllOrders(exchangeNames []string) {
 // Cancel will find the order in the orderManager, send a cancel request
 // to the exchange and if successful, update the status of the order
 func (o *orderManager) Cancel(cancel *order.Cancel) error {
+	var err error
+	defer func() {
+		if err != nil {
+			Bot.CommsManager.PushEvent(base.Event{
+				Type:    "order",
+				Message: err.Error(),
+			})
+		}
+	}()
+
 	if cancel == nil {
-		return errors.New("order cancel param is nil")
+		err = errors.New("order cancel param is nil")
+		return err
 	}
-
 	if cancel.Exchange == "" {
-		return errors.New("order exchange name is empty")
+		err = errors.New("order exchange name is empty")
+		return err
 	}
-
 	if cancel.ID == "" {
-		return errors.New("order id is empty")
+		err = errors.New("order id is empty")
+		return err
 	}
 
 	exch := Bot.GetExchangeByName(cancel.Exchange)
 	if exch == nil {
-		return ErrExchangeNotFound
+		err = ErrExchangeNotFound
+		return err
 	}
 
 	if cancel.AssetType.String() != "" && !exch.GetAssetTypes().Contains(cancel.AssetType) {
-		return errors.New("order asset type not supported by exchange")
+		err = errors.New("order asset type not supported by exchange")
+		return err
 	}
 
-	err := exch.CancelOrder(cancel)
+	log.Debugf(log.OrderMgr, "Order manager: Cancelling order ID %v [%+v]",
+		cancel.ID, cancel)
+
+	err = exch.CancelOrder(cancel)
 	if err != nil {
-		return fmt.Errorf("%v - Failed to cancel order: %v", cancel.Exchange, err)
+		err = fmt.Errorf("%v - Failed to cancel order: %v", cancel.Exchange, err)
+		return err
 	}
 	var od *order.Detail
 	od, err = o.orderStore.GetByExchangeAndID(cancel.Exchange, cancel.ID)
 	if err != nil {
-		return fmt.Errorf("%v - Failed to retrieve order %v to update cancelled status: %v", cancel.Exchange, cancel.ID, err)
+		err = fmt.Errorf("%v - Failed to retrieve order %v to update cancelled status: %v", cancel.Exchange, cancel.ID, err)
+		return err
 	}
 
 	od.Status = order.Cancelled
+	msg := fmt.Sprintf("Order manager: Exchange %s order ID=%v cancelled.",
+		od.Exchange, od.ID)
+	log.Debugln(log.OrderMgr, msg)
+	Bot.CommsManager.PushEvent(base.Event{
+		Type:    "order",
+		Message: msg,
+	})
+
 	return nil
 }
 
+// GetOrderInfo calls the exchange's wrapper GetOrderInfo function
+// and stores the result in the order manager
+func (o *orderManager) GetOrderInfo(exchangeName, orderID string, cp currency.Pair, a asset.Item) (order.Detail, error) {
+	if orderID == "" {
+		return order.Detail{}, errors.New("order cannot be empty")
+	}
+
+	exch := Bot.GetExchangeByName(exchangeName)
+	if exch == nil {
+		return order.Detail{}, ErrExchangeNotFound
+	}
+	result, err := exch.GetOrderInfo(orderID, cp, a)
+	if err != nil {
+		return order.Detail{}, err
+	}
+
+	err = o.orderStore.Add(&result)
+	if err != nil && err != ErrOrdersAlreadyExists {
+		return order.Detail{}, err
+	}
+
+	return result, nil
+}
+
 // Submit will take in an order struct, send it to the exchange and
 // populate it in the orderManager if successful
 func (o *orderManager) Submit(newOrder *order.Submit) (*orderSubmitResponse, error) {
@@ -381,7 +420,8 @@ func (o *orderManager) Submit(newOrder *order.Submit) (*orderSubmitResponse, err
 
 	return &orderSubmitResponse{
 		SubmitResponse: order.SubmitResponse{
-			OrderID: result.OrderID,
+			IsOrderPlaced: result.IsOrderPlaced,
+			OrderID:       result.OrderID,
 		},
 		InternalOrderID: id.String(),
 	}, nil

engine/orders_test.go

@@ -278,6 +278,31 @@ func TestCancelOrder(t *testing.T) {
 	}
 }
 
+func TestGetOrderInfo(t *testing.T) {
+	OrdersSetup(t)
+	_, err := Bot.OrderManager.GetOrderInfo("", "", currency.Pair{}, "")
+	if err == nil {
+		t.Error("Expected error due to empty order")
+	}
+
+	var result order.Detail
+	result, err = Bot.OrderManager.GetOrderInfo(fakePassExchange, "1234", currency.Pair{}, "")
+	if err != nil {
+		t.Error(err)
+	}
+	if result.ID != "fakeOrder" {
+		t.Error("unexpected order returned")
+	}
+
+	result, err = Bot.OrderManager.GetOrderInfo(fakePassExchange, "1234", currency.Pair{}, "")
+	if err != nil {
+		t.Error(err)
+	}
+	if result.ID != "fakeOrder" {
+		t.Error("unexpected order returned")
+	}
+}
+
 func TestCancelAllOrders(t *testing.T) {
 	OrdersSetup(t)
 	o := &order.Detail{

engine/rpcserver.go

@@ -811,7 +811,7 @@ func (s *RPCServer) GetOrder(_ context.Context, r *gctrpc.GetOrderRequest) (*gct
 		Quote:     currency.NewCode(r.Pair.Quote),
 	}
 
-	result, err := exch.GetOrderInfo(r.OrderId, pair, "") // assetType will be implemented in the future
+	result, err := s.OrderManager.GetOrderInfo(r.Exchange, r.OrderId, pair, "") // assetType will be implemented in the future
 	if err != nil {
 		return nil, fmt.Errorf("error whilst trying to retrieve info for order %s: %s", r.OrderId, err)
 	}