feat: unwrap native token in borrow/withdraw operations #2313
Conversation
🦋 Changeset detectedLatest commit: 9e5727e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
gleiser-oliveira
changed the title
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
653a124 to
cd8212e
Compare
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
cd8212e to
224d516
Compare
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
224d516 to
5435382
Compare
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
5435382 to
7bf9d31
Compare
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
7bf9d31 to
2a0d2e1
Compare
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
651a84e to
938cad6
Compare
| const shouldSelectNativeToken = | ||
| canWrapNativeToken && nativeTokenBalanceTokens?.gte(asset.userWalletBalanceTokens); |
There was a problem hiding this comment.
I don't think we should ever select the token being wraped by default. The UX can be confusing for users this way, as they'd be looking at a certain market (say WETH), but the form would directly propose them to repay their balance with another token (ETH). For someone who isn't familiar with the UI, they might think they can't repay using WETH.
After checking I realized we have this logic on the Supply form too. Let me know what you think, personally I'd remove the logic from both forms.
There was a problem hiding this comment.
This is actually a request from product, to select either the native token or the wrapped version, based on the balance the user has
There was a problem hiding this comment.
I see. Then my only suggestion would be to change this line to use gt instead gte, so that the underlying token is still favored over over the token being wrapped. What do you think?
| const shouldSelectNativeToken = | ||
| canWrapNativeToken && nativeTokenBalanceTokens?.gte(asset.userWalletBalanceTokens); |
There was a problem hiding this comment.
Same comment as on the Repay form: I wouldn't change the token selected by default.
There was a problem hiding this comment.
While testing the UX with the new wrap/unwrap features, I realized we currently cannot repay an entire WETH loan using ETH. The way we currently do this for other markets is by passing MAX_UINT256 to the repayBorrow function of the corresponding VToken contract, but as far as I can understand the NativeTokenGateway contract does not allow to do the same.
I've asked the contract team to confirm whether my assumption is correct and if the contract can be updated to permit this behavior ((here)[https://github.com/VenusProtocol/isolated-pools/pull/361#discussion_r1526043355]).
| if (vTokenBalanceMantissa) { | ||
| if (formValues.receiveNativeToken) { | ||
| return redeemAndUnwrap({ | ||
| amountMantissa: vTokenBalanceMantissa, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Let's not use nested IF statements, it tends to make the logic harder to read.
A nice way to do that could be to merge all the redeem mutation functions into one withdraw function under the API client (similarly to how the repay function handles multiple use cases), which would determine what contract to call based on parameters passed to it. What do you think?
We can do this in a separate PR and I'm happy to assist. I'd follow this logic for all four actions: supply, borrow, withdraw, repay, this way we might simplify the forms a little bit.
There was a problem hiding this comment.
Sounds good! I agree, we can refactor it in a separate PR, this way the form only has to reach out a single mutation and keep this logic out of it.
| if (tokenBalance.token.address in acc) { | ||
| return acc; | ||
| } | ||
| return { | ||
| ...acc, | ||
| [tokenBalance.token.address]: tokenBalance, | ||
| }; |
There was a problem hiding this comment.
Nit: you could use the areAddressesEqual utility function to prevent cases where the same token is returned twice with the same address but using a different casing.
| @@ -387,6 +392,27 @@ const RepayForm: React.FC<RepayFormProps> = ({ asset, pool, onCloseModal }) => { | |||
| [isWrapUnwrapNativeTokenEnabled, asset.vToken.underlyingToken.tokenWrapped], | |||
| ); | |||
|
|
|||
| const nativeTokenBalanceTokens = useMemo(() => { | |||
There was a problem hiding this comment.
I think we should keep prefixing variables to make it easy to understand what balance we are defining here: userWalletNativeTokenBalanceTokens.
I'd follow this logic on other forms as well (Supply/Borrow/Withdraw).
| fromToken: asset.vToken.underlyingToken, | ||
| fixedRepayPercentage: undefined, | ||
| }); | ||
| const { data: nativeTokenBalanceData } = useGetBalanceOf( |
There was a problem hiding this comment.
nit: I think we should keep prefixing variables to make it easy to understand what balance we are defining here: userWalletNativeTokenBalanceData.
gleiser-oliveira
force-pushed
the
feat/unwrap-native-token-borrow-withdraw
branch
from
69cb2d8 to
4616427
Compare
Jira ticket(s)
VEN-2391
Changes
borrowAndUnwrapredeemUnderlyingAndUnwrapandredeemAndUnwrap