-
Notifications
You must be signed in to change notification settings - Fork 263
Conversation
Ok, accounting/pnl.go is not compatible, which I didn't catch because its not compiled as part of Kelp. We could expose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some suggestions which should simplify things a little
Updated. I may have misunderstood something about the global hack variable suggestion. If I have it named |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these changes look good. here's what's left (also commented inline):
- update method signature for
GetOrderBook(pair *model.TradingPair, maxCount int32) (*model.OrderBook, error)
- fix PrivateSdexHack global to hold api and network only
plugins/priceFeed.go
Outdated
@@ -38,6 +41,12 @@ func MakePriceFeed(feedType string, url string) (api.PriceFeed, error) { | |||
} | |||
tickerAPI := api.TickerAPI(exchange) | |||
return newExchangeFeed(url, &tickerAPI, &tradingPair), nil | |||
case "sdex": | |||
sdexFeed, e := newSDEXFeed(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this to makeSDEXFeed; by convention we use make*
instead of new*
so as to not confuse with the new keyword in Golang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Should we also change the other price feed functions from new
to make
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not at this point since it's not a big deal right now. can address later if needed as a separate issue.
closes #97 |
I've made the remaining changes. |
cmd/trade.go
Outdated
@@ -163,6 +163,10 @@ func init() { | |||
sdexAssetMap, | |||
) | |||
|
|||
// setting the temp hack variables for the sdex price feeds | |||
plugins.PrivateSdexHack.API = sdex.API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be client
(not sdex.API
) and utils.ParseNetwork(botConfig.HorizonURL)
instead of sdex.Network
(can extract into a network
variable first.
it's reasonable for MakeSDEX to do something like this if it needs to: API = nil
since that's an abstraction we get with the make factory method. So when we reach into the sdex.API here we're breaking that abstraction and creating a dependency where there shouldn't be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plugins/sdex.go
Outdated
|
||
// GetOrderBook gets the SDEX order book | ||
func (sdex *SDEX) GetOrderBook(pair *model.TradingPair) (orderBook horizon.OrderBookSummary, e error) { | ||
//b, e := api.LoadOrderBook(*assetBase, *assetQuote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove commented out code for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plugins/sdex.go
Outdated
@@ -813,3 +813,19 @@ func (sdex *SDEX) GetLatestTradeCursor() (interface{}, error) { | |||
|
|||
return records[0].PT, nil | |||
} | |||
|
|||
// GetOrderBook gets the SDEX order book | |||
func (sdex *SDEX) GetOrderBook(pair *model.TradingPair) (orderBook horizon.OrderBookSummary, e error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should conform to the API method described in the previous comments:
GetOrderBook(pair *model.TradingPair, maxCount int32) (*model.OrderBook, error)
this would require transforming orders from horizon's format to the model.OrderBook
format
I can give you sample code for this if needed, I have something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sample code would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Reidmcc the sample code below converts a single manageOffer
to a model.Order
.
You'd need to do something similar, converting from horizon.OrderBookSummary
to model.OrderBook
(which mostly contains a list of model.Order
). Hope this is useful.
func manageOffer2Order(mob *build.ManageOfferBuilder, baseAsset horizon.Asset, quoteAsset horizon.Asset) (*model.Order, error) {
orderAction := model.OrderActionSell
price := model.NumberFromFloat(float64(mob.MO.Price.N)/float64(mob.MO.Price.D), utils.SdexPrecision)
volume := model.NumberFromFloat(float64(mob.MO.Amount)/math.Pow(10, 7), utils.SdexPrecision)
isBuy, e := assetsEqual(quoteAsset, mob.MO.Selling)
if e != nil {
return nil, fmt.Errorf("could not compare assets, error: %s", e)
}
if isBuy {
orderAction = model.OrderActionBuy
// TODO need to test price and volume conversions correctly
// volume calculation needs to happen first since it uses the non-inverted price when multiplying
volume = model.NumberFromFloat(volume.AsFloat()*price.AsFloat(), utils.SdexPrecision)
price = model.InvertNumber(price)
}
return &model.Order{
Pair: &model.TradingPair{
Base: model.FromHorizonAsset(baseAsset),
Quote: model.FromHorizonAsset(quoteAsset),
},
OrderAction: orderAction,
OrderType: model.OrderTypeLimit,
Price: price,
Volume: volume,
Timestamp: model.MakeTimestamp(time.Now().UnixNano() / int64(time.Millisecond)),
}, nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, I just committed my own version before I saw this, let me know if I've got it.
…factor/exctract order transform step
This adds a "sdex" pricefeed option to buysell and sell, as we talked about on #39. It doesn't have any custom strategy stuff, just the feeds.