Claim investment flow 3 #2200

nenadV91 · 2022-01-18T19:44:20Z

Summary

added claim input field functionality

W3stside · 2022-01-18T20:32:52Z

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

-
-  // divide maxValue with parsed value to get invest amount
-  return maxValue.multiply(parsedValue).asFraction
+enum ErrorMsgs {


can move this into types folder

mm, not sure. I would say that if this is only local to this component, is better to have it closer to the place you use.

It would be different if we reuse these in different components

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

alfetopito

Build didn't trigger so I could not test it in the PR.
I'll try it locally and report back

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

alfetopito · 2022-01-18T22:36:39Z

Looking at claiming on behalf of another account is working as expected

Claiming for yourself though, not so much

Getting divided by 0 error

When I type stuff into the input.
This account has no funds.

Also the percentage bar stays at 100%, probably due to the div by 0 error

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

W3stside · 2022-01-18T22:39:09Z

@nenadV91 I pushed directly to this branch recommended changes. sorry would normally PR but time is short.

alfetopito · 2022-01-18T22:44:34Z

Working nice after the last changes.

Still, there's this funny bug:

With no funds, press max amount whatever

Screen.Recording.2022-01-18.at.14.45.08.mov

It disables the input and sets bar to 100%

Probably missing:

Run validation on mount (to disable the field when no balance and set error message)
Set 100% to investment amount, not how much the user can invest like the max button (min(balance, investAmount))

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

W3stside · 2022-01-18T23:05:18Z

Working nice after the last changes.

Still, there's this funny bug:

With no funds, press max amount whatever

Screen.Recording.2022-01-18.at.14.45.08.mov
It disables the input and sets bar to 100%

Probably missing:

Run validation on mount (to disable the field when no balance and set error message)

Set 100% to investment amount, not how much the user can invest like the max button (min(balance, investAmount))

fixed in latest commit 3f94fb6

github-actions · 2022-01-18T23:07:30Z

🔭 GP Swap: Gnosis Protocol v2 Swap UI

…ultiply

* Added TODO about duplicated const * Fixing percentage calculation for max values when balance < maxCost * Refactored _calculatePercentage to return already the formatted % Co-authored-by: Leandro <[email protected]>

alfetopito

Ready to go!

elena-zh · 2022-01-19T09:58:25Z

Working nice after the last changes.

Still, there's this funny bug:

With no funds, press max amount whatever

Screen.Recording.2022-01-18.at.14.45.08.mov
It disables the input and sets bar to 100%

Probably missing:

Run validation on mount (to disable the field when no balance and set error message)

Set 100% to investment amount, not how much the user can invest like the max button (min(balance, investAmount))

Reported in #2201 , so I linked this issue to the PR

nenadV91 · 2022-01-19T10:57:01Z

@elena-zh This should be fixed now since the Invest max button will not show if the balance is 0

elena-zh · 2022-01-19T11:03:25Z

Just to clarify: won't we use percentage buttons on the 'Available investment used' bar?

elena-zh · 2022-01-19T11:04:53Z

Seems that there is a bit illogical validation message:

Moreover, it is not removed when I press on the Max button

nenadV91 · 2022-01-19T11:05:58Z

Just to clarify: won't we use percentage buttons on the 'Available investment used' bar?, at this point no.

elena-zh · 2022-01-19T11:12:11Z

I can't invest ETH

or USDC

anxolin · 2022-01-19T13:28:53Z

I can't invest ETH

@elena-zh This was introduced in another PR. is fixed now that @nenadV91 merged claim branch

anxolin

Good one.

For now, just code review. i will try to claim and use it

anxolin · 2022-01-19T13:33:43Z

src/custom/constants/index.ts

@@ -10,6 +10,7 @@ export const INITIAL_ALLOWED_SLIPPAGE_PERCENT = new Percent('5', '1000') // 0.5%
 export const RADIX_DECIMAL = 10
 export const RADIX_HEX = 16

+// TODO: remove, this is duplicated with `import { ONE_HUNDRED_PERCENT } from 'constants/misc'`
 export const ONE_HUNDRED_PERCENT = new Percent(1, 1)


good point, would be nice a follow up PR after

anxolin · 2022-01-19T13:35:37Z

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

-
-  // divide maxValue with parsed value to get invest amount
-  return maxValue.multiply(parsedValue).asFraction
+enum ErrorMsgs {


mm, not sure. I would say that if this is only local to this component, is better to have it closer to the place you use.

It would be different if we reuse these in different components

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx

elena-zh · 2022-01-19T14:36:40Z

Still I can't claim: See 'GNO' option:

USDC option is also not claimable.
ETH option is claimable.

anxolin · 2022-01-19T14:57:09Z

nitpic:

for another PR. If we select a bigger amount than available, shouldn't we leave the investment bar to 100%? it creates a weird effect if you increase the amount

Other than that, works great.

We will need to now make use of this values, cause we still invest 100% even if you reduce the amount.

alfetopito · 2022-01-19T16:35:16Z

The errors claiming investments in this PR are nothing introduced here, simply missing more thing to be implemented.

The amount selected in the inputs is not yet passed down to the contract.

The account I’m testing with has enough to cover 100% investment in GNO and ETH, but has only partial ~40% USDC.

When the contract call is made, it says I’ll invest 100% USDC but I don’t have that much.
Thus, the error.

No error with this PR, good to merge.
Will be fixed when the amounts are passed down to the claim callback

…low-3 Claim investment flow 3

nenadV91 changed the base branch from develop to claim January 18, 2022 19:44

W3stside reviewed Jan 18, 2022

View reviewed changes

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx Show resolved Hide resolved

alfetopito reviewed Jan 18, 2022

View reviewed changes

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx Outdated Show resolved Hide resolved

nenadV91 and others added 3 commits January 18, 2022 22:52

Added some input functionality

06d3330

update logic

29102e8

pr comments

6453e28

W3stside force-pushed the claim-investment-flow-3 branch from a9be98e to 6453e28 Compare January 18, 2022 22:54

alfetopito reviewed Jan 18, 2022

View reviewed changes

src/custom/pages/Claim/InvestmentFlow/InvestOption.tsx Outdated Show resolved Hide resolved

dont show max on noBalance

3f94fb6

Leandro added 4 commits January 18, 2022 15:27

Do not show max button when claiming for someone else

a9346cb

Fix vCOW amount calculation. Should be using price.quote instead of m…

8a63de2

…ultiply

Removed duplicated css rule

fab0bfe

Removed summary of investments from approve screen

9c5b8f6

alfetopito mentioned this pull request Jan 19, 2022

Fix claim percentages #2206

Merged

Leandro and others added 2 commits January 18, 2022 17:19

Removed unused import

f864cf2

Fix claim percentages (#2206)

b9338c0

* Added TODO about duplicated const * Fixing percentage calculation for max values when balance < maxCost * Refactored _calculatePercentage to return already the formatted % Co-authored-by: Leandro <[email protected]>

alfetopito approved these changes Jan 19, 2022

View reviewed changes

W3stside marked this pull request as ready for review January 19, 2022 09:24

nenadV91 added 3 commits January 19, 2022 12:41

Fix for error msg handling

c166a8d

Merge branch 'claim' into claim-investment-flow-3

b7727a9

Some style fixes

d15e7d1

anxolin reviewed Jan 19, 2022

View reviewed changes

Updated error msgs

50aebf1

Update amount parsing

a570d78

nenadV91 merged commit e96c340 into claim Jan 19, 2022

alfetopito deleted the claim-investment-flow-3 branch January 19, 2022 18:45

nenadV91 mentioned this pull request Jan 20, 2022

'Invest max positive' button sets 100% pn the progress bar when 0 tokens in the wallet balance #2201

Closed

anxolin pushed a commit to anxolin/cowswap that referenced this pull request Apr 1, 2022

Merge pull request gnosis/cowswap#2200 from gnosis/claim-investment-f…

2d2d0d4

…low-3 Claim investment flow 3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Claim investment flow 3 #2200

Claim investment flow 3 #2200

nenadV91 commented Jan 18, 2022

W3stside Jan 18, 2022

anxolin Jan 19, 2022

alfetopito left a comment

alfetopito commented Jan 18, 2022

W3stside commented Jan 18, 2022

alfetopito commented Jan 18, 2022 •

edited

Loading

W3stside commented Jan 18, 2022

github-actions bot commented Jan 18, 2022

alfetopito left a comment

elena-zh commented Jan 19, 2022 •

edited

Loading

nenadV91 commented Jan 19, 2022

elena-zh commented Jan 19, 2022

elena-zh commented Jan 19, 2022 •

edited

Loading

nenadV91 commented Jan 19, 2022

elena-zh commented Jan 19, 2022

anxolin commented Jan 19, 2022

anxolin left a comment

anxolin Jan 19, 2022

anxolin Jan 19, 2022

elena-zh commented Jan 19, 2022 •

edited

Loading

anxolin commented Jan 19, 2022

alfetopito commented Jan 19, 2022

Claim investment flow 3 #2200

Claim investment flow 3 #2200

Conversation

nenadV91 commented Jan 18, 2022

Summary

W3stside Jan 18, 2022

Choose a reason for hiding this comment

anxolin Jan 19, 2022

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

alfetopito commented Jan 18, 2022

W3stside commented Jan 18, 2022

alfetopito commented Jan 18, 2022 • edited Loading

W3stside commented Jan 18, 2022

github-actions bot commented Jan 18, 2022

alfetopito left a comment

Choose a reason for hiding this comment

elena-zh commented Jan 19, 2022 • edited Loading

nenadV91 commented Jan 19, 2022

elena-zh commented Jan 19, 2022

elena-zh commented Jan 19, 2022 • edited Loading

nenadV91 commented Jan 19, 2022

elena-zh commented Jan 19, 2022

anxolin commented Jan 19, 2022

anxolin left a comment

Choose a reason for hiding this comment

anxolin Jan 19, 2022

Choose a reason for hiding this comment

anxolin Jan 19, 2022

Choose a reason for hiding this comment

elena-zh commented Jan 19, 2022 • edited Loading

anxolin commented Jan 19, 2022

alfetopito commented Jan 19, 2022

alfetopito commented Jan 18, 2022 •

edited

Loading

elena-zh commented Jan 19, 2022 •

edited

Loading

elena-zh commented Jan 19, 2022 •

edited

Loading

elena-zh commented Jan 19, 2022 •

edited

Loading