v1.6.48: Add liquidation warning for stop market trigger price #315
Conversation
moo-onthelawn
changed the title
moo-onthelawn
changed the title
| @@ -543,6 +621,19 @@ internal class TriggerOrdersInputValidator( | |||
| return null | |||
| } | |||
|
|
|||
| private fun requiredTriggerToLiquidationPrice(type: String?, side: String?): RelativeToPrice? { | |||
| return when (type) { | |||
| "STOP_MARKET" -> | |||
There was a problem hiding this comment.
If we make these case enums instead of strings, we can avoid having an else -> null case and remove a source of nullability.
I would also consider whether else is even a valid state to be in. If not, we should just error() out instead of returning null. In general, we should be looking to avoid nullable types as much as possible.
There was a problem hiding this comment.
ah interesting; yeah the current pattern I've seen in the repo is to return early (return null) if we get an unexpected null type (but then detekt yelled at me because we had too many return statements in a function oops). Happy to pivot to using error() more liberally - I think this whole file could probably be revamped a bit more (i basically did the min to get it to pass detekt) - filing a tix here.
| null | ||
| } | ||
| } | ||
| else -> null |
There was a problem hiding this comment.
is this else case needed...? (as in, does the compiler complain if you remove it) and if it is, i would again consider whether this is a valid state to be in and if we should just error out instead.
There was a problem hiding this comment.
when using enum and sealed types in a when, the compiler can enforce that we are checking every single case, so we shouldn't need an else case here.
| @@ -130,6 +132,7 @@ internal class TriggerOrdersInputValidator( | |||
| market: Map<String, Any>?, | |||
| oraclePrice: Double, | |||
| tickSize: String, | |||
| liquidationPriceErrors: List<Any>? = null, | |||
There was a problem hiding this comment.
might we be able to instead pass position as an argument and then move the liquidationPriceErrors calculations into this function body? Seems like that is the existing pattern for this function's body
There was a problem hiding this comment.
hmm so liquidationPriceErrors only applies to stop loss orders (not take profit) - which is why I have it passed in from the caller. I could pass in a 🤔 oh actually I do the check for type anyways in the stopLossLiquidation function; so ignore me, yes candoisStopLoss? as a param here but not sure if that's much better
| ): Map<String, Any> { | ||
| val fields = listOf("price.triggerPrice") | ||
| val action = "APP.TRADE.MODIFY_TRIGGER_PRICE" | ||
| // Localizations uses TRIGGER_PRICE_LIMIT as paramater name | ||
| val params = | ||
| mapOf("TRIGGER_PRICE_LIMIT" to mapOf("value" to triggerLiquidation, "format" to "price")) |
There was a problem hiding this comment.
got rid of formatting to price because I don't think this is actually great for internationalization - the $ sign can go before or after the number depending on the language, and I don't think the formatter actually takes into account language? (maybe I'm wrong - but it feels like we should punt this responsibility to our internationalization team)
There was a problem hiding this comment.
rip actually added it back because tickSize doesn't work unless the format is "price". Will go back to localization repo and remove the extra $
| liquidationPriceError( | ||
| "BUY_TRIGGER_TOO_CLOSE_TO_LIQUIDATION_PRICE", | ||
| "ERRORS.TRIGGERS_FORM_TITLE.BUY_TRIGGER_TOO_CLOSE_TO_LIQUIDATION_PRICE", | ||
| "ERRORS.TRIGGERS_FORM.BUY_TRIGGER_TOO_CLOSE_TO_LIQUIDATION_PRICE_NO_LIMIT", |
There was a problem hiding this comment.
strings localized here: dydxprotocol/v4-localization#411
moo-onthelawn
changed the title
moo-onthelawn
deleted the
mulan/ct-778-prevent-user-from-putting-sl-below-liquidation
branch
Tested by building locally on v4-web; added tests. Logic mainly copied from here. before pulling in this updated abacus, web/mobile should also pull in dydxprotocol/v4-localization#411.
One nit about the current localization:
$(and we format it as a price) - decided it was better to remove formatting from abacus and leave it to internationalization team (esp since we're not guaranteed that $ comes before the number in diff languages)