Warning claim too much eth #2256

alfetopito · 2022-01-22T02:21:39Z

Summary

Waterfalls into #2250

Show warning/error when using too much native currency to invest

Uses current gas estimation for 1 approval. Might need to adjust that for a higher amount

If it's a Smart contract wallet, show a warning and let user proceed.
If it's an EOA wallet, show an error and block from moving forward until amount is corrected.

Smart contract:

EOA:

To Test

With and EOA, load one account that has a ETH investment option
Fund the account with not enough ETH to cover the full investment
On approvals page, select to invest max

You'll see the error as you are using everything in your wallet

With a Smart contract wallet, load one account that has a ETH investment option
Fund the account with exactly the amount needed to cover the full investment (this step is only need if claiming on behalf of an EOA. If you have a Safe with claims do the same as step 2)

On approvals page, you should see the warning and be able to move forward

Background

Note: Color is of course dumb and doesn't quite work on light more.
Neither does the error color for that matter.
Counting on @biocom to make it look good

github-actions · 2022-01-22T02:31:55Z

🔭 GP Swap: CoW Protocol v2 Swap UI

fairlighteth · 2022-01-22T13:29:54Z

Nice, can definitely style it further:

Should we use the same ETH 'reservation' logic as with the wrap/unwrap? Believe we count there for 2 approvals at minimum? Probably 1 approval only is bleeding edge.

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

fairlighteth · 2022-01-22T13:32:56Z

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

@@ -284,7 +307,8 @@ export default function InvestOption({ approveData, claim, optionIndex }: Invest
            </label>
            <i>Receive: {formatSmartLocaleAware(vCowAmount, AMOUNT_PRECISION) || 0} vCOW</i>
            {/* Insufficient balance validation error */}
-            {inputError ? <small>{inputError}</small> : ''}
+            {inputError && <small>{inputError}</small>}
+            {inputWarning && <small className="warn">{inputWarning}</small>}


Will probably refactor to pass a prop instead of applying a className.

anxolin

Looks great to me, and worked.
I guess it could have more testing in mainnet, cause in Rinkeby is so cheap that even though i tested very near the limit, i was not getting the warning (i assume 10 tx are cheaper than the marginal Ether i had)

When i went slightly less, i saw the error.
Nice introduction of not blocking WARN

anxolin · 2022-01-22T13:20:12Z

src/custom/components/swap/EthWethWrap/helpers.ts


  // USER_BALANCE - (USER_WRAP_AMT + 1_TX_CST) / 1_TX_COST = AVAILABLE_TXS
  const txsAvailable = nativeBalance.subtract(nativeInput.add(singleTxCost)).divide(singleTxCost)
  return txsAvailable.lessThan('1') ? null : txsAvailable.toSignificant(1)
 }
+
+export function _estimateTxCost(gasPrice: ReturnType<typeof useGasPrices>, native: Currency | undefined) {


i think is a bit strange to expose a function starting with an underscore. I assume this was a "private function" convention. If it's useful to expose, maybe should be without?

Also, not sure if this should live in a more generic place instead of inside swap -> wrapEth

anxolin · 2022-01-22T13:24:06Z

src/custom/components/swap/EthWethWrap/helpers.ts

+  const amount = BigNumber.from(gas).mul(MINIMUM_TXS).mul(AVG_APPROVE_COST_GWEI)
+
+  return {
+    multiTxCost: CurrencyAmount.fromRawAmount(native, amount.toString()),


nit: I felt the name was a bit hard to understand. What about recomendedMarginalEther? i also don't love it. Maybe you find a better one

I'm just moving this fn around.
If you want to complain about the names bother @W3stside =P

But ok... I'll refactor this next week when I'm more in the mood.

its just a nit, the PR was approved :)

i wrote this? 😂

anxolin · 2022-01-22T13:25:46Z

src/custom/pages/Claim/styled.ts

@@ -935,6 +935,10 @@ export const InvestInput = styled.span`
    color: red;
    margin: 12px 0;
    font-size: 15px;
+
+    &.warn {
+      color: orange;


@biocom might want to review colors for validations

anxolin · 2022-01-22T13:37:44Z

One question though, right now, if we use max investment it will be use not the max investment but the max investment given your balance (judging by this screenshot):

It might be OK since we show the % already, but curious on @biocom, @alfetopito opinions

fairlighteth · 2022-01-22T13:46:42Z

One question though, right now, if we use max investment it will be use not the max investment but the max investment given your balance (judging by this screenshot):

@anxolin Yes, at the end of the day, the question is if the label invest max possible covers the expectation that it will use a combination of your balance and the maximum amount you're eligible to invest.

My assumption is, that a majority will over-read this, and take it as a 'use my max balance', based on expected UX of using trading/DeFi UI's in general.

But perhaps as you pointed out, given the Available investment used is in place, one might figure out quickly that:

The majority/all of their balance is being used
Only say, 63% of the available investment is actually utilized.

Something to do a few user test runs with?

anxolin · 2022-01-22T17:32:55Z

Yes @biocom , i useed it more and more, and I like this approach very much. The progress bar makes it intuitive in my oppinion. The user might understand quicly that they only have balance to cover 63% of their opportunity.

alfetopito · 2022-01-22T19:35:16Z

One question though, right now, if we use max investment it will be use not the max investment but the max investment given your balance (judging by this screenshot):

I believe this is the right behavior because doesn't make sense to have a button that fills in incorrect data.

If we set it to fill is all investment opportunity but you have no balance to cover it, you can't proceed.
If we set it to fill in all of user's balance but that is more than what you can invest, you can't proceed.

The way it's implemented makes sure you fill in everything you can.

alfetopito · 2022-01-22T19:43:26Z

Nice, can definitely style it further:

* Should we use the same ETH 'reservation' logic as with the wrap/unwrap? Believe we count there for 2 approvals at minimum? Probably 1 approval only is bleeding edge.

One reason we need to find out how much gas we need for this transfer because it'll be blocking for EOAs.

Also, imagine this is not a wallet you regularly use and you only deposited on it enough to cover the investment, but forgot about the gas. You'll have to either deposit more or invest a tiny bit less to succeed.

I agree the gas for 1 approval would be much cheaper than claiming, depending on how many claims we are doing, but we'd need some experimentation on mainnet to find the best spot

anxolin · 2022-01-23T17:37:19Z

Yep, i think we all agree then! Happy to get this merged

W3stside · 2022-01-24T11:44:04Z

src/custom/components/swap/EthWethWrap/helpers.ts

 }) => {
-  if (!nativeBalance || !nativeInput || nativeBalance.lessThan(nativeInput.add(singleTxCost))) return null
+  if (!nativeBalance || !nativeInput || !singleTxCost || nativeBalance.lessThan(nativeInput.add(singleTxCost))) {


can you cache the last nativeBalance check?

elena-zh · 2022-01-24T14:30:04Z

LGTM besides this rounding issue: #2276

When smart wallet account, show warning

alfetopito · 2022-01-24T17:45:21Z

As discussed, I'll address the comments in a follow up PR.
Merging as is for now

alfetopito self-assigned this Jan 22, 2022

alfetopito requested review from a team January 22, 2022 02:21

fairlighteth reviewed Jan 22, 2022

View reviewed changes

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

fairlighteth reviewed Jan 22, 2022

View reviewed changes

fairlighteth approved these changes Jan 22, 2022

View reviewed changes

anxolin approved these changes Jan 22, 2022

View reviewed changes

Base automatically changed from disable-claim-review-on-zero-or-not-approved to develop January 22, 2022 18:06

W3stside reviewed Jan 24, 2022

View reviewed changes

elena-zh approved these changes Jan 24, 2022

View reviewed changes

Leandro added 5 commits January 24, 2022 09:25

Removed no longer used const regarding balance threshold

c11c224

Refactored gas price estimation and related helpers

3a646ec

When EOA account using all native funds, show error

3d278f4

When smart wallet account, show warning

Coloring warning message organge

bff3f6d

Fix typo on comment

5ee6163

alfetopito force-pushed the warning-claim-too-much-eth branch from 4052480 to 5ee6163 Compare January 24, 2022 17:27

alfetopito added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Jan 24, 2022

mergify bot merged commit b1335dc into develop Jan 24, 2022

alfetopito mentioned this pull request Jan 24, 2022

Pr2256/follow up #2279

Merged

alfetopito deleted the warning-claim-too-much-eth branch January 24, 2022 23:02

alfetopito mentioned this pull request Jan 25, 2022

[RELEASE ⛰️ ] v1.10.0-RC.0 #2289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning claim too much eth #2256

Warning claim too much eth #2256

alfetopito commented Jan 22, 2022

github-actions bot commented Jan 22, 2022

fairlighteth commented Jan 22, 2022

fairlighteth Jan 22, 2022

anxolin left a comment

anxolin Jan 22, 2022

anxolin Jan 22, 2022

anxolin Jan 22, 2022

alfetopito Jan 22, 2022

anxolin Jan 23, 2022

W3stside Jan 24, 2022

anxolin Jan 22, 2022

anxolin commented Jan 22, 2022

fairlighteth commented Jan 22, 2022

anxolin commented Jan 22, 2022

alfetopito commented Jan 22, 2022

alfetopito commented Jan 22, 2022

anxolin commented Jan 23, 2022

W3stside Jan 24, 2022

elena-zh commented Jan 24, 2022

alfetopito commented Jan 24, 2022

Warning claim too much eth #2256

Warning claim too much eth #2256

Conversation

alfetopito commented Jan 22, 2022

Summary

Smart contract:

EOA:

To Test

Background

github-actions bot commented Jan 22, 2022

fairlighteth commented Jan 22, 2022

Choose a reason for hiding this comment

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

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

anxolin commented Jan 22, 2022

fairlighteth commented Jan 22, 2022

anxolin commented Jan 22, 2022

alfetopito commented Jan 22, 2022

alfetopito commented Jan 22, 2022

anxolin commented Jan 23, 2022

Choose a reason for hiding this comment

elena-zh commented Jan 24, 2022

alfetopito commented Jan 24, 2022