Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

finnian0826
Copy link
Contributor

@finnian0826 finnian0826 commented Dec 18, 2024

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

NEW Manual Testing

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-08.at.13.59.31.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-08.at.13.57.32.mp4

OLD 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

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)

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.10%. Comparing base (e72427d) to head (ec8c5d5).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6385      +/-   ##
==========================================
+ Coverage   89.09%   89.10%   +0.01%     
==========================================
  Files         736      737       +1     
  Lines       31838    31884      +46     
  Branches     6040     6052      +12     
==========================================
+ Hits        28366    28411      +45     
- Misses       3276     3425     +149     
+ Partials      196       48     -148     
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/components/InLineNotification.tsx 100.00% <100.00%> (ø)
src/earn/EarnEnterAmount.tsx 93.75% <100.00%> (-0.18%) ⬇️
src/viem/prepareTransactions.ts 99.09% <ø> (ø)

... and 67 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 e72427d...ec8c5d5. Read the comment docs.

Comment on lines 61 to 63
const feeCurrencySymbol =
prepareTransactionsResult.type === 'not-enough-balance-for-gas'
? prepareTransactionsResult.feeCurrencies[0].symbol
Copy link
Contributor Author

@finnian0826 finnian0826 Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@finnian0826 finnian0826 marked this pull request as ready for review December 31, 2024 19:11
src/components/GasFeeWarning.test.tsx Outdated Show resolved Hide resolved
src/analytics/Properties.tsx Outdated Show resolved Hide resolved
src/components/GasFeeWarning.tsx Outdated Show resolved Hide resolved
src/components/GasFeeWarning.tsx Outdated Show resolved Hide resolved
Copy link

emerge-tools bot commented Jan 2, 2025

📸 Snapshot Test

No snapshots generated

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

🛸 Powered by Emerge Tools

MuckT
MuckT previously approved these changes Jan 2, 2025
@@ -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`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`,

?

? prepareTransactionsResult.feeCurrencies[0]
: prepareTransactionsResult.feeCurrency

const flowToNotEnoughGasDescriptionString = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@satish-ravi satish-ravi Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 407 to 424
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())
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satish-ravi @MuckT if we had the CTA onPress function inside the warning component, then we would also have to use a generic analytics event, or add another prop for which analytics event should be used for the CTA press. Not sure which of those we would want to do?

import { Spacing } from 'src/styles/styles'
import { PreparedTransactionsResult } from 'src/viem/prepareTransactions'

export enum GasFeeWarningFlow {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you consider string literals over enum for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MuckT MuckT dismissed their stale review January 3, 2025 23:07

Additional feedback / changes

prepareTransactionsResult,
flow,
changeInputValueFn,
testIdPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required in order to disambiguate warnings? We should never have more than one of these warnings at once, no? Or is this just to add additional context to the test ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just to add additional context to the test ID to make it clearer in tests, but you're right there should never be more than one at once so it is not needed. I can remove if that would be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in d6b4c16

return false
}

const feeCurrency =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to determine which of these conditions in the nested ternarys below correspond to which cases from the Figma was a bit cumbersome, though not sure if there's a better way (e.g., switching over error type so we don't have to repeat the same flow/error checks multiple times. you could also define the cta action this way as well so as not to have to repeat the error check again in onPressCta)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I could make a switch case for each of the error types and define all of the relevant things within each case. That would make it easier to add new error types if they come up (as you mentioned in your other comment), but not sure what the default/fallback should be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript supports type safety to ensure that all possible cases are handled, as here, so you could use this as the contents of your default case. Conveniently, this will give us compile errors if we ever add a new error type without handling it here.

const feeCurrency =
prepareTransactionsResult.type === 'not-enough-balance-for-gas'
? prepareTransactionsResult.feeCurrencies[0]
: prepareTransactionsResult.feeCurrency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we'll ever add another error type/string to the transaction result object -- as it is now, we catch the need-decrease-spend-amount-for-gas case implicitly, but if we ever add anything else, this will break silently. it's probably not a big deal, but wonder if we should be more explicit about that check?

