feat(warning): Add generic gas fee warning component #6385

finnian0826 · 2024-12-18T18:22:12Z

Description

Generic gas warning create in GasFeeWarning.tsx. Used in EarnEnterAmount. For cases 2-5 rounded n is omitted, since it is difficult to find for cases 2-4, and in case 5 there is no cta so no immediate action to be taken (slack thread discussing here).

Analytics added for when the error shows, analytics for cta press are to be included in the onPressCta prop.

Test plan

EarnEnterAmount unit test updated
GasFeeWarning unit tests created

Manual Testing:
Not enough gas currency:
| | |

Transacting with max amount:
| | |

Dapp:

Tapping CTA:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-12-30.at.14.49.28.mp4

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-12-31.at.12.07.10.mp4

Related issues

Fixes ACT-1482

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-12-23T17:52:32Z

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.05%. Comparing base (8d74b49) to head (85dc50a).

Files with missing lines	Patch %	Lines
src/earn/EarnEnterAmount.tsx	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6385      +/-   ##
==========================================
- Coverage   89.05%   89.05%   -0.01%     
==========================================
  Files         735      736       +1     
  Lines       31350    31386      +36     
  Branches     5820     5835      +15     
==========================================
+ Hits        27919    27950      +31     
- Misses       3237     3242       +5     
  Partials      194      194

Files with missing lines	Coverage Δ
src/analytics/Events.tsx	`100.00% <100.00%> (ø)`
src/analytics/Properties.tsx	`100.00% <ø> (ø)`
src/components/GasFeeWarning.tsx	`100.00% <100.00%> (ø)`
src/viem/prepareTransactions.ts	`99.09% <ø> (ø)`
src/earn/EarnEnterAmount.tsx	`94.61% <85.71%> (-0.38%)`	⬇️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d74b49...85dc50a. Read the comment docs.

finnian0826 · 2024-12-31T18:59:17Z

src/components/GasFeeWarning.tsx

+  const feeCurrencySymbol =
+    prepareTransactionsResult.type === 'not-enough-balance-for-gas'
+      ? prepareTransactionsResult.feeCurrencies[0].symbol


In the designs, it seems like it leans towards hardcoding this to be CELO or ETH (this is cases 2-4), however in all of the use cases currently it is done like this (using prepareTransactionsResult.feeCurrencies[0]). I think it is easier to keep it this way for two reasons:

ETH tokenId is not available in NetworkConfig, and would be needed for CTA (in order to prefill in CICO flow), so would need to be added for all networks, increasing amount of hardcoded things

The CTA is implemented in the component that uses this warning, so the implementer would have to get the hardcoded tokenId based on the network of the prepared transaction, rather than just looking at the first feeCurrencies.

I can add in the ETH tokenId's if we want but wanted to check first about doing it this way instead.

locales/base/translation.json

src/components/GasFeeWarning.test.tsx

src/analytics/Properties.tsx

src/components/GasFeeWarning.tsx

emerge-tools · 2025-01-02T16:56:47Z

📸 Snapshot Test

No snapshots generated

Name	Added	Removed	Modified	Unchanged	Errored	Approval
Celo (test) org.celo.mobile.test	0	0	0	0	0	N/A

🛸 Powered by Emerge Tools

satish-ravi · 2025-01-02T16:53:29Z

src/analytics/docs.ts

