-
Notifications
You must be signed in to change notification settings - Fork 262
Conversation
* Update trade.go * Update trade.go * Update trade.go
* Update buysell.md * Update README.md * Update buysell.md * Update balanced.md * Update sell.md * Update trade.go * Update buysell.md * Update trade.go * Update balanced.md * Update sell.md
* cap3: 1 - implement liabilities for XLM * cap3: 2 - integrate liabilities into willOversell and willOverbuy methods * cap3: 3 - remove fractionalReserveMultiplier cli arg * cap3: 4 - offers in the same tx will contribute to liabilities, incorporate into cachedLiabilities * cap3: 5 - handle case of setting incrementalBuy for native asset * cap3: 6 - refactored willOversell and willOverbuy to extract common offer logic * cap3: 7 - added support for checking XLM fee and min reserves * cap3: 8 - update ordering of operations in strategies (sellSideStrategy, mirrorStrategy) when not all offers can be placed — always place inside orders first * cap3: 9 - native fee inclusion checks source/trader account usage * cap3: 10 - delete offer if nothing was created and we were planning to modify an existing offer * cap3: 11 - prepend deleteOps so we "free" up our liabilities capacity to place the new/modified offers * cap3: 12 - better error propagation from sdex.createModifySellOffer * cap3: 13 - extracted liability updates after placing/modifying offers to callers in sellSideStrategy and mirrorStrategy * cap3: 14 - prepend deleteOps for mirror strategy too * cap3: 15 - log liabilities in trader.go after resetting and after updating ops * cap3: 16 - add liabilities in mirror strat * cap3: 17 - added support for partial offers in sellSideStrategy, refactored updateSellLevel * cap3: 18 - updated comment when resetting cached liabilities * cap3: 19 - add current offer amounts to liabilities when not modifying an offer * cap3: 20 - add a caching layer for asset balances to reduce requests * cap3: 21 - cleaner logging of asset balance and trust amounts * cap3: 22 - let new capacity constraint system handle max limits for sellSideStrategy * cap3: 23 - reset caches before pruning and updating operations * cap3: 24 - update CHANGELOG * new release: v1.0.0-rc2 * expand assets allowed to use with the Kraken exchange, fixes stellar-deprecated#13 * new release: v1.0.0-rc3 * remove default rate offset percent from sample strategy config files * Print strat (stellar-deprecated#15) * Update trade.go * Update trade.go * Update trade.go * enable Travis CI, closes stellar-deprecated#17 (stellar-deprecated#18) * enable Travis CI * go_import_path * travis: ./bin/kelp version * doc clarify full path needed for configs (stellar-deprecated#16)
Catch up to root fork - attempt 2
wanted to acknowledge this PR, will look at it soon. |
Cool, thanks. |
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.
style suggestion, otherwise it looks good.
plugins/sellSideStrategy.go
Outdated
if s.divideAmountByPrice { | ||
targetAmount = *model.NumberFromFloat(targetAmount.AsFloat()/targetPrice.AsFloat(), targetAmount.Precision()) | ||
} | ||
if targetAmount.AsFloat() > 0 && targetPrice.AsFloat() > 0 { |
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.
the code convention is to return errors when they are encountered rather than to wrap the real logic in deeply nested if blocks (you can imagine how deep this can get if you have a series of error checks).
instead of using the pattern:
if (condition) {
// do something
} else {
// log error
}
can you use this pattern:
if targetPrice.AsFloat() == 0 {
return nil, nil, fmt.Errorf("targetPrice is 0")
}
if targetAmount.AsFloat() == 0 {
return nil, nil, fmt.Errorf("targetAmount is 0")
}
// rest of the code stays here
note that in the suggestion above we're returning an error rather than handling it by logging. the caller should handle any errors that arise from this method.
Changes made. Once we have this finalized and committed I'll check the rest of the UpdateWithOps funcs. |
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.
lgtm, thanks!
Fix the divide by zero error from #5
Added a conditional to check that both relevant numbers are >0. This should exclude cases where no valid number exists as well. I think this would need to be done for all UpdateWithOps funcs across strategy plugins.