Added swap vCow for Cow hook

[nenadV91]

Summary

• adds hook to swap vCow for Cow
• passes the estimated gasLimit to transaction

[github-actions]

• 🔭 GP Swap: CoW Protocol v2 Swap UI

[alfetopito]

Minor comments.

As for the behaviour, was it supposed to work already?
I guess not, as the account I tried had gchain claims and it failed.

[anxolin]

I'm approving, but i think the redux state could be different.

We already know if there's a tx going on of swapVcow. You just make a hook wich filters tx by swapVcow == true

[anxolin]

I already mentioned in some other PR but i was not 100% sure about this approach.

Why a global unique status? Are we sure we want to take this approach? In principle you can send a claimAll and before is mined send another one.

I guess you are right this approach is simpler, only allowing the user to do one at a time, but we could just do the same thing and have a hook that return if there's any claimAll going on

[nenadV91]

Why a global unique status? Are we sure we want to take this approach? In principle you can send a claimAll and before is mined send another one.

The issue is if we don't set some unique status while the swap transaction is being mined the vested amount is still the same (coming from the hook) as it was before the transaction was sent until its mined, so the user would think that something wasn't right and probably try to swap again because the UI doesn't reflect the change

[anxolin]

okok, maybe we can leave it for now...

But, it could just be opening the modal the attempting, the submitted cold be derived from what i just mentioned above, and initial is just not needed. I feel we overcomplicate (maybe i miss somthing)

[anxolin]