tokenSymbol: feeCurrency.symbol,
})
: t('gasFeeWarning.descriptionMaxAmount', {
context: flow,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the context set up correctly here? AFAIK the way that context is used is by defining translation strings like someString_CONTEXT1, someString_CONTEXT2 in the translation file, but currently you have the "context" strings as nested objects, which differs from the way other context strings are used within this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yeah I didn't change that one over, I will fix that now

Comment on lines 156 to 165
[AppEvents.gas_fee_warning_impression]: {
flow: GasFeeWarningFlow
errorType: 'need-decrease-spend-amount-for-gas' | 'not-enough-balance-for-gas'
tokenId: string
}
[AppEvents.gas_fee_warning_cta_press]: {
flow: GasFeeWarningFlow
errorType: 'need-decrease-spend-amount-for-gas' | 'not-enough-balance-for-gas'
tokenId: string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we include networkId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added for both events in c34fbe6

Comment on lines 2837 to 2840
"descriptionNotEnoughGas_Send": "Adjust the amount you're sending or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Swap": "Adjust the amount you're swapping or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Deposit": "Adjust the amount you're depositing or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Withdraw": "Adjust the amount you're withdrawing or add {{tokenSymbol}} to continue",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just talked to product earlier today about this, I don't think adjusting the amount will make a difference as this is triggered when the user doesn't have the minimum amount for gas. Not sure if the copy is finalized here, so we could do this in a follow up PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that linked design saying that there will not be a description, or has the copy just not been added yet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if its final. Would be good to check with product

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked with product and confirmed it should be empty, so removed and updated in 81a166d

}: {
prepareTransactionsResult?: PreparedTransactionsResult
flow: GasFeeWarningFlow
changeInputValueFn?: (amount: string) => void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onPressSmallerAmount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in c34fbe6

Comment on lines 45 to 52
const title =
flow === 'Dapp'
? t('gasFeeWarning.titleDapp')
: t('gasFeeWarning.title', { tokenSymbol: feeCurrency.symbol })
const description =
flow === 'Dapp'
? t('gasFeeWarning.descriptionDapp', { tokenSymbol: feeCurrency.symbol })
: prepareTransactionsResult.type === 'not-enough-balance-for-gas'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered using context for Dapp too and avoid this nested ternary? Don't feel strongly but you could also do something like the below, which might be more readable

const { title, description, ctaLabel } = useMemo(() => {
  if (dapp) {
    return { title, description, ctaLabel }
  } else if (notEnoughBalanceForGas) {
    return { title, description, ctaLabel }
  } else {
    return { title, description, ctaLabel }
  }
}, [deps])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 0459e45, used context for title, but used the if/else for description and ctaLabel since those already had context for some of the string so didn't want to try to mess with having double context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the useMemo had to be moved higher in the file and so has some weird checks, added a comment below about it

Comment on lines 74 to 82
prepareTransactionsResult.type === 'not-enough-balance-for-gas'
? navigate(Screens.FiatExchangeAmount, {
tokenId: feeCurrency.tokenId,
flow: CICOFlow.CashIn,
tokenSymbol: feeCurrency.symbol,
})
: changeInputValueFn
? changeInputValueFn(prepareTransactionsResult.decreasedSpendAmount.toString())
: null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use if over ternary here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in c34fbe6

}
}, [prepareTransactionsResult])

const { title, description, ctaLabel } = useMemo(() => {
Copy link
Contributor Author

@finnian0826 finnian0826 Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useMemo is kind of weird because it cannot be conditional (can't be after the return false), so has to do extra checks. Not sure if there is a better way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can return an empty object instead of explicitly null. And you can check the condition below (where the component returns false) to just check if title is undefined, so that way you don't need to repeat the preparedTransaction check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 81a166d

Comment on lines 94 to 95
} else {
onPressSmallerAmount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be an else if instead of an else then ternary. Or you can also use optional chaining operator e.g., onPressSmallerAmount?.(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 81a166d

}
}, [prepareTransactionsResult])

const { title, description, ctaLabel } = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can return an empty object instead of explicitly null. And you can check the condition below (where the component returns false) to just check if title is undefined, so that way you don't need to repeat the preparedTransaction check again.

Comment on lines 2837 to 2840
"descriptionNotEnoughGas_Send": "Adjust the amount you're sending or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Swap": "Adjust the amount you're swapping or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Deposit": "Adjust the amount you're depositing or add {{tokenSymbol}} to continue",
"descriptionNotEnoughGas_Withdraw": "Adjust the amount you're withdrawing or add {{tokenSymbol}} to continue",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if its final. Would be good to check with product

@@ -928,7 +928,7 @@ describe('EarnEnterAmount', () => {
})
})

it('should track analytics and navigate correctly when tapping cta to add gas', async () => {
it('should show gas warning error when prepareTransactionsResult is type not-enough-balance-for-gas, and tapping cta behaves as expected', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also add a test for the need-decrease-spend-amount-for-gas? since the cta action for that is set by EarnEnterAmount

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new unit test added in 81a166d

@@ -82,7 +82,7 @@ export function InLineNotification({
)}
<View style={styles.contentContainer}>
{!!title && <Text style={styles.titleText}>{title}</Text>}
<Text style={[styles.bodyText]}>{description}</Text>
{!!description && <Text style={[styles.bodyText]}>{description}</Text>}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed for spacing to be correct in the new case where there is no description. But is it okay for the view to possibly be empty? Or do I need to do something like (!title && !description) || !!description?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand your proposal. Would there even be a usage of this component with both title and description not set?

Copy link
Contributor

@satish-ravi satish-ravi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A w/ nits

Comment on lines +30 to +31
gas_fee_warning_impression = 'gas_fee_warning_impression',
gas_fee_warning_cta_press = 'gas_fee_warning_cta_press',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this to FeeEvents? I think the two events currently there may not even be used anymore though, so could just replace these with those.

return {}
}
const title = t('gasFeeWarning.title', {
context: flow === 'Dapp' ? 'Dapp' : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context: flow === 'Dapp' ? 'Dapp' : undefined,
context: flow

I think this should work the same, if a string for a value isn't found, it should fallback to the default one

@@ -82,7 +82,7 @@ export function InLineNotification({
)}
<View style={styles.contentContainer}>
{!!title && <Text style={styles.titleText}>{title}</Text>}
<Text style={[styles.bodyText]}>{description}</Text>
{!!description && <Text style={[styles.bodyText]}>{description}</Text>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand your proposal. Would there even be a usage of this component with both title and description not set?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants