[13] using a fixed price feed will result in large rounding errors for BTC

ddombrowsky commented 4 years ago

When doing a simple bot to trade around the buy/sell spread of a BTC market, the "buy" side is pretty much useless, because it only uses 7 decimal points to represent the price. e.g.

1/0.0000069 = 144927 XLM per BTC 1/0.0000070 = 142857 XLM per BTC

That's almost 1.5% spread.

I can't find any way in the configuration to work around this limitation.

Expected behavior

The bot should place buy orders in a BTC market with a normal spread, not one every 2070 XLM.

Frequency

This always happens. Either it's a limitation of the configuration, or I'm just not seeing the right way to configure this.

A sample config using the buysell strategy is:

DATA_TYPE_B="fixed"
DATA_FEED_B_URL="1.0"
DATA_TYPE_A = "sdex"
DATA_FEED_A_URL="BTC:GBVOL67TMUQBGL4TZYNMY3ZQ5WGQYFPFD5VJRWXR72VA33VFNL225PL5/XLM:"

Your Environment

  cli version: master:v1.8.1
  gui version: v0.0.1-alpha
  git branch: master
  git hash: 90e31022e46f5585a81f6890e6a1d4747455673f
  build date: 20200217T212830Z
  env: release
  GOOS: linux
  GOARCH: amd64

Context

I am unable to run a simple bot on a BTC market because of this bug.

ddombrowsky commented 4 years ago

maybe if I could change PricePrecision in :

2020/04/17 22:32:06 orderConstraints for trading pair BTC/XLM: OrderConstraints[PricePrecision: 7, VolumePrecision: 7, MinBaseVolume: 0.0000001, MinQuoteVolume: <nil>]

ddombrowsky commented 4 years ago

This appears to be hardcoded:

plugins/sdex.go

 30 var sdexOrderConstraints = model.MakeOrderConstraints(7, 7, 0.0000001)

Welp, that was fun while it lasted.

nikhilsaraf commented 4 years ago

@ddombrowsky the Stellar DEX only allows 7 decimals of precision for all assets. This cannot be changed.

You are already pricing your market as BTC/XLM, which is ~= 143000 so you should not be seeing the issues mentioned since the number is large. If you set your market to XLM/BTC you would see a price ~= 0.0000069 and you would see the precision issues mentioned.

Can you share your log file so I can get a better understanding of the issues you are facing.

ddombrowsky commented 4 years ago

@nikhilsaraf

@ddombrowsky the Stellar DEX only allows 7 decimals of precision for all assets. This cannot be changed.

While this is true, prices are specified in numerator / denominator values, so it is very possible to place a "buy" order for BTC between the values of 144927 XLM per BTC and 142857 XLM per BTC.

You are already pricing your market as BTC/XLM, which is ~= 143000 so you should not be seeing the issues mentioned since the number is large. If you set your market to XLM/BTC you would see a price ~= 0.0000069 and you would see the precision issues mentioned.

If you flip the pricing, you'll then see the same issue on the sell side. I've tried both.

Can you share your log file so I can get a better understanding of the issues you are facing.

Here's an example using the "balanced" method:

2020/04/18 10:28:25 buy  | modify | old level=1 | new level = 1 | triggers=[amount] | targetPriceQuote=142857.14285714 | targetAmtBase=0.00000020 | curPriceQuote=142857.14285714 | lowPriceQuote=129870.12987013 | highPriceQuote=158730.15873016 | curAmtBase=0.00000010 | minAmtBase=0.00000018 | maxAmtBase=0.00000022
2020/04/18 10:28:25 buy  | create | new level=2 | priceQuote=142857.14285714 | amtBase=0.00000020
2020/04/18 10:28:25 buy  | create | new level=3 | priceQuote=142857.14285714 | amtBase=0.00000020
2020/04/18 10:28:25 sell, done creating preceding offers (numLevelsConsumed=0, hitCapacityLimit=false, numOps=0, newTopOfferPrice=<nil>)
2020/04/18 10:28:25 sell | modify | unmodified original level = 1 | newLevel number = 1
2020/04/18 10:28:25 sell | modify | unmodified original level = 2 | newLevel number = 2
2020/04/18 10:28:25 sell | modify | unmodified original level = 3 | newLevel number = 3

You'll notice that the priceQuote for all 3 buy orders are the same.

I don't know enough about go to point to a quick fix. I'm hoping this isn't a system limitation.

ddombrowsky commented 4 years ago