nit: are you sure we want to include this? I thought in the call we mentioned this was not needed (approve and wrap don't have it and is implied in the wallet)

[nenadV91]

My understanding was that we want to add this because some wallets calculate this badly

[anxolin]

My understanding was that we want to add this because some wallets calculate this badly

Why do you say this? Also if this is the case, why aren't we doing it in the wrapping an approve?

[W3stside]

It is there @anxolin 😂

cowswap/src/custom/hooks/useApproveCallback/useApproveCallbackMod.ts

Line 128 in baf2293

return tokenContract.estimateGas.approve(spender, amountToApprove.quotient.toString()).catch((error) => {

[anxolin]

For a future PR, and with less prio, we could try to get an estimation on how much we are swapping approximately?

[W3stside]

i think this is absolutely not necessary. adding the amount in the modal will not be accurate and may cause MORE confusion with users saying "oh but the modal showed this but i got xx" and the fact that it's vesting linearly. i think it's better to leave it as is, with no amount

[fairlighteth]

@W3stside On a second thought, I think in practice the deviation of amounts is likely negligible for most account balances. Unless you own millions of vCOW token where it can be somewhat noticeable in mere minutes, right?

[elena-zh]

Hey @nenadV91 , swapping works!
But it would be great to change 'Swapping..' button to 'Convert' back when a transaction fails

Also, we use inconsistent naming of the procedure: We press on the 'Convert' button, and we call 'Swap' action (swap is displayed in the modal/activity history, the button is changed to 'Swapping..'). So, I think, we should use whether 'Convert/converting..' everywhere, or 'Swap/Swapping'

As an enhancement I'd like to propose to show vCOW and COW icons in the activity modal history

[fairlighteth]

I think given this is a on-chain transaction, the transaction modal is closer to the UNWRAP and WRAP modal.

Some text suggestions for the modal (if possible in this PR).

• Almost there! => Convert vCOW to COW
• Swap 0.0067 vCOW for COW => Converting 0.0067 vCOW to COW
• Sign the order with your wallet => Approve the vCOW conversion with your wallet
• The order is submitted and ready to be settled => The vCOW conversion is submitted

[elena-zh]

I think given this is a on-chain transaction, the transaction modal is closer to the UNWRAP and WRAP modal.

Some text suggestions for the modal (if possible in this PR).

• Almost there! => Convert vCOW to COW
• Swap 0.0067 vCOW for COW => Converting 0.0067 vCOW to COW
• Sign the order with your wallet => Approve the vCOW conversion with your wallet
• The order is submitted and ready to be settled => The vCOW conversion is submitted

Looks great!
I would add this one:
Activity history and Toast notifications: Swap vCOW for COW --> Convert <amount > vCOW to COW'

[elena-zh]

Hey @nenadV91 , looks much better!
But I still see 'Swapping' button when conversion transaction is running.
Besides, the button's state is not changed when a transaction fails

Also, could you please change the converting modal text according to @fairlighteth notes above?
Thanks!

[elena-zh]

I have noticed, that we do not have 'Cancelling' or 'Cancelled' states for the Convert transactions.
So look: here I have cancelled the TX, but it is still in 'Pending' state in the app. Could we add to this types of transactions the same logic as we did in #2474 PR?

Pending TX https://rinkeby.etherscan.io/tx/0x15961948b1566942741cf583bdc7d7de7f81d29f3d704609fe05f017fca6596c
Cancelled TX https://rinkeby.etherscan.io/tx/0xef8367088bc65aa1405df29f123ccc38b0abe157bb93c3d7d158f5fdb33cdd55

Also, I have a pending forever transaction when I speed up it.
Pending TX: https://rinkeby.etherscan.io/tx/0xb813afa2dd75c71d6f1e0f70e4ca6b3dfa52ae3ce5e79916426c218778469590
Speeded up TX: https://rinkeby.etherscan.io/tx/0xc45a0d2245763a6a40b9c4949c5ed50eb59a31fd0701f55833dba24da409da9a

[nenadV91]

Minor comments.

As for the behaviour, was it supposed to work already? I guess not, as the account I tried had gchain claims and it failed.

It should work on Rinkeby. I changed the claim repo variable so I think that is why its not working on gchain and mainet.

[elena-zh]

@nenadV91 , tiny nitpick 'Convert vCOW to COW'

[elena-zh]

The button's states/changed text on it looks good to me!
The only thing that is left is to check texts on the modal

to

and resolve these case #2577 (comment)

[nenadV91]

@elena-zh Regarding cancelation issue I don't think this is specifically related to this PR because I tried to do the same for wrap and claim and also stuck on infinite pending. Maybe it has to do something with the new Rinkeby addresses we use but I am not sure.

[nenadV91]

As an enhancement I'd like to propose to show vCOW and COW icons in the activity modal history

@fairlighteth Do you have some idea what is this about. Is the issue related to existing code or is this something new we need to add?

[nenadV91]

Apart from cancel and those icons, the rest of the things mentioned in PR should be fixed now.

[fairlighteth]

@fairlighteth Do you have some idea what is this about. Is the issue related to existing code or is this something new we need to add?

I checked and it doesn't output any image in the DOM. Checking in on src/custom/components/AccountDetails/Transaction/ActivityDetails.tsx L236 - L253 and it seems to check for:

const inputToken = activityDerivedState?.order?.inputToken || null
const outputToken = activityDerivedState?.order?.outputToken || null

[fairlighteth]

👍🏼

[anxolin]

@nenadV91 merge this so @davidalbela can test the subsidy better.

I think is mostly fine. Some nitpicks we found.

Also I think we should not set this to 0, but just show clearly that something is happening. The button message helps. Maybe even a shimmer effect. But the amount i wouldn't change it until the trade is executed.

[alfetopito]

@elena-zh Regarding cancelation issue I don't think this is specifically related to this PR because I tried to do the same for wrap and claim and also stuck on infinite pending. Maybe it has to do something with the new Rinkeby addresses we use but I am not sure.

If I recall correctly, this has to do with Blocknative API. Speed ups/cancellations are a tx replacement with the same nonce.
To have it working locally you need to set the env var, which is not set by default.

If you want I can dig deeper and take a look at that.

[nenadV91]

Ok I am merging this and we can address these comments in the future PR's

[elena-zh]

@elena-zh Regarding cancelation issue I don't think this is specifically related to this PR because I tried to do the same for wrap and claim and also stuck on infinite pending. Maybe it has to do something with the new Rinkeby addresses we use but I am not sure.

If I recall correctly, this has to do with Blocknative API. Speed ups/cancellations are a tx replacement with the same nonce. To have it working locally you need to set the env var, which is not set by default.

If you want I can dig deeper and take a look at that.

@nenadV91 , @alfetopito , I see. Thank you for the explanation.
I retested these issues in Prod, so they are also there. Btw, yes, I see a lot of failed blocknative requests in the console on every environment.

I opened #2583 issue for this.