search

jl777 / SuperNET

56 stars 217 forks source link

Marketmaker should not report `price` for test currencies #862

Open sindresorhus opened 6 years ago

sindresorhus commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

jl777 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

portfoliozero:1

something to indicate the specific behavior of the coin

jl777 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

ok, i can do istest:true