-
Notifications
You must be signed in to change notification settings - Fork 96
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
feat(earn): handle claims in withdraw submit saga #6203
Conversation
# Conflicts: # src/earn/EarnDepositBottomSheet.tsx
Co-authored-by: Finnian Jacobson-Schulte <140328381+finnian0826@users.noreply.github.com>
src/earn/saga.ts
Outdated
@@ -456,7 +463,132 @@ export function* withdrawSubmitSaga(action: PayloadAction<WithdrawInfo>) { | |||
} | |||
} | |||
|
|||
export function* claimSubmitSaga(action: PayloadAction<WithdrawInfo>) { |
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 disagree with the choice to make this its own function. The vast majority of it is copy/paste and doing the exact steps as withdrawSubmitSaga. I don't think there is a strong motivation to separate them for separation of concerns, because their concerns are very close. Code-wise it looks like you will need to add two if statements max to withdrawSubmitSaga to be able to make it optionally include withdraw transactions or not.
am i missing something here?
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.
Co-authored-by: Joe Bergeron <jbergero@alum.mit.edu>
# Conflicts: # src/earn/EarnConfirmationScreen.test.tsx # src/earn/EarnConfirmationScreen.tsx # src/earn/hooks.ts
Description
Adds claim handling to the
withdrawSubmitSaga
insrc/earn/saga.ts
to handle pending claim transactions. This PR also includes a fix where withdrawals that include claims do not prepare or include a separate claim transaction.Test plan
Related issues
Backwards compatibility
Yes
Network scalability
N/A