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

Cancel-everything: should only cancel all of one ecosystem #219

Closed
dexX7 opened this issue Nov 29, 2014 · 10 comments
Closed

Cancel-everything: should only cancel all of one ecosystem #219

dexX7 opened this issue Nov 29, 2014 · 10 comments

Comments

@dexX7
Copy link

dexX7 commented Nov 29, 2014

  1. A1 starts with 50.0 MSC, 50.0 TMSC, 50 MIndiv1 and 50.0 TDiv1
  2. A1 offers 50.0 MSC for 50.0 MDiv1
  3. A1 offers 50.0 TMSC for 50 TIndiv1
  4. A1 cancels everything in the test ecosystem (50.0 TMSC, 50 TIndiv1)

This should only cancel the offer in the test ecosystem, but it appears to cancel-everything and the scope is not limited to the given ecosystem.

Related test lines: test_cancel_everything_scope.py#L143-L540

@zathras-crypto
Copy link

Interesting point - spec is ambiguous on this - I actually agree with DexX on what I think is good behaviour but spec doesn't explicitly state that it is eco dependent, rather giving impression everything is cancelled regardless of anything else:

"CANCEL-EVERYTHING cancels all open orders for all currencies."

@dexX7
Copy link
Author

dexX7 commented Nov 29, 2014

Hm. The reason I tend to believe it's limited to one ecosystem is the note about an explicit ecosystem field here:

OmniLayer/spec#284 (comment)

@ghost
Copy link

ghost commented Dec 1, 2014

I think for consistency with not allowing cross-ecosystem trades in tx21, the cancel's should also only apply to one ecosystem (with cross-ecosystem behaviors being removed)

@m21
Copy link

m21 commented Dec 1, 2014

Then the spec needs to be changed to indicate that? I am fine either way.
@marv-engine

@dexX7
Copy link
Author

dexX7 commented Dec 1, 2014

It's currently undefined and ambigious.

@dacoinminster
Copy link

I think it is not limited to an ecosystem currently, and the spec should
reflect that.

On Mon, Dec 1, 2014 at 9:49 AM, dexX7 [email protected] wrote:

It's currently undefined and ambigious.


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

@dexX7
Copy link
Author

dexX7 commented Dec 1, 2014

@dacoinminster: it's not a question, if it is limited to an ecosystem, but if it should be. Unless exceptional circumstances are given, current behavior should not dictate what ends up in the spec, but the other way around and adding such a limitation seems trivial and should not have any weight here.

I created OmniLayer/spec#290 and if you have an opinion on this, please go ahead.

@dacoinminster
Copy link

Well, changing the code is more than trivial - it affects not just the spec
and code, but also RPC interface. When the code gets ahead of the spec I
tend to go with the code unless there is an obvious problem with the way it
was implemented. Perhaps this slightly violates the "sandbox" intention of
Test MSC, but I'd point out that most testing is now taking place on
testnet, so we probably shouldn't care.

That said, if the devs want to change this for some reason, I have no
objection to changing the spec too.

I'll paste this comment in issue #290 too.

On Mon, Dec 1, 2014 at 12:55 PM, dexX7 [email protected] wrote:

@dacoinminster https://github.com/dacoinminster: it's not a question,
if it is limited to an ecosystem, but if it should be. Unless
exceptional circumstances are given, current behavior should not dictate
what ends up in the spec, but the other way around and adding such a
limitation seems trivial and should not have any weight here.

I created OmniLayer/spec#290
OmniLayer/spec#290 and if you have an
opinion on this, please go ahead.


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

@dexX7
Copy link
Author

dexX7 commented Dec 9, 2014

Addressed by #223.

@marv-engine
Copy link

I'm just seeing this now but my thinking is that all actions take place
within an ecosystem, which had to be clearly indicated in the tx message.

On Monday, December 1, 2014, Michael [email protected] wrote:

Then the spec needs to be changed to indicate that? I am fine either way.
@marv-engine https://github.com/marv-engine


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

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
[email protected]

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

No branches or pull requests

5 participants