Add extra fields to analytics for bridge and send #763
Conversation
|
📦 build.zip [updated at Apr 22, 1:27:59 PM UTC] |
src/ui/pages/SwapForm/SwapForm.tsx
Outdated
| const showPriceImpactWarning = | ||
| priceImpact?.kind === 'loss' && | ||
| (priceImpact.level === 'medium' || priceImpact.level === 'high'); | ||
| const priceImpactWarningVisible = priceImpact |
There was a problem hiding this comment.
Is it Boolean(priceImpact && isSignificantValueLoss(priceImpact))?
src/ui/pages/SwapForm/SwapForm.tsx
Outdated
| }), | ||
| quote: selectedQuote, | ||
| warningWasShown: priceImpactWarningVisible, | ||
| outputAmountColor: priceImpactWarningVisible ? 'red' : 'grey', |
There was a problem hiding this comment.
There is a case where we show red text but doesn't show warning.
Looks like this should be taken into account here
There was a problem hiding this comment.
hm I think you're right
There was a problem hiding this comment.
Also, there is an unfortunate detail. Because we should reflect UI state in the analytics, it looks like we need calculate warning text here, on this level, and pass it as a param to the component. Otherwise, we will get inconsistency one day :(
There was a problem hiding this comment.
there will be some "unavoidable" duplication (of course every duplication is avoidable at some cost :)) in SwapForm and BridgeForm "anyway", so I'm not sure if it is worth DRY'ing outputAmountColor. I mean the best thing I could do is to extract a function to calculate that outputAmountColor which is going to be called in SwapFrom and BridgeForm... but I'm not even sure if this extraction makes sense
There was a problem hiding this comment.
but I fixed the condition behind outputAmountColor ✅
| chain: spendChain, | ||
| }), | ||
| quote: selectedQuote, | ||
| warningWasShown: isHighPriceImpact, |
There was a problem hiding this comment.
We shouldn't show warning in bridge at all right now. So this field should be false.
| const isSignificantLoss = | ||
| priceImpact?.kind === 'loss' && | ||
| (priceImpact.level === 'medium' || priceImpact.level === 'high'); | ||
| const isSignificantLoss = priceImpact |
There was a problem hiding this comment.
| asset_name_received: toMaybeArr(incoming?.map(getAssetName)), | ||
| asset_address_sent: toMaybeArr(outgoing?.map(getAssetAddress)), | ||
| asset_address_received: toMaybeArr(incoming?.map(getAssetAddress)), | ||
| warning_was_shown: warningWasShown, |
There was a problem hiding this comment.
Do you send these fields for Approve transactions?
If I remember right, we still need send them, but with the default values
There was a problem hiding this comment.
By default values you mean
warning_was_shown: undefined,
output_amount_color: undefined,
or
warning_was_shown: false,
output_amount_color: 'grey',
?
There was a problem hiding this comment.
updated to
warning_was_shown: false,
output_amount_color: 'grey',
in case of approval tx ✅
No description provided.