Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SO: unwanted orders price shifts due to incrementing price #575

Closed
bitphage opened this issue Apr 28, 2019 · 4 comments
Closed

SO: unwanted orders price shifts due to incrementing price #575

bitphage opened this issue Apr 28, 2019 · 4 comments
Assignees
Labels
component: Staggered Orders [2] Advanced Issue Issue is a technically advanced or complex task. Requires prior knowledge [3] Type: Bug

Comments

@bitphage
Copy link
Collaborator

From bitshares/python-bitshares#227 (comment)

In dexbot we're incrementing prices to place staggered orders, this is a test which shows actual order price and initially calculated price are different:

        closer_order = worker.place_closer_order(asset, order, place_order=True)
    
        # Test for correct price
>       assert closer_order['price'] == order['price'] * (1 + worker.increment)
E       assert 99.01980296969697 == 99.0099009894
E         -99.01980296969697
E         +99.0099009894

So, we should switch from incrementing prices to incrementing amount only (base or quote) to place staggered orders, and as a consequence we need a method to place orders specifying both base and quote amounts (see the first message)

Another example: test setup failed to place initial orders to close the target spread, see that 1% error:

_______________________________________________________ test_maintain_strategy[mountain-config0] ________________________________________________________

initial_allocation = {}

    def test_maintain_strategy(initial_allocation):
        """ Check if intial orders placement is correct
        """
        worker = initial_allocation
    
        # Check target spread is reached
>       assert worker.actual_spread < worker.target_spread + worker.increment
E       assert 0.030186976828429968 < (0.02 + 0.01)
E        +  where 0.030186976828429968 = {}.actual_spread
E        +  and   0.02 = {}.target_spread
E        +  and   0.01 = {}.increment
@bitphage bitphage added [3] Type: Bug [2] Advanced Issue Issue is a technically advanced or complex task. Requires prior knowledge labels Apr 28, 2019
@bitphage
Copy link
Collaborator Author

The less asset precision, the bigger price increment error is.

@MarkoPaasila
Copy link
Collaborator

The problem is that we assume that we get the price we specify. When calculating prices we must do so taking into account the available resolution, and use an actually possible price going forward. See my pr for reference. I'm working on an improved version, but it seems pretty specific to Staggered Orders (with it's order size limits), and maybe not so suitable as a generic function.

bitphage added a commit to bitphage/DEXBot that referenced this issue Sep 10, 2019
This allows to fix tests failing due to Codaone#575. Note this is not a final
fix!
bitphage added a commit to bitphage/DEXBot that referenced this issue Jan 28, 2020
Problem described here Codaone#575
Workaround is to use approx value for spread comparison accepting a
slight error.
bitphage added a commit to bitphage/DEXBot that referenced this issue Mar 14, 2020
Problem described here Codaone#575
Workaround is to use approx value for spread comparison accepting a
slight error.
@bitphage bitphage self-assigned this Apr 9, 2020
@bitphage
Copy link
Collaborator Author

bitphage commented Apr 9, 2020

Further workaround improvement was 00a920f

@bitphage
Copy link
Collaborator Author

bitphage commented Apr 9, 2020

I think this could be closed now as applied workaround eliminated (almost?) all consequences.

For later, I would refactor SO to use Decimal amounts on orders calculations and precise prices based on such amounts. Price correction method was proposed in #512, but it's usage requires SO refactor anyway. Also, could be implemented more clean used Decimal and custom mul() div() as recommended in https://docs.python.org/3/library/decimal.html#decimal-faq

@bitphage bitphage closed this as completed Apr 9, 2020
bitphage added a commit to bitphage/DEXBot that referenced this issue Apr 11, 2020
Sevaral tests were market as xfailed due to imprecise price caclulation,
this commit adds pytest.approx() to allow slight error, see Codaone#575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Staggered Orders [2] Advanced Issue Issue is a technically advanced or complex task. Requires prior knowledge [3] Type: Bug
Projects
None yet
Development

No branches or pull requests

2 participants