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

Marketmaker should not report price for test currencies #862

Open
sindresorhus opened this issue May 23, 2018 · 10 comments
Open

Marketmaker should not report price for test currencies #862

sindresorhus opened this issue May 23, 2018 · 10 comments

Comments

@sindresorhus
Copy link

PIZZA returns a price when calling the portfolio endpoint. I assume this is because Marketmaker uses the coinmarketcap API and there actually is a real currency called PIZZA, so it return the value for that. This case should be detected and the price returned should always be 0.

@jl777
Copy link
Owner

jl777 commented May 23, 2018

what happened was somebody traded KMD for PIZZA, KMD has actual value, so it lists PIZZA with value.

it seems to be a higher level issue as to what is and isnt a test currency. couldnt the GUI just as easily not use any value for PIZZA?

i can make a hardcoded special case, but in general putting such things in the core leads to unintended consequences.

@sindresorhus
Copy link
Author

We have worked around it in HyperDEX, but technically it belongs in Marketmaker. If it's not handled in Marketmaker, each GUI app has to be surprised by the behavior, spend time looking into why it's showing a price, and then work around it.

@jl777
Copy link
Owner

jl777 commented May 23, 2018

but even the coins definitions is not internal to marketmaker. how can it know what to do about coins that are created externally?

@jl777
Copy link
Owner

jl777 commented May 23, 2018

i guess if a "testcoin":1 is added to the coin definition, then marketmaker could change behavior based on that. is suppressing a price the only difference?

this is a slippery slope adding special cases for externally defined things, but if we can abstract it into a specific behavior change, it should be ok.

maybe better is "noportfolio":1 ?

@lukechilds
Copy link
Contributor

maybe better is "noportfolio":1 ?

If it doesn't get returned in the response of the portfolio command, it'll break our GUI.

We need it to act like a normal coin, the only difference being the price is always 0.

@lukechilds
Copy link
Contributor

lukechilds commented May 23, 2018

@sindresorhus mentioned isTest: true in our internal discussion which I think makes sense.

@jl777 ah, just re-read your message above:

i guess if a "testcoin":1 is added to the coin definition, then marketmaker could change behavior based on that. is suppressing a price the only difference?

Yes, this is exactly what we should do IMO.

@jl777
Copy link
Owner

jl777 commented May 23, 2018

portfoliozero:1

something to indicate the specific behavior of the coin

@jl777
Copy link
Owner

jl777 commented May 23, 2018

also making it istest:1 could be done, but I feat that will be overloaded by other behavior changes that are wanted for test coins

are you sure zero portfolio value is the only change we will ever do for test coins?

@sindresorhus
Copy link
Author

portfoliozero:1

Should be portfolioPrice: false or if you insist noportfolioprice: 1.

also making it istest:1 could be done, but I feat that will be overloaded by other behavior changes that are wanted for test coins

Is that a bad thing? It's normal to have some specific behavior for test fixtures.

are you sure zero portfolio value is the only change we will ever do for test coins?

No, that's why we suggested isTest: true.

@jl777
Copy link
Owner

jl777 commented May 28, 2018

ok, i can do istest:true

DeckerSU pushed a commit to DeckerSU/SuperNET that referenced this issue Nov 5, 2022
…tfix

Use correct direction of the pair for best_orders sell action jl777#862
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

3 participants