-
Notifications
You must be signed in to change notification settings - Fork 465
Refactor/actions reserve overview #2799
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
base: main
Are you sure you want to change the base?
Conversation
…factor/reserve-overview-page-sdk
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
mgrabina
left a comment
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.
HUGE work! Adding some comments.
Idea for relase: prepare a dashboard on amplitude to track all actions and quickly monitor in case we have any issues.
| ).toString(); | ||
| if (reserve.userState) { | ||
| maxAmountToBorrow = reserve.userState.borrowable.amount.value || '0'; | ||
| maxAmountToSupply = getMaxAmountAvailableToSupplySDK(walletBalance, reserve, underlyingAsset); |
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.
aren't we losing minRemainingBaseTokenBalance condition?
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.
During debugging, I noticed an inconsistency between the Legacy flow and the SDK. The difference in the amounts was exactly the value I was using for minRemainingBaseTokenBalance. For that reason, I simplified it. I have the feeling that the SDK already includes minRemainingBaseTokenBalance in its backend calculations. not 100% sure, but is working identical like that
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.
makes sense
| balance = walletBalances[API_ETH_MOCK_ADDRESS.toLowerCase()]; | ||
| } | ||
|
|
||
| const isNativeSelected = reserve.acceptsNative && selectedAsset === baseAssetSymbol; |
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.
should we use isWrappedBaseAsset condition instead of acceptsNative? not sure
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 question everywhere in this file
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.
isWrappedBaseAsset is a flag from API legacy, not present in SDK. The equivalent would be that the object reserve.acceptsNative is not null.
| visibleDecimals={2} | ||
| /> | ||
| <Typography variant="subheader2" color="text.secondary"> | ||
| sDAI |
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.
Is this hardcoded symbol intended?
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. same as the Legacy Flow
| tokenWrapperAddress={wrappedTokenConfig.tokenWrapperAddress} | ||
| tokenIn={wrappedTokenConfig.tokenIn.underlyingAsset} | ||
| amountToSupply={amount} | ||
| decimals={18} |
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.
18 decimals hardcoded intended?
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, required for the case sDai (i just followed the legacy flow)
| <TxActionsWrapper | ||
| blocked={blocked} | ||
| mainTxState={mainTxState} | ||
| approvalTxState={{}} |
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.
we are probably losing the approval state in the button 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.
We’re using the useApprovalTx hook, which handles all approval state and execution internally, so we no longer need to pass approvalTxState to the wrapper. Tested and it works fine.
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 but that wrapper isn't chaing the text based on the approvalTxState?
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.
ok, make sense. updated
| reserve={poolReserve} | ||
| /> | ||
| ) : ( | ||
| <SupplyActions |
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.
or SDK? let's add some comments explaining why we need different Actions for Wrapped or when we use each one for future reference
Same for other actions
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.
sorry! you are right, for the fallback should be SupplyActionsSDK as well
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.
comments added
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
You must have Developer access to commit code to Aave on Vercel. If you contact an administrator and receive Developer access, commit again to see your changes. Learn more: https://vercel.com/docs/accounts/team-members-and-roles/access-roles#team-level-roles |
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
General Changes
• Refactored the Supply and Borrow actions on the
reserve-overviewpage to fully use the SDK for calculating health factor previews and for building the transaction to be sent.• Special cases for wrapped tokens (DAI/sDAI) and USDT resets are all covered and tested.
• This PR also includes the refactor of Withdraw and Repay (added here for future use in the dashboard page).
Developer Notes
• For testing purposes, you can access the Supply SDK and Borrow SDK in the reserve-overview page.
• You can access the Withdraw SDK and Repay SDK in the supplied position view and borrowed position view respectively.
• NOTE: Major issue blocking the release of the Repay SDK flow: I have tested that the SDK’s Repay action does not support aToken repayments. This means we cannot repay debt using collateral; only ERC-20 tokens from the wallet are supported.
Reviewer Checklist
Please ensure you, as the reviewer(s), have gone through this checklist to ensure that the code changes are ready to ship safely and to help mitigate any downstream issues that may occur.
.env.examplefile as well as the pertinant.github/actions/*files