Skip to content
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: reset amount inputs after successful deposit/withdrawal #1938

Merged
merged 120 commits into from
Feb 10, 2025

Conversation

dschlabach
Copy link
Contributor

@dschlabach dschlabach commented Feb 7, 2025

What changed? Why?
This PR includes the following behavior changes for Earn:

  • After a successful deposit or withdrawal, reset the inputs and re-fetch the relevant balances
  • Re-fetch user balances when user navigates between tabs
  • Use redeem method instead of withdraw method on vault contract for greater decimal precision

Notes to reviewers

How has it been tested?

@@ -41,11 +35,35 @@ export function WithdrawButton({ className }: WithdrawButtonReact) {
[updateLifecycleStatus],
);

const handleOnSuccess = useCallback(
(res: TransactionResponse) => {
console.log('res:', res);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove

@@ -16,9 +16,10 @@ export function buildWithdrawFromMorphoTx({
amount,
receiverAddress,
}: WithdrawFromMorphoParams): Call[] {
console.log('amount:', amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove

const withdrawTxData = encodeFunctionData({
abi: MORPHO_VAULT_ABI,
functionName: 'withdraw',
functionName: 'redeem', // redeem is the number of *shares*, withdraw is the number of *assets*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still unsure about the difference between shares and assets after reading this. That doesn't need to be clarified here, just calling out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I have another PR in the pipe to clean up some of this stuff, I can try and improve it then

cpcramer
cpcramer previously approved these changes Feb 10, 2025
@dschlabach dschlabach merged commit e15bf3a into main Feb 10, 2025
16 checks passed
@dschlabach dschlabach deleted the dms/earn-reset branch February 10, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants