Manage position

[ferostabio]

References issue https://github.com/Fujicracy/fuji-v2/issues/41

Adding some particularly important items that need to be taken into account when tackling this task

• [x] Load vaults and positions at the app level, regardless of page
• [x] Update again when loading the page in order to get the latest
• [x] Before navigating to borrow with a set vault, always check (if possible) if there’s an existing position
• [x] When loading a position, check basically URL parameters to see if we have valid information
• [x] Clearing inputs when switching from position to borrow and back...
• [x] Unless the user goes to access the position screen from Borrow
• [x] Update input errors and button based on a current position
• [x] Show position estimate based on position + user input
• [x] Move transaction modal to the app level
• [x] Add the vault's address to the transaction history, otherwise we'll never be able to show it
• [x] Update transaction modal to account for the managing operations
• [x] Change active vault if needed and reload data when opening position page directly

[github-actions[bot]]

Please test this PR on: https://v2-staging-288-dot-fuji-306908.ey.r.appspot.com

[0xdcota]

Hey @ferostar, I implemented the function and actually renamed it to viewFuturePosition. I was able to calculate in there all the values I think you may need in this overview dashboard and is all returned in a Position type. Now, I think your approach idea was a bit different, and the calculation meant to be done once for "collateral" and another for "debt". However, let me know what you think on my proposal. Completely understand if this is not how you want to handle it and happy to split it in the whichever way you desire.

[ferostabio]

Hey @ferostar, I implemented the function and actually renamed it to viewFuturePosition. I was able to calculate in there all the values I think you may need in this overview dashboard and is all returned in a Position type. Now, I think your approach idea was a bit different, and the calculation meant to be done once for "collateral" and another for "debt". However, let me know what you think on my proposal. Completely understand if this is not how you want to handle it and happy to split it in the whichever way you desire.

Hey @DaigaroCota I still have to separate the structure somehow, but what you did is great 🙌. I'll just create another function to do it. Thanks!

[ferostabio]

@DaigaroCota I have a problem using the function.

If you go to /my-positions and select one, it will take you to [pid] which will grab the query URLs and pass the data to /borrow/Wrapper. There I grab the right position from positions and call the viewFuturePosition function, but this is what I get.

[0xdcota]

@DaigaroCota I have a problem using the function.

If you to /my-positions and select one, it will take you to [pid] which will grab the query URLs and pass the data to /borrow/Wrapper. There I grab the right position from positions and call the viewFuturePosition function, but this is what I get.

I think I know why, and is probably because I defined that as a getter instead of a property. Let me look into this.

[ferostabio]

I think there's another issue @DaigaroCota . Say the user has 5usd as debt. Througout the app, the AssetMeta for it will be { amount: 5, usdValue: 1 }. But the position returned in my positions call has { amount: 5, usdValue: 5 } so it shows that the debt is 25 instead of 5.

[0xdcota]

I think there's another issue @DaigaroCota . Say the user has 5usd as debt. Througout the app, the AssetMeta for it will be { amount: 5, usdValue: 1 }. But the position returned in my positions call has { amount: 5, usdValue: 5 } so it shows that the debt is 25 instead of 5.

Oh ok I think this is an interpretation issue on what is usdValue, and I was thinking, since working on positions, that we will have this issue at some point. I was interpreting usdValue as the total value in dollars of the collateral/debt. Otherwise I would had name that "price" or usdPrice. So I think we have to align in wether this usdValue is = to price or the dollar value of the collateral/debt.

[ferostabio]

The flow is (pretty much) done. Next step is to take care of some remaining inconsistencies with the returned positions. And testing everything and bug fixing, of course.

[ferostabio]

Fixed a zillion bugs already, going to keep at it.

@brozorec some notes:

• When previewing borrow, the first step returns no token (is needed and returned in other cases, even if it’s the same token)
• We need to handle allowance diffs for all operations.
• We shouldn't forget that "borrow limit" is still hardcoded

[brozorec]

@ferostar great job so far :clap: I quickly tested it, and stumbled upon some cases that weren't well spec'ed out.

1. The user needs to sign a permit only when there's a borrow or a withdrawal action
2. When managing a position, the routes returned by the SDK are not correct. I had to fix that. For ex.: the user has a position on Polygon, and they want to deposit+borrow from Optimism. In that case, there should be only one available route: bridge from Optimism and deposit+borrow on Polygon. Let me double-check and confirm that, but I think that allowing the user to choose a route should be available only when they open the position, i.e only on the borrow page. That would mean that when calling the preview methods of the sdk, we have to define a new param (is a new position or an existing one).
3. I suggest the following changes in wording that will make things clearer on "Remove position": "Collateral from" becomes "Withdraw to" and "Borrow to" becomes "Payback from".
4. I also think it makes more sense to swap the collateral and the debt inputs when we are on "Remove position", i.e. first comes the "Payback from" (debt) and than "Withdraw to" (collateral). Logically, we first repay and then withdraw.
5. When the user is managing a position and they choose a 3-chain operation, we need to display a message that those operations are not available yet. For ex: the position is on Polygon and they select to deposit from Arbitrum and to borrow to Optimism.
6. When repaying, remove error "Debt amount too low".
7. When on "Remove position", the selected chain should match the chain of the borrowed asset.

---

Regarding the points, you brought up:

When previewing borrow, the first step returns no token (is needed and returned in other cases, even if it’s the same token)

Let me check that.

We need to handle allowance diffs for all operations.

Agree; we talked about it the week before and said we wanted to handle that after the big refactoring of the borrow store.

We shouldn't forget that "borrow limit" is still hardcoded

I said I'll check that and I haven't yet, sorry :pray:

[ferostabio]

Thanks for bringing up all this points @brozorec ! Super helpful, I've been playing by ear when there weren't specs, just making progress until having some feedback, so everything you say makes sense. Routes/previews are the most complicated from the batch it seems, we can discuss them next week.

[brozorec]

Regarding point 1. from the list above. The SDK exposes a static method Sdk.needSignature(actions) that will return if there's a need to sign a permit. I can see in the borrow store that we already make use of it.

---

The issues on point 2. turned out to be easier handled than I thought yesterday. With #356 I suggested how to manage that, let me know if this approach works for you.

[ferostabio]

1) You're right, we're saving the result of the needSignature call in the needPermit state variable, but we're not using that at all. It's never read. It'll be a matter of checking the value before calling sign.

2) formType is another variable that wasn't used at all, anywhere. I was just saving the manage vs borrow boolean in the local state, but I didn't realize it was going to be needed. So your solution works but only if we update the borrow state's formType property from the BorrowWrapper component. But in order to do it right, I'll get rid of the managePosition property I pass to subcomponents and use this formType from the global state instead. I'll do that on Monday and merge.

[ferostabio]

Did all of the mentioned points above but wanted to add a note about 5, preventing 3-chain operations: instead of showing an error I simply prevented the user from changing the chains when managing a position.

Now... @brozorec @DaigaroCota please test the hell out of everything. There are a lot of points that weren't in the specs, already found a lot of bugs but I'm sure I'm missing a lot of things (and introduced a couple of bugs in the meantime 😅) but I'm not even sure. Need a lot of feedback, @brozorec points above were wonderful.

[brozorec]

Hey @ferostar glad my comments helped :) I have limited capacity to test today and tomorrow, here some more comments:

instead of showing an error I simply prevented the user from changing the chains when managing a position.

The main value proposition of the v2 is "deposit, borrow and repay your debt positions from any chain" so we need to enable at least the 2-chain interactions when user wants to manage their position.

The general rules for managing positions should be:

• when the user does a single operation (only deposit, only borrow, ...), they can do it from chain that's different from the chain where the position lives
• when the user does deposit+borrow, the deposit chain can be anything, but the borrow chain has to be the position's chain
• when the use does payback+withdraw, the payback chain can be anything, but the withdraw has to be the position's chain

I'll get rid of the managePosition property I pass to subcomponents and use this formType from the global state instead.

I didn't know about managePosition property, if that's what you want to use to diff between "only-borrow" and "manage", I'm good :+1:

---

Regarding the allowance when repaying, should we create a new ticket and PR or we do it here?

[0xdcota]

@ferostar Some feedback on test on the front-end

• [x] Show error when user wants to borrow less than minAmount of contract (stablecoins no less than 1 dollar)
• [ ] Transaction modal shows success in all cases; however, if error it should show error.
• [x] Check needsPermit in BorrowButton component.
• [x] In manage my position "Current Loan-To-Value" percentage has many decimals.
• [x] When trying to make simple Deposit collateral in manage position an error is thrown in console: Unexpected undefined param
• [x] In the Borrow page, when attempting to do a deposit-borrow across chains the bridge fee is estimated as ~$0.00. Not sure if this is intended, but perhaps decimals should adapt to at least show there is a fee. It is different to say fee == 0 or below two decimals.
• [ ] When having an open position with collateral and trying to withdraw with MAX, the lower button becomes disabled with message Too much collateral?

[ferostabio]

The general rules for managing positions should be:

Got it, I'll disable changing chains according to those rules.

Regarding the allowance when repaying, should we create a new ticket and PR or we do it here?

I would create a new ticket/PR and base it in this branch. If you do it here, the conversation will turn into a nightmare. We should keep this one for bugs.

[0xdcota]

@brozorec did we remove the relayer fee from here? If we are sponsoring it, at least we should notify user we are sponsoring. This with the idea that user knows there is a sponsored fee, and there is no "rug" feeling later on.

[ferostabio]

@brozorec changing chain requires a lot more thought. It's not trivial to implement. Just one example:

• Polygon-only position WETH -> DAI. User switches to Gnosis... but there's only USDC there, so the screen now shows USDC -and the frontend has no way of knowing that before changing chains and seeing the returned tokens, as far as I think.

At the same time, and regardless of the token change, the user might type a collateral amount (so it is possible to choose chains everywhere) and then clears the collateral, so we're in only borrowing mode and the user already is in a different chain.

We need to handle all different cases/possibilities and how to treat each.

[0xdcota]

@brozorec @ferostar, one of the potential vulnerabilities identified by Macro and Hats competition is giving infinite erc-20- approval to the router contract. Lets please comment out the infinite approval from the front end for private beta.

[ferostabio]

Now implementing https://github.com/Fujicracy/fuji-v2/issues/363

[brozorec]

Got to test the manage position page and here are my findings:

• [x] On "Remove position" In the payback input, instead of "LTV ..% (Recommended: ..%)" we display the balance of the debt asset in the user's wallet and a "max" button. When the user clicks on the max button:

• if "user balance" >= "user debt amount", we fill in the input with "user debt amount"

• if "user balance" < "user debt amount", we fill in the input with "user balance" In the withdraw input, instead of "balance" and "max", we display "LTV ..% (Recommended: ..%)"

• [x] On "Add position", when the form is in an idle state and no inputs are filled yet, the disabled button is better "Deposit & Borrow" instead of "Borrow"

• [x] On "Overview", "LTV liquidation threshold" should be multiplied by 100 (0.8% -> 80%) similar to how it's on the borrow page.

• [x] Remove the "after" label on the borrowed amount if no inputs are filled yet

• [x] Only withdrawing to a chain different from the one of the position doesn't work. The 2nd param of sdk.preview.withdraw should be vault.collateral.chainId instead of collateralToken.chainId. Even after I changed it, the button displayed an incorrect error. + when I clicked on the routes, the app crashed.

• [x] On the borrow page, when the user is not connected and clicks on the button "Connect wallet" in the form, the auth modal doesn't show up

[ferostabio]

@brozorec crash happens because preview.withdraw returns a step without a token. So far, in all cases, steps have a token property, but not in this case. Is this a bug in the sdk or is the frontend is wrong about the assumption?

[ferostabio]

@brozorec double checked the code and the app always expects to have values both in the start and end steps. They're used for the icon and the symbol, both required. Without any of them, of course, the app crashes. But I saw the code and it's pretty clear in that the start token isn't even sent.

How should the frontend handle it? Just to be clear, it's the routing card that needs it. No way to render it, as it stands, without start/end step's token.

[brozorec]

@ferostar In the case of only borrow or only withdraw, the start step doesn't include a token. As you pointed out as well, we can see the first step as some sort of "request": withdraw or borrow and no token is to be transferred from the user. I suggest the routing card displays a generic image instead of the token with a msg "Request". Do you that could work?

Another point, I see you have changed the content of the allowance modal, however, the address is not the one of the router but of the vault and the chain needs to be the source chain. You can get the router address from the sdk by importing CONNEXT_ROUTER_ADDRESS[srcChainId].

[ferostabio]

@ferostar In the case of only borrow or only withdraw, the start step doesn't include a token. As you pointed out as well, we can see the first step as some sort of "request": withdraw or borrow and no token is to be transferred from the user. I suggest the routing card displays a generic image instead of the token with a msg "Request". Do you that could work?

I believe so, but we also need to change the text since we won't have the token symbol and it would just make no sense. We need:

• New image
• New text @brozorec should we ask Mark today?

Another point, I see you have changed the content of the allowance modal, however, the address is not the one of the router but of the vault and the chain needs to be the source chain. You can get the router address from the sdk by importing CONNEXT_ROUTER_ADDRESS[srcChainId].

I was 51% sure the vault's address was the wrong one but didn't know about this one 👍.

[ferostabio]

A couple of notes @brozorec

Questions:

• Button title for the case when the user is attempting to payback more than the position amount
• Button title for the case when the user is attempting to withdraw more than the position amount
• In overview, when managing a position, when “…below current price”we’re supposed to change the amount color when… exactly? Depending on the recommended ltv?

Missing things… am I right? Anything missing?

• Show deposit APR in Overview
• Borrow limit
• Gas fees

[brozorec]

Button title for the case when the user is attempting to payback more than the position amount

Payback more than amount due

Button title for the case when the user is attempting to withdraw more than the position amount

Withdraw more than allowed

In overview, when managing a position, when “…below current price”we’re supposed to change the amount color when… exactly? Depending on the recommended ltv?

We follow the same logic as in "my-positions". If not possible, let's keep it simple: when x < 20% display in red, otherwise in green.

Borrow limit

We calculate it as follows: collateral amount collateral price maxLtv

Gas fees

Maybe we handle the gas fees and the other fees in a separate PR. I think we have to add in the Est. cost the expected slippage as well but let's talk about it tomorrow.

Show deposit APR in Overview

:+1:

Figured out that when on "Remove position" the route should inverted. For ex. my position below is on Polygon and I'm operating from Polygon so the route should be Polygon > Optimism.

[ferostabio]

@brozorec solved all issues plus some. We can do another round of testing and tackle gas fees on another PR.

[ferostabio]

@NikolaiYurchenko we already merged slippage here, so please have a look at this one:

57:6 Warning: React Hook useEffect has a missing dependency: 'slippage'. Either include it or remove the dependency array. Mutable values like 'textInput.current' aren't valid dependencies because mutating them doesn't re-render the component. react-hooks/exhaustive-deps.

This thread about it is pretty interesting.

[ferostabio]

Already fixed the above, we shouldn't use useRef like that anymore. @brozorec please test slippage once more please!