@@ -67,6 +67,7 @@ export const eventDocs: Record<AnalyticsEventType, string> = {
  [AppEvents.in_app_review_impression]: `User sees an in-app review request`,
  [AppEvents.in_app_review_error]: `Error while attempting to display in-app review`,
  [AppEvents.handle_deeplink]: `When a deeplink that leads into the app is detected and handled`,
+  [AppEvents.show_gas_fee_warning]: `When the gas fee warning is shown to the user`,


Suggested change

[AppEvents.show_gas_fee_warning]: `When the gas fee warning is shown to the user`,

[AppEvents.gas_fee_warning_impression]: `When the gas fee warning is shown to the user`,

?

satish-ravi · 2025-01-02T16:56:36Z

src/components/GasFeeWarning.tsx

+      ? prepareTransactionsResult.feeCurrencies[0]
+      : prepareTransactionsResult.feeCurrency
+
+  const flowToNotEnoughGasDescriptionString = {


you can consider using context instead of doing this mapping yourself. E.g., the translation strings could be made something like gasFeeWarning.description.notEnoughGas_<GasFeeWarningFlow>, and then you can just do t('gasFeeWarning.description.notEnoughGas', { context: <GasFeeWarningFlow>, tokenSymbol })
There are other places in the app where we do this if you want a complete example

satish-ravi · 2025-01-02T17:04:16Z

src/components/GasFeeWarning.test.tsx

+    expect(queryByTestId('test/GasFeeWarning')).toBeFalsy()
+  })
+  it.each`
+    scenario                                      | flow                          | prepareTransactionsResult                  | feeCurrencyTokenId   | title                                            | description                                                                    | ctaLabel


I guess it doesn't hurt but whats the value of testing all the different variants with 2 different symbols? We don't seem to be doing anything custom depending on symbols

I was trying to have test coverage for all the cases outlined by product in the figma doc, and those cases include Celo and non-Celo chains so I had one of each

ok, I guess figma had those differences because celo is the only one with multiple gas tokens, we don't really handle that in this component, so it seems superfluous to test the variations.

satish-ravi · 2025-01-02T19:24:44Z

src/earn/EarnEnterAmount.tsx

+          onPressCta={() => {
+            AppAnalytics.track(EarnEvents.earn_deposit_add_gas_press, {
+              gasTokenId: feeCurrencies[0].tokenId,
+              depositTokenId: pool.dataProps.depositTokenId,
+              networkId: pool.networkId,
+              providerId: pool.appId,
+              poolId: pool.positionId,
+            })
+            if (prepareTransactionsResult && prepareTransactionsResult.type !== 'possible') {
+              prepareTransactionsResult.type === 'not-enough-balance-for-gas'
+                ? navigate(Screens.FiatExchangeAmount, {
+                    tokenId: prepareTransactionsResult.feeCurrencies[0].tokenId,
+                    flow: CICOFlow.CashIn,
+                    tokenSymbol: prepareTransactionsResult.feeCurrencies[0].symbol,
+                  })
+                : handleAmountInputChange(prepareTransactionsResult.decreasedSpendAmount.toString())
+            }
+          }}


Is this something that can be done in the warning component? Seems inconsistent to have the component define the CTA label, but the action is defined by the consumer. I assume this is going to be the same for all usages?

In the AC of the ticket it says that the component should have an optional action prop for handling the CTA, so I thought that meant to pass in an onPress function. But I agree I think it makes more sense for the CTA to exist within the component because "Buy CELO" should always do the same thing, regardless of flow. @MuckT was there a reason to pass in the CTA?

satish-ravi · 2025-01-02T19:25:04Z

src/components/GasFeeWarning.tsx

+import { Spacing } from 'src/styles/styles'
+import { PreparedTransactionsResult } from 'src/viem/prepareTransactions'
+
+export enum GasFeeWarningFlow {


did you consider string literals over enum for this?

no but I can change it to that, in Valora should we prefer string literals over enums? Or what is the convention?

Don't think we have a defined convention, but we did talk about this in an eng sync and I think we've been favoring literals over enums recently

finnian0826 added 2 commits December 18, 2024 11:21

feat(warning): Add generic gas fee warning component

6aa745d

add in description and cta label

fdd9e6f

finnian0826 added 6 commits December 23, 2024 14:21

implement, update unit tests

03940c0

fix hook issue

e0cb9ff

remove extra hook

9a13634

add unit tests

08d5cfe

cleanup

8df26f8

cleanup

5a252c9

finnian0826 commented Dec 31, 2024

View reviewed changes

finnian0826 added 2 commits December 31, 2024 12:04

reduce amoutn when gas token

0108485

clean up analytics property

e317b85

finnian0826 marked this pull request as ready for review December 31, 2024 19:11

finnian0826 added 2 commits January 1, 2025 08:12

Merge branch 'main' into finnian0826/act-1482

ef17d0b

fix merge

654409a

MuckT reviewed Dec 31, 2024

View reviewed changes

locales/base/translation.json Outdated Show resolved Hide resolved

MuckT reviewed Dec 31, 2024

View reviewed changes

src/components/GasFeeWarning.test.tsx Outdated Show resolved Hide resolved

src/analytics/Properties.tsx Outdated Show resolved Hide resolved

finnian0826 added 5 commits December 31, 2024 15:55

feedback pt. 1

7a33d2b

update property

0c6c76d

update strings

3f9a16d

update tests

425c77e

clean up

1224eec

MuckT reviewed Jan 1, 2025

View reviewed changes

src/components/GasFeeWarning.tsx Outdated Show resolved Hide resolved

src/components/GasFeeWarning.tsx Outdated Show resolved Hide resolved

feedback and fix tests

d2ba333

Merge branch 'main' into finnian0826/act-1482

51df96f

MuckT approved these changes Jan 2, 2025

View reviewed changes

satish-ravi reviewed Jan 2, 2025

View reviewed changes

finnian0826 added 2 commits January 2, 2025 12:42

rename analytics

1795348

feedback

21005b7

finnian0826 added 2 commits January 2, 2025 14:27

use string literal

8b3a671

remove superfluous tests

85dc50a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(warning): Add generic gas fee warning component #6385

feat(warning): Add generic gas fee warning component #6385

finnian0826 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

finnian0826 Dec 31, 2024 •

edited

Loading

emerge-tools bot commented Jan 2, 2025 •

edited

Loading

satish-ravi Jan 2, 2025

satish-ravi Jan 2, 2025

satish-ravi Jan 2, 2025 •

edited

Loading

finnian0826 Jan 2, 2025

satish-ravi Jan 2, 2025

satish-ravi Jan 2, 2025

finnian0826 Jan 2, 2025

satish-ravi Jan 2, 2025

finnian0826 Jan 2, 2025

satish-ravi Jan 2, 2025

	[AppEvents.show_gas_fee_warning]: `When the gas fee warning is shown to the user`,
	[AppEvents.gas_fee_warning_impression]: `When the gas fee warning is shown to the user`,

feat(warning): Add generic gas fee warning component #6385

Are you sure you want to change the base?

feat(warning): Add generic gas fee warning component #6385

Conversation

finnian0826 commented Dec 18, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

finnian0826 Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

emerge-tools bot commented Jan 2, 2025 • edited Loading

📸 Snapshot Test

No snapshots generated

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satish-ravi Jan 2, 2025 • edited Loading

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

finnian0826 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

finnian0826 Dec 31, 2024 •

edited

Loading

emerge-tools bot commented Jan 2, 2025 •

edited

Loading

satish-ravi Jan 2, 2025 •

edited

Loading