Unfortunately, it's not as simple as editing that one line in plugins/sdex.go. It blows up:

2020/04/18 10:48:03 error in selling sub-strategy: unable to create preceding offers: unable to create new preceding offer: error: cannot create or modify offer, invalid price: -92233.72036855

ddombrowsky commented 4 years ago

If I look at one of the raw offers:

{
  "id": "125589649803780097",
  "paging_token": "125589649803780097",
  "transaction_successful": true,
  "source_account": "GAWYHDRCFT2YOFFDYAAPSLJB6ULIZBSNNPJKW6KIEU3EQ7KJZPUBVT24",
  "type": "manage_sell_offer",
  "type_i": 3,
  "created_at": "2020-04-18T16:58:02Z",
  "transaction_hash": "4d143c226e44d06c303644551ef5c70f0fedbba4dbc724cd56a50f65dcad3519",
  "amount": "0.0287770",
  "price": "0.0000070",
  "price_r": {
    "n": 7,
    "d": 1000000
  },
  "buying_asset_type": "credit_alphanum4",
  "buying_asset_code": "BTC",
  "buying_asset_issuer": "GBVOL67TMUQBGL4TZYNMY3ZQ5WGQYFPFD5VJRWXR72VA33VFNL225PL5",
  "selling_asset_type": "native",
  "offer_id": "0"
}

That 7/1000000 isn't going to work.

nikhilsaraf commented 4 years ago

@ddombrowsky if the bot computes prices for the two buy levels as 0.00000701234 and 0.00000701345 then both of them will get rounded down to 7 decimal places (because of the SDEX limitation), making them 0.0000070. This would result in the same price for both offers when inverted = 1/0.0000070 = 142857.1428571. In order to fix this, you will need to increase the spread in your config to at least 1.42857% on either side [1].

Since you are seeing a price of 0.0000070 it could be one of two things:

you are getting 7 decimal places of precision but you are being rounded as described above. The only solution to this is increasing your spread as described above.
your price is actually something like 0.000007234 but you are getting only 6 decimal places of precision, which would mean it's a bug in kelp since this should show up as 0.0000072. If so I will put out a fix ASAP.

In order to figure out which situation we are in here it would help to see your full log file. Can you upload it here?

[1] Note that every trading exchange has a minimum spread that is possible depending on the precision of price that is allowed. Since SDEX has 7 decimals of precision, the smallest amount you can represent is 0.0000001. At a price of 0.0000070, this means the minimum allowed spread is 0.0000001/0.0000070 = 1.42857%. As the price of the underlying asset increases, the minimal allowed spread decreases. This may not be the issue you are seeing but it's an important factor to consider when configuring your bot and I thought I should mention it here. This means that your levels need to be at least 1.42857% apart from each other on either side, so they produce different prices after rounding.

ddombrowsky commented 4 years ago

@nikhilsaraf

you are getting 7 decimal places of precision but you are being rounded as described above. The only solution to this is increasing your spread as described above.

This is what I'm seeing. It's using 7 decimals of precision, which means BTC assets priced in XLM cannot be priced accurately using the kelp system.

This means that your levels need to be at least 1.42857% apart from each other on either side, so they produce different prices after rounding.

Again, this is not true. It is very possible to place orders in a BTC/XLM market between 1/0.0000070 and 1/0.0000069. This is not a limitation of the protocol.

I'll attached the full output of the kelp binary, using the balanced strategy, run overnight.

If you can confirm that this is a system limitation, I might dig into the code to see if there is a fix that doesn't overhaul the guts of the trading bot. Otherwise, it looks like this bot isn't useful for any assets trading higher than 100,000 XLM per unit.

(Consequentially, it means anything trading above 1,000,000 XLM will barely work at all, such as BTC/NGNT)

ddombrowsky commented 4 years ago

screenlog.19.zip

nikhilsaraf commented 4 years ago

@ddombrowsky The code to add support for more than 7 decimals in Kelp is relatively straightforward (see experimental commit of modifications needed).

However, I believe there is a restriction in the go sdk which gives an error "more than 7 significant digits". This error is currently preventing us from using more than 7 decimals in kelp. stellar-core will honor more than 7 decimals internally as it uses fractions to represent price. We would need to have this patched in the go sdk first after which it would work in kelp with the experimental commit. See a test that proves the limitations of the amount.ParseInt64 function.

I will try to update the precision issue for prices via the go sdk but that may take some time as I'm tied up with releasing the GUI for Kelp.

For now you could try running the branch in the first link as a starting point (there are some hacks in it right now as you can see with the arbitrary decimal values of 11 and 13 and excessive logging). You could create a fork of the stellar/go repo (see the glide.yaml file for the fork currently being used) to correct the issue in the sdk at amount.go#ParseInt64. If you can get the sdk to allow more than 7 decimals then it should work with the experimental commit proposed (first link).

Note that Kelp is currently using an older version of the go sdk so it's possible that the latest go sdk does not have this issue and then it's just a matter of upgrading to the new sdk.

ddombrowsky commented 4 years ago

@nikhilsaraf

This error is currently preventing us from using more than 7 decimals in kelp.

Can you instead specify the price using the fractional notation? IIRC the SDK allows you to specify one or the other, when posting an order.

nikhilsaraf commented 4 years ago

The old Go SDK allows you to specify the price in fractions, which is what we are doing internally via the new Go SDK. However from what I recall, the check for 7 decimals of precision still applies in the old Go SDK. This check is not correct imo and should be removed.

I haven't looked into an alternative code path for this check yet, which would mitigate the issue. The old Go SDK is deprecated and Kelp has partially migrated to the new SDK where this precision issue would not be a problem anyway. However, the migration is blocked on other work slated for Q2/Q3 and until we complete the migration we will still need to go through the old SDK to sign and submit transactions.

I will be leaving this issue open to be fixed soon once I've gotten through the release of rc1 of the GUI.

ddombrowsky commented 4 years ago

as a work-around, I was able to hack in a switch from "sell" to "buy" when the price is too low to be significant.

https://github.com/ddombrowsky/kelp/commits/405-flip-buy-sell

@nikhilsaraf If this problem is still present in the "new" go SDK, then I can re-write this to be a non-hack fix.

bartekn commented 4 years ago

Created two issues in Go SDK that should fix the behaviour described here: https://github.com/stellar/go/issues/2514 https://github.com/stellar/go/issues/2515.

nikhilsaraf commented 4 years ago

Leaving this issue open until the related issues in stellar/go are resolved so I remember to allow more than 7 decimals. Once fixed we should be able to remove the associated compatibility code from Kelp.

nikhilsaraf commented 4 years ago

@ddombrowsky you can check out the new pendulum strategy, which is an improvement on the balanced strategy

ddombrowsky commented 4 years ago

@nikhilsaraf does this mean I can remove my code that flipped the buy/sell values if the precision wasn't enough? (see my commits on https://github.com/ddombrowsky/kelp/commits/415-balanced-amounts )

ddombrowsky commented 4 years ago

BTW I tried out stock 1.9.0, and I still get the same problem. All the "buy" orders are for the same amount:

2020/07/08 16:12:11 buy  | create | new level=1 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=2 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=3 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=4 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=5 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=6 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=7 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=8 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=9 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=10 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=11 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=12 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=13 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=14 | priceQuote=117647.05882353 | amtBase=0.00000040
2020/07/08 16:12:11 buy  | create | new level=15 | priceQuote=117647.05882353 | amtBase=0.00000040

nikhilsaraf commented 3 years ago

note to self: consider using https://github.com/shopspring/decimal library

ddombrowsky commented 3 years ago

looks like both issues are still open in the stellar go SDK: stellar/go#2514 stellar/go#2515 . As such, code on the master branch will still not work with markets that require more than a few decimal points of precision (such as BTC/USD or BTC/anything really).

I've updated my fork to merge in changes from current master. As before, this does the following:

Uses the PassiveOffer flag temporarily when a sale price is less than 0.001
When the price is too small, it flips the order from a ManageSellOffer to a ManageBuyOffer, which flips the fraction and effectively works around the precision problem.
It disables some tracking code because that is all written specifically for ManagedSellOffer objects. This could be fixed, but would require a bit more refactoring.

Code is here: https://github.com/ddombrowsky/kelp/tree/405-flip-buy-sell

If anyone besides me find this useful or interesting, let me know and I will clean it up so I can submit it as a PR. (@nikhilsaraf @bartekn )

ddombrowsky commented 2 years ago

FYI: The rounding bug is STILL present in the latest version of kelp and the SDK.

I have updated my fork to the latest v1.12.0-dd, cli version: master-dd:v1.12.0-14-g3f14c71c

https://github.com/ddombrowsky/kelp/tree/master-dd

stellar-deprecated / kelp