-
Notifications
You must be signed in to change notification settings - Fork 86
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
refactor(earn-types): combine withdraw actions and earn enter mode #6187
base: tomm/act-1389
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## tomm/act-1389 #6187 +/- ##
=================================================
+ Coverage 88.90% 88.91% +0.01%
=================================================
Files 733 733
Lines 31158 31160 +2
Branches 5427 5429 +2
=================================================
+ Hits 27700 27705 +5
+ Misses 3415 3412 -3
Partials 43 43
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 am not sure I see the benefits of combining the enums yet, but maybe I am missing some context. This just seems to broaden the type for things that need to remain specific, which opens up the possibility of bugs. Can you list where this common type would be beneficial?
@@ -69,11 +67,11 @@ export interface BeforeDepositAction { | |||
} | |||
|
|||
export interface WithdrawAction { | |||
name: WithdrawActionName | |||
name: EarnActiveAction |
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 don't think we should be using a broader type here. Since deposit
, swap-deposit
aren't valid "withdraw" actions. This opens up the possibility of bugs / handling invalid types. Can we still keep a narrow type here? You can still have a broader type if it makes things easier in other places, but we should keep types as narrow as possible
@@ -81,11 +81,11 @@ export type StackParamList = { | |||
[Screens.EarnInfoScreen]: undefined | |||
[Screens.EarnEnterAmount]: { | |||
pool: EarnPosition | |||
mode?: EarnEnterMode | |||
mode?: EarnActiveAction |
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.
same comment as above, the screen shouldn't need to deal with invalid action names.
@@ -196,7 +196,7 @@ export async function prepareWithdrawTransactions({ | |||
} | |||
} | |||
|
|||
export function usePrepareTransactions(mode: EarnEnterMode) { | |||
export function usePrepareTransactions(mode: EarnActiveAction) { |
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.
the implementation of this function seems wrong now, if mode
is exit | claim-rewards
this would end up calling prepareDepositTransactions
@@ -34,7 +34,7 @@ export async function prepareDepositTransactions({ | |||
feeCurrencies: TokenBalance[] | |||
pool: EarnPosition | |||
hooksApiUrl: string | |||
shortcutId: EarnEnterMode | |||
shortcutId: Exclude<EarnActiveAction, 'exit'> |
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.
can we include only deposit related shortcuts
@@ -163,7 +163,7 @@ export async function prepareWithdrawTransactions({ | |||
feeCurrencies: TokenBalance[] | |||
pool: EarnPosition | |||
hooksApiUrl: string | |||
shortcutId: EarnEnterMode | |||
shortcutId: Exclude<EarnActiveAction, 'exit'> |
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.
same comment as above
title: string | ||
details: string | ||
iconComponent: React.MemoExoticComponent<({ color }: { color: Colors }) => JSX.Element> | ||
onPress: () => void | ||
} | ||
|
||
export type EarnEnterMode = 'deposit' | 'swap-deposit' | 'withdraw' | ||
export type EarnActiveAction = 'withdraw' | 'claim-rewards' | 'deposit' | 'swap-deposit' | 'exit' |
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.
if you're changing this to Action
, can we also change variable names to be action
instead of mode
} | ||
[Screens.EarnConfirmationScreen]: { | ||
pool: EarnPosition | ||
mode: WithdrawActionName | ||
mode: EarnActiveAction |
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.
same here, seems like this screen is specific to the withdraw actions
Description
There were two types previously used for the earn flow:
EarnEnterMode
- Used to prepare the correct transaction inEarnEnterAmount.tsx
andEarnDepositBottomSheet.tsx
.WithdrawActionName
- Used to determine what to display on theEarnConfirmationScreen.tsx
and prepare transactions for that screen.This PR refactors this logic so that earn flow uses the new
EarnActiveAction
type which mirrors the currently available shortcuts plus anexit
option. This should make determining the appropriate display and transaction preparation easier by being more consistent throughout the flow.Test plan
Related issues
Backwards compatibility
Yes
Network scalability
N/A