Locked gno claim 2 #405

nenadV91 · 2022-04-13T15:24:58Z

Summary

continues on this one Claim vested COW for locked GNO #364
rebased to develop
added moo sound and updated balance button in the header
adds contract links to the locked GNO card

github-actions · 2022-04-13T15:25:13Z

CLA Assistant Lite:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

1 out of 2 committers have signed the CLA.
✅ @jfschwarz
❌ @lint-action
_{You can retrigger the CLA Action by commenting recheckcla in this Pull Request}

github-actions · 2022-04-13T15:32:45Z

🔭 GP Swap: CoW Protocol v2 Swap UI

elena-zh · 2022-04-15T09:31:49Z

Hey @nenadV91 , great job!
Some issues that I have encountered:

Vested amounts do not match in the tooltip and in the 'Vested' field
Claim button shows a successful state, and disabled when the transaction is failed
I see another COW token address to add to MM. Shouldn't it be the same as for the 'regular' COW token?
Maybe related to the p/1, but strange amount of vested tokens was claimed: I saw 16 tokens available, but received 1438 to my wallet
Maybe a nitpick, but 'Claim' button is disabled and is not changed to the 'Claiming...' once pressed, but transaction is not yet confirmed in the connected wallet

However, nevermind: we do the same for the 'Convert 'button
I'm still not sure about what contract to show for the 'View Contract' button. All the rest cards show (v)COW token address, so here, I assume, we also should show COW token address? Or it is better to show a token distribution contract address?

@alfetopito , @fairlighteth , @anxolin , WDYT?
It might not be related to the current task, but an amount of COW from locked GNO is not included into a discount calculation. I think, we should include it. WDYT?

I have created a separate task Include an amount of COW from locked GNO into discount calculation #409 for this point.

Thank you!

alfetopito

Have not tested the app, just reviewed the code.
Left a couple of nitpicks/suggestions

alfetopito · 2022-04-18T10:50:27Z

src/custom/pages/Profile/LockedGnoVesting/claimData/index.ts

+const chainNames = {
+  [SupportedChainId.MAINNET]: 'mainnet',
+  [SupportedChainId.RINKEBY]: 'rinkeby',
+  [SupportedChainId.XDAI]: 'gnosisChain',
+}


I think we probably have that mapping somewhere else, don't we?

https://github.com/gnosis/cowswap/blob/e28bfd3c271be3183a963846513201dc9000e576/src/custom/constants/chains/chainsMod.ts#L85

alfetopito · 2022-04-18T11:03:36Z

src/custom/pages/Profile/LockedGnoVesting/claimData/index.ts

+  const promise =
+    chunkCache.get(path) ??
+    (fetch(
+      `https://raw.githubusercontent.com/gnosis/locked-gno-cow-merkle-distro/${DISTRO_REPO_BRANCH_NAME}/${path}`
+    ).then((res) => res.json()) as Promise<Record<string, Claim>>)
+  chunkCache.set(path, promise)


Nitpick: could it exit earlier in case there's a cache hit and avoid overwriting the cached value on every call?

alfetopito · 2022-04-18T11:05:37Z

src/custom/pages/Profile/LockedGnoVesting/claimData/rinkeby.json

