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

chore: add estimated USD value to swap analytics events #4060

Merged
merged 4 commits into from
Aug 11, 2023
Merged

Conversation

silasbw
Copy link
Collaborator

@silasbw silasbw commented Aug 10, 2023

Test plan

Verify properties in analytics backends.

Copy link
Contributor

@dievazqu dievazqu left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -1257,13 +1257,15 @@ interface SwapEventsProperties {
fromTokenBalance: string
swapExecuteTxId: string
swapApproveTxId: string
estimatedUsdValue?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do you think it makes sense to include this field in the SwapQuoteEvent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, probably. I was going to chat with you about clarifying some of the other fields (e.g., .usdValue) before adding this one to SwapQuoteEvent (and by implication swap_review_submit).

src/swap/saga.ts Outdated
@@ -162,6 +181,7 @@ export function* swapSubmitSaga(action: PayloadAction<SwapInfo>) {
ValoraAnalytics.track(SwapEvents.swap_execute_success, {
...defaultSwapExecuteProps,
...timeMetrics,
estimatedUsdValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think you can include this value in the defaultSwapExecuteProps

src/swap/saga.ts Outdated
@@ -79,6 +94,10 @@ export function* swapSubmitSaga(action: PayloadAction<SwapInfo>) {
? fromToken.balance.shiftedBy(fromToken.decimals).toString()
: ''

const estimatedUsdValue = fromToken
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about having 2 fields, one for the estimated usd value for the selling token, and another for the buying token?

Something like:

const estimatedUsdValueSellingToken = calculateEstimatedUsdValue({ tokenInfo: fromToken, tokenAmount: sellAmount })
const estimatedUsdValueBuyingToken = calculateEstimatedUsdValue({ tokenInfo: toToken, tokenAmount: buyAmount })

In theory, fromToken, toToken, sellAmount and buyAmount will be defined. So I don't know if we need to check them.
If they aren't defined we will have errors in other parts as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #4060 (238d4e6) into main (829fde8) will decrease coverage by 0.02%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4060      +/-   ##
==========================================
- Coverage   83.24%   83.23%   -0.02%     
==========================================
  Files         718      718              
  Lines       26739    26752      +13     
  Branches     3634     3638       +4     
==========================================
+ Hits        22259    22266       +7     
- Misses       4417     4422       +5     
- Partials       63       64       +1     
Files Changed Coverage Δ
src/swap/saga.ts 92.59% <76.92%> (-2.15%) ⬇️

... and 1 file 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 829fde8...238d4e6. Read the comment docs.

@silasbw silasbw merged commit f706891 into main Aug 11, 2023
15 checks passed
@silasbw silasbw deleted the silasbw/usd0 branch August 11, 2023 18:47
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.

2 participants