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

Add missing "cancel" checks, "cancel-everything" ecosystem limitation #223

Closed

Conversation

dexX7
Copy link

@dexX7 dexX7 commented Dec 3, 2014

  • trade_MP can be used with zero amounts
  • trade_MP can be used with non existing properties
  • fixes behavior where raw encoded offer with zero units for sale fails, but zero units desired passes etc.
  • guards and fillins of todos for "meta dex logic math"

... and cancel-everything that behaves as follows:

  • both properties in main ecosystem -> cancel main properties
  • both properties in test ecosystem -> cancel test properties
  • cross ecosystem values (...) cancel offers of both ecosystems

Consider the last part as debatable of course.

@dexX7
Copy link
Author

dexX7 commented Dec 3, 2014

If desired, I can split this one up, but I was too tired yesterday. Sub parts:

  • flexible input validation of trade_MP to remove constraints related to the fields that are simply junk for some actions
  • don't always reject Bitcoin as property in any case, because the property field could just have been abused as filler and come with "zero" values
  • ensure sufficient values are provided for "cancel-at-price" and "cancel-currency-pair"
  • cross ecosystem behavior and support for "cancel-everything" limited to one ecosystem

Imho, related to the last part, there is consensus for the general idea. I'd want a limitation of "cancel-everything" to one ecosystem, Marv seems to support it too, as the comment linked in the other thread implies, Faiz supports it for consistency, zathras supports it as well. So if you don't hate it, let's do it. ;)

JR's objection was the work needed to adjust the code - which I tackled with this PR. I can push a PR to refine the spec on this as well and imho it's not a spec violation, but the spec is rather ambigious and not clearly defined in this context.

Opinions?

Edit: not to mention all the regtests that don't fail anymore. ;)

@dacoinminster
Copy link

heh. Nice. If the other devs don't object and you are willing to do the
spec work too, this is okay by me.

On Wed, Dec 3, 2014 at 12:16 PM, dexX7 notifications@github.com wrote:

If desired, I can split this one up, but I was too tired yesterday. Sub
parts:

  • flexible input validation of trade_MP to remove constraints related
    to the fields that are simply junk for some actions
  • don't always reject Bitcoin as property in any case, because the
    property field could just have been abused as filler and come with "zero"
    values
  • ensure sufficient values are provided for "cancel-at-price" and
    "cancel-currency-pair"
  • cross ecosystem behavior and support for "cancel-everything" limited
    to one ecosystem

Imho, related to the last part, there is consensus for the general idea.
I'd want a limitation of "cancel-everything" to one ecosystem, Marv seems
to support it too, as the comment linked in the other thread implies, Faiz
supports it for consistency, zathras supports it as well. So if you don't
hate it, let's do it. ;)

JR's objection was the work needed to adjust the code - which I tackled
with this PR. I can push a PR to refine the spec on this as well and imho
it's not a spec violation, but the spec is rather ambigious and not clearly
defined in this context.

Opinions?


Reply to this email directly or view it on GitHub
#223 (comment)
.

@dexX7
Copy link
Author

dexX7 commented Dec 5, 2014

@dacoinminster: PR pending: OmniLayer/spec#292

This commit adds all missing checks before executing "cancel-at-price",
"cancel-all-for-pair" and "cancel-everything".

Furthermore the behavior of "cancel-everything" was enhanced, such that
it cancels all open orders, if offered and desired properties are within
the same ecosystem, and otherwise it cancels all open orders for all
properties of both ecosystems.
@dexX7 dexX7 force-pushed the mcore-mdex-fix-improvements branch from 08afaac to bea28a1 Compare December 9, 2014 19:34
@dexX7 dexX7 changed the title Cancel-xxx, input validation, ecosystem checks Add missing "cancel" checks, "cancel-everything" ecosystem limitation Dec 9, 2014
@dexX7
Copy link
Author

dexX7 commented Dec 10, 2014

@m21:
Consider this as finalized - unless you or anyone else has anything to add of course.. ;)

With #223 test support is back and all tests pass.

@m21
Copy link

m21 commented Dec 19, 2014

TBD after the tag

@dexX7 dexX7 closed this Apr 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants