Fixed float dust bug on zero position #470

guilledk · 2023-02-27T20:43:15Z

In #462 we discovered a bug on the positioning system when trying to get to a zero position state, after investigation we found out it was caused by float precision, the solution is to truncate based on the maximun supported on a symbol by symbol basis.

goodboy · 2023-02-28T18:51:57Z

piker/clearing/_paper_engine.py

@@ -530,7 +535,7 @@ async def trades_dialogue(

    ):

-        with open_pps(broker, 'paper', False) as table:
+        with open_pps(broker, 'paper') as table:


not quite following why you're not writing immediately here any more?

I'm pretty sure that open_pps call is just meant to load the pps.toml file into the _positions module var and then iterate on it to send position messages to the ems.

piker/clearing/_paper_engine.py

piker/data/_source.py

piker/pp.py

piker/data/_source.py

goodboy

This looks excellent yet again @guilledk.

I have a few comments but nothing blocking afaict.

Great work and feel free to merge into the target dev branch whenever ready.
I presume you might need to rebase after the suggested changes made in the last review on #462 land.

piker/brokers/kraken/broker.py

jaredgoldman

LGTM other than the first comment

goodboy · 2023-03-01T18:41:55Z

I'm just gonna manually test with the same scenario I got in #462, presuming it's resolved I'll merge 🏄🏼

goodboy · 2023-03-01T23:16:29Z

@guilledk another thing I think should change is to just put the .symbol section fields under the position entry no?

when are we ever going to need to have a separate section?

a given asset_type = "crypto" price_tick_size = 0.01 lot_tick_size = 1e-5 set is always going to be specific to the particular market/symbol/fqsn for a given broker right?
i don't see any case where this extra sibling section is going to be shared/re-used right?

ad discussed in chat i think we can just stick all the symbol info fields directly in the pp entry since they're one to one:

[binance.paper."btcusdt.binance"]
size = 0.04235
ppu = 23611.15
bsuid = "btcusdt"
asset_type = "crypto"
price_tick_size = 0.01
lot_tick_size = 1e-5
clears = [
 { dt = "2023-03-01T23:37:51.559601+00:00", ppu = 23611.15, accum_size = 0.04235, price = 23611.15, size = 0.04235, cost = 0, tid = "0c162a01-9e63-4121-9475-182cf80753e8" },
]

goodboy · 2023-03-01T23:53:25Z

piker/pp.py

@@ -957,7 +981,8 @@ def open_pps(
            expiry = pendulum.parse(expiry)

        pp = pp_objs[bsuid] = Position(
-            Symbol.from_fqsn(fqsn, info={}),
+            Symbol.from_fqsn(
+                fqsn, entry['symbol']),


As mentioned in chat, we need to do a entry.get('symbol') here and instead just fill in a default Symbol here to continue supporting old pps.toml files.

Further for entries detected as crypto we should probably put a big warning that the entry needs to be recalculated to avoid the precision error this patch was originally meant to fix!

goodboy

I added in the bit about backward compat for existing pps.toml files (since this definitely breaks for me 😂

I'm more then happy to make a patch to this PR before landing 🏄🏼

Add decimal quantize API to Symbol to simplify by-broker truncation Add symbol info to `pps.toml` Move _assert call to outside the _async_main context manager Minor indentation and styling changes, also convert a few prints to log calls Fix multi write / race condition on open_pps call Switch open_pps to not write by default Fix integer math kraken syminfo _tick_size initialization

…ps.toml causing it to load pp wrong on second open, also fix header leak bug

In order to support existing `pps.toml` files in the wild which don't have the `asset_type, price_tick_size, lot_tick_size` fields, we need to only optionally read them and instead expect that backends will write the fields going forward (coming in follow patches). Further this makes some small asset-size (vlm accounting) quantization related adjustments: - rename `Symbol.decimal_quant()` -> `.quantize_size()` since that is explicitly what this method is doing. - and expect an input `size: float` which we cast to decimal instead of doing it inside the `.calc_size()` caller code. - drop `Symbol.iterfqsns()` which wasn't being used anywhere at all.. Additionally, this drafts out a new replacement market-trading-pair data type to eventually replace `.data._source.Symbol` -> `MktPair` which we aren't using yet, but serves as the documentation-driven motivator ;) and, it relates to #467.

Also includes a retyping of `Client._pair: dict[str, Pair]` to look up pair structs and map all alt-key-name-sets to each for easy precision info lookup to set the `.sym` field for each transaction including for on-chain transfers which kraken provides as an "asset decimals" field, presumably pulled from the particular block-token's limitation info.

…info Backward compat support for `Transaction.sym: Symbol`

goodboy · 2023-03-03T17:35:57Z

With the merging of my sub-pr i think this is ready to land 🏄🏼

i saw one last msg that was interesting with a -0.0, not sure what that's about.

 piker.clearing._ems _ems.py:694 Received broker trade event:
{'account': 'paper',
 'avg_price': 22409.6,
 'broker': 'binance',
 'currency': 'btcusdt',
 'name': 'position',
 'size': -0.0,
 'symbol': 'btcusdt.binance'}

gonna do one more review pass.

goodboy

Pretty sure this is finally ready!

I guess we just need to make sure the tests are all good in CI 🚀

guilledk added the bug guille broke it prolly label Feb 27, 2023

guilledk self-assigned this Feb 27, 2023

goodboy requested review from jaredgoldman and goodboy and removed request for jaredgoldman February 28, 2023 16:47

goodboy mentioned this pull request Feb 28, 2023

Paper trade improvements #462

Merged

3 tasks