feat: Add Buy Telemetry #1969
feat: Add Buy Telemetry #1969
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cpcramer
force-pushed
the
paul/add-buy-telemetry
branch
from
3f4fd2c to
fb12718
Compare
src/core/analytics/types.ts
Outdated
| amount?: number; | ||
| token?: string; | ||
| }; | ||
| [BuyEvent.BuyOptionSelected]: CommonAnalyticsData & { | ||
| option: BuyOption; | ||
| option?: BuyOption; | ||
| }; | ||
| [BuyEvent.BuySuccess]: CommonAnalyticsData & { | ||
| address: string; | ||
| amount: number; | ||
| from: string; | ||
| paymaster: boolean; | ||
| to: string; | ||
| transactionHash: string; | ||
| address?: string; | ||
| amount?: number; | ||
| from?: string; | ||
| paymaster?: boolean; | ||
| to?: string; | ||
| transactionHash?: string; |
There was a problem hiding this comment.
related to comment above, optional seems weird on these. if BuyOptionSelected, i'd expect it to be required that there's a BuyOption, no?
There was a problem hiding this comment.
My thinking here: It just helps with type errors and flexibility with how we are handling analytics. Sometimes things break and I don't ever want analytics to block. While i'd prefer all parameters to be passed, I'd still rather have some information about the event then no event at all.
There was a problem hiding this comment.
I think you should make these value optional and not key optional. That'll ensure that you know exactly what you should pass in the ideal state.
e.g.
address: string | undefined
amount number | undefined
...
There was a problem hiding this comment.
That's a great idea. I should actually refactor all event types to follow this pattern.
| @@ -415,6 +451,7 @@ export function BuyProvider({ | |||
| updateLifecycleStatus, | |||
| useAggregator, | |||
| walletCapabilities, | |||
| sendAnalytics, | |||
There was a problem hiding this comment.
This is a little weird to expose out of BuyProvider, the fact that we're returning it from multiple contexts suggests there are some design flaws with useAnalytics
There was a problem hiding this comment.
Update - We are not exposing sendAnalytics out of the BuyProvider here. In general, analytics could use a refactor to remain consistent across all components, reduce unnecessary memoization, minimize amount of code needed to support, etc .
There was a problem hiding this comment.
yep my bad, read this wrong! sorry
cpcramer
force-pushed
the
paul/add-buy-telemetry
branch
from
6dbfac0 to
aacc3a9
Compare
cpcramer
force-pushed
the
paul/add-buy-telemetry
branch
from
aacc3a9 to
a514291
Compare
What changed? Why?
Notes to reviewers
How has it been tested?