Add filled form event #767
Conversation
|
📦 build.zip [updated at Apr 25, 4:28:02 PM UTC] |
| emitter.on('finalQuoteReceived', ({ quote, formView, scope }) => { | ||
| const params = createParams({ | ||
| request_name: 'swap_form_filled_out', | ||
| screen_name: scope === 'Swap' ? 'Swap' : 'Bridge', |
There was a problem hiding this comment.
I think, we can do
invariant(scope === 'Swap' || scope === 'Bridge')
And then
screen_name: scope,
client_scope: scope,
There was a problem hiding this comment.
thanks, yes, makes sense :) updated ✅
src/background/events.ts
Outdated
|
|
||
| export interface FinalQuoteReceivedParams { | ||
| quote: Quote; | ||
| formView: FormViewForAnalytics; |
There was a problem hiding this comment.
Because we went away from formView concept, we can rename this to formState?
| quantity: string | null; | ||
| } | ||
|
|
||
| function assetQuantityToValue( |
There was a problem hiding this comment.
We can reuse the same function from src/shared/analytics/shared/addressActionToAnalytics.ts
There was a problem hiding this comment.
Ah I see. It is slightly different. Well, then it is on you to decide what to so with it. May be it is ok to keep it this way
| isHighValueLoss, | ||
| } from 'src/ui/pages/SwapForm/shared/price-impact'; | ||
|
|
||
| function toMaybeArr<T>( |
There was a problem hiding this comment.
This one can be reused from src/shared/analytics/shared/addressActionToAnalytics.ts
| formView: FormViewForAnalytics; | ||
| quote: Quote; | ||
| }) { | ||
| const zerion_fee_percentage = quote.protocol_fee; |
There was a problem hiding this comment.
it looks like we can extract all parts of the formView here and don't use it below.
| }) { | ||
| const zerion_fee_percentage = quote.protocol_fee; | ||
| const feeAmount = quote.protocol_fee_amount; | ||
| const spendAsset = formView.spendAsset; |
There was a problem hiding this comment.
| usd_amount_received: toMaybeArr([usdAmountReceived]), | ||
| asset_amount_sent: toMaybeArr([quote.input_amount_estimation]), | ||
| asset_amount_received: toMaybeArr([quote.output_amount_estimation]), | ||
| asset_name_sent: toMaybeArr([formView.spendAsset?.name]), |
There was a problem hiding this comment.
| enough_balance, | ||
| enough_allowance: Boolean(quote.transaction), | ||
| warning_was_shown: isHighLoss, | ||
| // TODO add fdv_asset_sent |
There was a problem hiding this comment.
Should we add this in this PR?
Endpoint is ready to be used.
There was a problem hiding this comment.
added missing fields ✅
| request_name: 'swap_form_filled_out', | ||
| screen_name: scope, | ||
| client_scope: scope, | ||
| action_type: scope, |
There was a problem hiding this comment.
action_type can be Trade or Send 🫠🫠🫠
There was a problem hiding this comment.
in general it can, but not in formFilledOut event handler
so the scope should be of type scope: "Swap" | "Bridge" in FormFilledOutParams (if I'm not missing smth)
There was a problem hiding this comment.
I'll update this part in a minute
| fdv_asset_sent: fdvAssetSent, | ||
| fdv_asset_received: fdvAssetReceived, | ||
| bridge_fee_usd_amount: bridgeFeeAmountInUsd, | ||
| outputAmountColor, |
There was a problem hiding this comment.
output_amount_color: outputAmountColor
There was a problem hiding this comment.
Also something is not right with these conditions
There was a problem hiding this comment.
ah, thanks! it should be
const outputAmountColor =
priceImpact && isSignificantValueLoss(priceImpact) ? 'red' : 'grey';
updated ✅
| contract_type: quote.contract_metadata?.name, | ||
| enough_balance, | ||
| enough_allowance: Boolean(quote.transaction), | ||
| warning_was_shown: isHighPriceImpact, |
There was a problem hiding this comment.
Something is not right here
But I can't understand what exactly
There was a problem hiding this comment.
should be fixed, just checked locally ✅
| return { | ||
| usd_amount_sent: toMaybeArr([usdAmountSend]), | ||
| usd_amount_received: toMaybeArr([usdAmountReceived]), | ||
| asset_amount_sent: toMaybeArr([quote.input_amount_estimation]), |
There was a problem hiding this comment.
Do we need to convert these values? Need to be checked
There was a problem hiding this comment.
Right now it shows value in gwei, not in the token itself
There was a problem hiding this comment.
we convert values like these in addressActionToAnalytics so probably I should do the same here
| enabled?: boolean; | ||
| mapResponse?: (response: unknown) => T; | ||
| mergeResponse?(currentValue: T | null, nextValue: T | null): T | null; | ||
| onEnd?: (value: null | T) => void; |
There was a problem hiding this comment.
That way my first though as well, but ended up naming it onEnd to match the existing event name and the corresponding event handler
src/ui/shared/requests/useQuotes.ts
Outdated
| return updatedItem || item; | ||
| }); | ||
| }, | ||
| onEnd: (quotes) => emitter.emit('quotesReceived', quotes), |
There was a problem hiding this comment.
Can we add a param to useQuotes({ onEnd })?
To avoid event emitter here. Not sure is it important or not
There was a problem hiding this comment.
great idead, updated ✅
| `${url ?? 'no-url'}-${refetchHash}`, | ||
| url ?? null, | ||
| { | ||
| mergeResponse: (currentValue, nextValue) => { |
There was a problem hiding this comment.
Let's remove this param completely from useEventSource
No description provided.