@@ -0,0 +1,159 @@
+[


I think this was talked about while I was away, but wouldn't be better to store the claim indexes in the same repo as the claim files instead of adding them to the Cowswap's repo like was done for the airdrop?
Or we expect the amount to be significantly smaller that this is acceptable?

I'm personally not very found of having them here.

I second this. I think these are the indexes but, anyways, nice to have to the merkle data project, right?

src/custom/pages/Profile/LockedGnoVesting/hooks.ts

src/custom/pages/Profile/LockedGnoVesting/index.tsx

src/custom/state/claim/hooks/index.ts

W3stside

i think the history got mixed from the develop merge, could you fix it please

elena-zh · 2022-04-19T13:57:01Z

It is impossible to claim COW from locked GNO in GC (neither here nor in #364 pr)

However, it should work based on the commenthttps://github.com/gnosis/cowswap/pull/2599#issuecomment-1084459113

src/custom/pages/Profile/LockedGnoVesting/hooks.ts

jfschwarz · 2022-04-25T12:34:35Z

It is impossible to claim COW from locked GNO in GC (neither here nor in #364 pr)

Thank you for finding this, @elena-zh! This is due to a copy&paste mistake in the contract addresses. (see suggested change above)

jfschwarz · 2022-04-25T12:38:13Z

Vested amounts do not match in the tooltip and in the 'Vested' field

This is because the tooltip shows the total vested balance at the current moment, while in the field we show the amount that can be claimed now, which is the difference of the vested balance and the amount that has been already claimed earlier. We should maybe change the field label to "Claimable" to make this difference more clear?

elena-zh · 2022-04-25T12:56:42Z

Vested amounts do not match in the tooltip and in the 'Vested' field

This is because the tooltip shows the total vested balance at the current moment, while in the field we show the amount that can be claimed now, which is the difference of the vested balance and the amount that has been already claimed earlier. We should maybe change the field label to "Claimable" to make this difference more clear?

I think, it would be great!
Due to according to the 'Convert vCOW to COW' logic we simply show the same amount in both fields, so users might be confused in their expectations in what they see and what they will receive.

So a better explanation will help to avoid these issues.
Btw, it would be nice to provide a better explanation here, I think

A least, to change 'entire vested' to entire 'claimable' amount.

Thanks!

jfschwarz · 2022-04-26T10:03:02Z

@elena-zh

I see another COW token address to add to MM. Shouldn't it be the same as for the 'regular' COW token?

On rinkeby the test deployment of the vesting contract uses a special COW token (0xc50c011B7a2B960D6cb4ac93deFd04A529fDDB3a). We couldn’t use the existing COW token on rinkeby because we needed to fund the vesting contract with a large number of COW so all the existing accounts can be used for testing.

Maybe related to the p/1, but strange amount of vested tokens was claimed: I saw 16 tokens available, but received 1438 to my wallet

Hum, was that on Rinkeby? Looking at the token transfer history of our testing COW token, I can't see any transfer of 1438, but only one with 16, so it looks like it went through as expected.

elena-zh · 2022-04-26T12:59:37Z

@elena-zh

I see another COW token address to add to MM. Shouldn't it be the same as for the 'regular' COW token?

On rinkeby the test deployment of the vesting contract uses a special COW token (0xc50c011B7a2B960D6cb4ac93deFd04A529fDDB3a). We couldn’t use the existing COW token on rinkeby because we needed to fund the vesting contract with a large number of COW so all the existing accounts can be used for testing.

Maybe related to the p/1, but strange amount of vested tokens was claimed: I saw 16 tokens available, but received 1438 to my wallet

Hum, was that on Rinkeby? Looking at the token transfer history of our testing COW token, I can't see any transfer of 1438, but only one with 16, so it looks like it went through as expected.

@jfschwarz , thank you for the explanation!
Regarding to the issue with a transferred token's amount, seems that I did not took into account previously transferred tokens there, so I mistakenly thought that they were added to the wallet in 1 Claim transaction.

Anyways, I will retest this issue again with another account.

Thanks!

elena-zh · 2022-04-26T14:11:06Z

I have retested changes, so now everything looks much-much better.
However, still we have some issues not resolved:

'Vested' amounts on the vCOW and locked GNO cards are calculated differently

so it is better to change the 'Vested' field's label to 'Claimable' on the locked GNO 'card
@nenadV91 , since you have changed 'Convert' button behavior and added 'successful' state to it, I think we need to enhance it a bit more and zero out the claimed amount when 'success' button is displayed. Currenty, it shows a previous claimed balance, and even can still show it when the button enables again
I'm still not sure about what contract to show for the 'View Contract' button. All the rest cards show (v)COW token address, so here, I assume, we also should show COW token address? Or it is better to show a token distribution contract address?

@alfetopito , @fairlighteth , @anxolin , @jfschwarz WDYT?

jfschwarz · 2022-04-26T14:35:50Z

'Vested' amounts on the vCOW and locked GNO cards are calculated differently
so it is better to change the 'Vested' field's label to 'Claimable' on the locked GNO 'card

This is fixed already in this extra PR (I don't have rights to commit to this branch directly)

@nenadV91 , since you have changed 'Convert' button behavior and added 'successful' state to it, I think we need to enhance it a bit more and zero out the claimed amount when 'success' button is displayed. Currenty, it shows a previous claimed balance, and even can still show it when the button enables again

This is actually not the previously claimed balance, but the amount that became vested since you last claimed. So it is correct that the claim button is enabled and the user could theoretically claim every 5 minutes (or however frequently they want).

I'm still not sure about what contract to show for the 'View Contract' button. All the rest cards show (v)COW token address, so here, I assume, we also should show COW token address? Or it is better to show a token distribution contract address?

This button also got fixed in the other PR (the current link is broken). I think the purpose of the link is to give the user confidence and transparency into the contract interaction that will be triggered by pressing the Claim button. So it should really be the vesting contract.

elena-zh · 2022-04-26T14:49:05Z

This is actually not the previously claimed balance, but the amount that became vested since you last claimed. So it is correct that the claim button is enabled and the user could theoretically claim every 5 minutes (or however frequently they want).

@jfschwarz , I have not recorded the video, but in my case I meant that the amount was not immediately updated.
So right after the convert transaction I saw '0.0463' converted token's amount+ success button, but then in a few (up to 10) seconds the amount was updated with a new value like '0.0001' or so.

nenadV91 · 2022-04-26T14:58:03Z

I've noticed another issue, when you logout the locked GNO card is not hidden and it stays there with previously connected account data

anxolin

It worked great for me the claiming!

Codewise looks good too.

I would think is good to merge as soon as we get Elena's stamp.

One small detail, please don't need to fix, just commenting. The icons and logos are wrong, because that is the logo for vCOW to COW conversion. It could be GNO and COW logos, or COW to COW. Same for the COW logo in the profile, it could have a GNO small image overign the logo to differentiate from the other. Again, not important at all.

What is important though, is to coordinate this with the backend, specially after the change from @nenadV91 where now we use the locked COW also for the discount.

When do we want to release this?

anxolin · 2022-04-26T17:56:04Z

src/custom/pages/Profile/LockedGnoVesting/claimData/rinkeby.json

@@ -0,0 +1,159 @@
+[


I second this. I think these are the indexes but, anyways, nice to have to the merkle data project, right?

src/custom/pages/Profile/LockedGnoVesting/hooks.ts

anxolin · 2022-04-26T17:59:59Z

src/custom/pages/Profile/LockedGnoVesting/hooks.ts

+const COW = COW_TOKENS[SupportedChainId.MAINNET]
+
+export const MERKLE_DROP_CONTRACT_ADDRESSES: Record<number, string> = {
+  [SupportedChainId.MAINNET]: '0x64646f112FfD6F1B7533359CFaAF7998F23C8c40',


Are these already the final ones with the right allocation?

src/custom/pages/Profile/LockedGnoVesting/hooks.ts

jfschwarz

Should we maybe leave the fee discount out of scope for now? Especially considering that work has to be done on the back-end for this first, I think this additional aspect should not block us from releasing the claiming feature. (People are already asking for the claim UI.)

jfschwarz · 2022-04-27T08:16:23Z

src/custom/state/cowToken/hooks.ts

+
+    if (account) {
+      if (vCowBalance) tmpBalance = JSBI.add(tmpBalance, vCowBalance.quotient)
+      if (lockedGnoBalance) tmpBalance = JSBI.add(tmpBalance, lockedGnoBalance.quotient)


I think this is not quite correct: We're adding the entire allocation without taking into account how much of it has already been claimed by the user. That share of the allocation that has been claimed is already in the user's wallet as COW and as such is already counted towards the fee discount.

We should use the useCowFromLockedGnoBalances hook instead and then only add allocated - claimed here.

@jfschwarz Good point, I've added fix for this in this PR #460 (in case we want to add this to fee discount).

src/custom/pages/Profile/LockedGnoVesting/index.tsx

elena-zh · 2022-04-28T15:06:10Z

Hey @nenadV91 , great changes!
A tiny issue: only in this PR 'Forum' link is not clickable when all (convert/claim/balance and banner) cards are displayed on the page

Could you please take a look at it?

elena-zh · 2022-04-28T15:21:23Z

Also, I have created related issue #487 that might be fixed later.

…ported

* use new rinkeby contracts * fix merkle drop contract address on GC * more specific names * localize vesting start date * improve copy * fix contract address link

* Add cow from locked GNO to combined balance * Add shimmer effect and handle disconnect

* Added some fixes * Update combined balance calculation * Added comments for locked GNO timestamps * Move locked GNO start date to const file * Moved some other locked GNO dates to const file * Added some PR updates * Small update

elena-zh

LGTM now!

alfetopito

Looks good code-wise, have not tested the app though

nenadV91 marked this pull request as ready for review April 14, 2022 15:38

nenadV91 requested a review from a team April 14, 2022 15:39

elena-zh mentioned this pull request Apr 15, 2022

Include an amount of COW from locked GNO into discount calculation #409

Closed

alfetopito reviewed Apr 18, 2022

View reviewed changes

W3stside reviewed Apr 18, 2022

View reviewed changes

jfschwarz suggested changes Apr 25, 2022

View reviewed changes

src/custom/pages/Profile/LockedGnoVesting/hooks.ts Outdated Show resolved Hide resolved

nenadV91 mentioned this pull request Apr 26, 2022

Claim vested COW for locked GNO #364

Closed

7 tasks

anxolin approved these changes Apr 26, 2022

View reviewed changes

nenadV91 mentioned this pull request Apr 26, 2022

Locked gno claim fixes #460

Merged

jfschwarz suggested changes Apr 27, 2022

View reviewed changes

alfetopito reviewed Apr 27, 2022

View reviewed changes

src/custom/pages/Profile/LockedGnoVesting/index.tsx Outdated Show resolved Hide resolved

elena-zh mentioned this pull request Apr 27, 2022

Freeze amount on Claim/Convert cards when transaction is running #463

Closed

nenadV91 force-pushed the locked-gno-claim-2 branch from a047ad4 to 6ed5e05 Compare April 28, 2022 12:08

elena-zh mentioned this pull request Apr 28, 2022

'Claim' and 'Convert' buttons are enabled when go back to an account with 'pending' transaction #487

Closed

nenadV91 mentioned this pull request Apr 29, 2022

Fix for governance card link #489

Merged

jfschwarz and others added 21 commits April 29, 2022 12:50

use path aliases

0ea520f

fix duplicate use of transaction confirmation modal, which is not sup…

799983e

…ported

some minor fixes

bd64fac

Added vCow locked GNO balance to header button

00d3e4a

Add MOO sounds to the claim from locked GNO transaction

8782b17

Add contract link buttons to locked GNO card

17a33ca

Updated locked GNO contract link

bb98c48

use new rinkeby contracts (#407)

7a4cb8d

Reset confirmed status after some time

75f74a5

Removed some unnecessary code from the merge

565ab48

Fixed some formatting issues

10b0436

Add confirmed state for convert vCow to cow

f1dce65

Fix for tooltip styles

740e882

Fix for failed transactions

6356d74

Locked gno claim 2 fixes (#453)

1085083

* use new rinkeby contracts * fix merkle drop contract address on GC * more specific names * localize vesting start date * improve copy * fix contract address link

Add cow from locked GNO to combined balance (#456)

a57d709

* Add cow from locked GNO to combined balance * Add shimmer effect and handle disconnect

Locked gno claim fixes (#460)

0ed1a4b

* Added some fixes * Update combined balance calculation * Added comments for locked GNO timestamps * Move locked GNO start date to const file * Moved some other locked GNO dates to const file * Added some PR updates * Small update

After rebase fixes

6c4465a

Temporary fix for missing cards on profile page

ed7bcd2

Fix for governance card link (#489)

3b577d1

Fix for convert button

17234fb

nenadV91 force-pushed the locked-gno-claim-2 branch from 30fac7a to 17234fb Compare April 29, 2022 10:50

elena-zh approved these changes Apr 29, 2022

View reviewed changes

Fix for locked GNO claim button on wallet change

e4f1d48

nenadV91 requested review from W3stside and alfetopito April 29, 2022 12:47

alfetopito approved these changes Apr 29, 2022

View reviewed changes

nenadV91 merged commit 39e1867 into develop Apr 29, 2022

github-actions bot locked and limited conversation to collaborators Apr 29, 2022

alfetopito deleted the locked-gno-claim-2 branch May 2, 2022 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Locked gno claim 2 #405

Locked gno claim 2 #405

nenadV91 commented Apr 13, 2022 •

edited

Loading

github-actions bot commented Apr 13, 2022

github-actions bot commented Apr 13, 2022

elena-zh commented Apr 15, 2022 •

edited

Loading

alfetopito left a comment

alfetopito Apr 18, 2022

anxolin Apr 26, 2022

alfetopito Apr 18, 2022

alfetopito Apr 18, 2022

anxolin Apr 26, 2022

W3stside left a comment •

edited

Loading

elena-zh commented Apr 19, 2022

jfschwarz commented Apr 25, 2022 •

edited

Loading

jfschwarz commented Apr 25, 2022

elena-zh commented Apr 25, 2022

jfschwarz commented Apr 26, 2022

elena-zh commented Apr 26, 2022

elena-zh commented Apr 26, 2022

jfschwarz commented Apr 26, 2022 •

edited

Loading

elena-zh commented Apr 26, 2022 •

edited

Loading

nenadV91 commented Apr 26, 2022

anxolin left a comment

anxolin Apr 26, 2022

anxolin Apr 26, 2022

jfschwarz left a comment

jfschwarz Apr 27, 2022 •

edited

Loading

nenadV91 Apr 27, 2022

elena-zh commented Apr 28, 2022

elena-zh commented Apr 28, 2022

elena-zh left a comment

alfetopito left a comment

Locked gno claim 2 #405

Locked gno claim 2 #405

Conversation

nenadV91 commented Apr 13, 2022 • edited Loading

Summary

github-actions bot commented Apr 13, 2022

github-actions bot commented Apr 13, 2022

elena-zh commented Apr 15, 2022 • edited Loading

alfetopito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

W3stside left a comment • edited Loading

Choose a reason for hiding this comment

elena-zh commented Apr 19, 2022

jfschwarz commented Apr 25, 2022 • edited Loading

jfschwarz commented Apr 25, 2022

elena-zh commented Apr 25, 2022

jfschwarz commented Apr 26, 2022

elena-zh commented Apr 26, 2022

elena-zh commented Apr 26, 2022

jfschwarz commented Apr 26, 2022 • edited Loading

elena-zh commented Apr 26, 2022 • edited Loading

nenadV91 commented Apr 26, 2022

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfschwarz left a comment

Choose a reason for hiding this comment

jfschwarz Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elena-zh commented Apr 28, 2022

elena-zh commented Apr 28, 2022

elena-zh left a comment

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

nenadV91 commented Apr 13, 2022 •

edited

Loading

elena-zh commented Apr 15, 2022 •

edited

Loading

W3stside left a comment •

edited

Loading

jfschwarz commented Apr 25, 2022 •

edited

Loading

jfschwarz commented Apr 26, 2022 •

edited

Loading

elena-zh commented Apr 26, 2022 •

edited

Loading

jfschwarz Apr 27, 2022 •

edited

Loading