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

Adds method to make order values match the fractional actuality #512

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

MarkoPaasila
Copy link
Collaborator

I suggest this is merged only once a strategy or two makes use of it.

Partial solution to #511.

@MarkoPaasila
Copy link
Collaborator Author

Just to clarify how the method works.
It uses internal amounts (integers). The smaller internal amount - which isn't necessarily the smaller absolute amount - is determined first, because it has less resolution. The other amount is determined next, because it has more granularity and can be better adjusted according to the first amount - to result in a price that matches the target price as closely as possible.

Eventually we can expand the method so you can ask for price, quote, or base to be rounded down or up. For now, it just gets the closest possible fractional value and returns it as float

Copy link
Collaborator

@thehapax thehapax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things.

  1. The name of the method is a bit confusing. perhaps " onchain price", as the word "fix" is non specific
  2. Can you provide an actual use case here with a unit test.
  3. returning 3 items or more, typically should be avoided, in general it would be better to return a datastructure like an arrayy, but clarification is needed in how would be used in a strategy. and if you actually need to return all 3 items.

@MarkoPaasila
Copy link
Collaborator Author

  1. I'll change that
  2. I'm yet to learn unit tests. But since this isn't urgent, we can let this linger until I have done it.
  3. It returns a dict, or doesn't it? I think it's necessary to return all 3 values, as they are all changed in the function, and we don't know which one of them will be used,

@thehapax
Copy link
Collaborator

  1. I'm yet to learn unit tests. But since this isn't urgent, we can let this linger until I have done it.

Actually unit tests are the main priority at the moment. It might be of benefit to see how it actually works. I'll send you references via chat.

@thehapax
Copy link
Collaborator

Hi @MarkoPaasila, as the base.py methods for price handling have been moved to BitsharesOrderEngine and this pull has conflicts, would you kindly resubmit a pull with fresh updates?

@bitphage bitphage added this to Development - Done in Current Development via automation Mar 11, 2020
@bitphage bitphage moved this from Development - Done to Development - In progress in Current Development Mar 11, 2020
@bitphage bitphage moved this from Development - In progress to Frozen in Current Development Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants