Static Typing: Transfer resources POC #364
Conversation
| @@ -81,6 +82,8 @@ open class TradingStateMachine( | |||
| private val maxSubaccountNumber: Int, | |||
| private val useParentSubaccount: Boolean, | |||
| ) { | |||
| private val internalState: InternalState = InternalState() | |||
There was a problem hiding this comment.
should we just take this opportunity to move away from a single monolithic state object?
There was a problem hiding this comment.
It doesn't hurt to have an object that aggregates different sub states. It'd make the migration a bit easier since it's a one-to-one mapping to the state dictionary.
There was a problem hiding this comment.
yeah i guess a couple thoughts:
if we want to break down the state machine into smaller pieces, it'd be easier if the different components were exposed directly rather than through an aggregate object. but maybe we can cross that bridge when we get there.
also, it seems a bit strange that internalState itself is a val, but holds mutable sub properties. currently, any consumer of InternalState could modify its values which I don't think we want? IMO, it'd be more clear if either:
a) internalState itself was a var (with perhaps a private set) and its sub fields were immutable
b) we just moved the sub fields to be come top-level mutable fields (basically get rid of InternalState) - again probably with private setters.
if we're concerned about this object growing and want fine-grained updates over copy(), i think b) is a better approach.
tl;dr: in general i think we should strive to model data with immutable values, and be strict on what objects are actually allowed to update them.
There was a problem hiding this comment.
Updated...
I'd rather delay immutable implementation until we switch all internal states to static types. copy() has a performance overhead, and we had run into performance issue before. We can only benchmark it after all states have been converted. Also, the current mutable implementation mirrors 1-to-1 to the state dictionary, so it's a relatively safer refactor. Even so, there is already substantial complexity of finding all the usage of the fields within the code-base. Switching to immutable at this stage would make the refactor riskier.
src/commonMain/kotlin/exchange.dydx.abacus/state/internalstate/InternalState.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/exchange.dydx.abacus/state/internalstate/InternalTransferInputState.kt
Outdated
Show resolved
Hide resolved
| import exchange.dydx.abacus.output.input.TransferInputChainResource | ||
| import exchange.dydx.abacus.output.input.TransferInputTokenResource | ||
|
|
||
| internal data class InternalTransferInputState( |
There was a problem hiding this comment.
TOL: should we remove Input from this? I think it's a bit confusing, since this isn't actually an input type from the UI (rather it is state needed for Transfer).
Maybe InternalTransferOptionsState?
There was a problem hiding this comment.
It's the state that drives the transfer input screens (i.e. deposit, withdrawal, transfer). The name is designed to be consistent with TradeInput.
Co-authored-by: mobile-build-bot-git <[email protected]>
Co-authored-by: mobile-build-bot-git <[email protected]>
Co-authored-by: mobile-build-bot-git <[email protected]>
As a proof of concept, creating an internal static typed object "InternalState" and using it to store the Transfer related resources.
Since those resources don't need further processing after fetching from Squid, we just generate the output types directly. Other use-cases might require creating new internal types to replace the data map.
There are also other transfer data that should be converted to static typing, but this PR shows that there can be an intermediate state where the data map co-existing with static typed data.
Tested with the Android app.