-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: orderbook calculation bugs, expose more data #442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but would really appreciate some unit tests to illustrate any edge or confusing cases we're concerned with. i'm not as familiar with this part of the code and there's a lot of logic here so i'm hesitant to stamp
src/commonMain/kotlin/exchange.dydx.abacus/processor/markets/OrderbookProcessor.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/exchange.dydx.abacus/processor/markets/OrderbookProcessor.kt
Show resolved
Hide resolved
@@ -510,43 +517,55 @@ internal class OrderbookProcessor(parser: ParserProtocol) : BaseProcessor(parser | |||
} | |||
|
|||
fun group(orderbook: List<Any>?, grouping: Double): List<Any>? { | |||
return if (orderbook != null && orderbook.size > 0) { | |||
return if (!orderbook.isNullOrEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[discussion/suggestion] IMO this is one those functions that has enough math and logic where it makes sense to test separately. we should probably be actively testing edge cases and difficult to reason-about situations like stuff that's been commented here.
i tried to start a pattern with adding the processor tests to the commonTest folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the existing orderbook tests hit the main points pretty well............and I feel like it would take me forever to set up new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the orderbook tests basically hit this directly
No description provided.