feat: Adding code to capture metrics for dapp swaps #36885
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations (10 files, +955 -18)
|
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| const commands = parsedTransactionData.args.commands as string; |
There was a problem hiding this comment.
However we extract the necessary data, should all the Uniswap logic remain encapsulated in app/scripts/lib/transaction/decode/uniswap.ts?
Maybe with a new getUniswapSwapData function?
There was a problem hiding this comment.
This is not standard parsing of abi data and will not be re-used I think. Additionally putting data in uniswap.ts will then need for a controller action to be called from the client.
ui/pages/confirmations/components/confirm/info/hooks/useDappSwapComparisonInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
| quotes[selectedQuoteIndex].quote.minDestTokenAmount, | ||
| 10, | ||
| ) | ||
| .minus(new Numeric(amountMin, 10)) |
There was a problem hiding this comment.
Is it best we provide slippage in our requests to fetchQuotes to be explicit?
Maybe we could control via feature flag?
As that will influence our minimums.
There was a problem hiding this comment.
@bschorchit : do you have a suggestion for slippage amount ?
There was a problem hiding this comment.
If we don't pass a slippage, does it uses the our swaps defaults?
I believe they will be in a better position to define the ideal slippage than us as ideal slippage varies based on the token pair being swapped.
A feature flag for this doesn't seem ideal as we would have to accomodate logic for specific token pairs there as well.
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useUniswapShieldInfo.ts
Outdated
Show resolved
Hide resolved
| percentage_change_in_token_amount: percentageChangeInTokenAmount, | ||
| percentage_change_in_token_min_amount: | ||
| percentageChangeInTokenMinAmount, | ||
| percentage_change_in_gas: percentageChangeInGas, |
There was a problem hiding this comment.
Are percentages sufficient for the metrics?
If we got loads of positive results, of higher increases, they may only amount to pennies, so should we also calculate fiat values for the total gas, amount changes, and total difference?
There was a problem hiding this comment.
@bschorchit : can you please detail what all field will be useful in metrics.
There was a problem hiding this comment.
We should get the usd values instead of percentages. We can calculate percentages later within Mixpanel.
Note: these should be usd values, not user preferred fiat values.
Properties proposed are the following:
swap_dapp_from_token_simulated_value_usd - $ usd simulated value for the sending token for the Uniswap swap
swap_dapp_to_token_simulated_value_usd - $ usd simulated value for the receiving token for the Uniswap swap
swap_dapp_minimum_received_value_usd - $ usd minimum value received for the receiving token for the Uniswap swap
swap_dapp_network_fee_usd - $ usd value for the total cost of that transaction. If it's a batch transaction that includes approve transactions, those should accounted too as part of the network fee.
swap_mm_from_token_simulated_value_usd - $ usd simulated value for the sending token for the MM swap
swap_mm_to_token_simulated_value_usd - $ usd simulated value for the receiving token for the MM swap
swap_mm_minimum_received_value_usd - $ usd minimum value received for the receiving token for the MM swap
swap_mm_network_fee - $ usd value for the total cost of that transaction. It should account for the approve transaction as well, if applicable and possible.
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [c50cf29]
UI Startup Metrics (1137 ± 83 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| this.controllerMessenger, | ||
| `${BRIDGE_CONTROLLER_NAME}:${BridgeBackgroundAction.TRACK_METAMETRICS_EVENT}`, | ||
| ), | ||
| [BridgeBackgroundAction.FETCH_QUOTES]: this.controllerMessenger.call.bind( |
There was a problem hiding this comment.
Ideally this would go in the bridge-controller-init file in the api property.
jpuri
changed the title
jpuri
changed the title
jpuri
force-pushed
the
uni_shield
branch
2 times, most recently
from
02c4771 to
c060e20
Compare
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [c6e3040]
UI Startup Metrics (1243 ± 75 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| } | ||
| }); | ||
|
|
||
| return selectedQuoteIndex; |
There was a problem hiding this comment.
use use the quote with highest value of minDestTokenAmount
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [e8c1115]
UI Startup Metrics (1208 ± 82 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
❌ test-e2e-chrome-api-specs failed. View the html report here. |
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [7333da1]
UI Startup Metrics (1230 ± 67 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
With the new folder structure we're using, should this just be in ui/pages/confirmations/components/dapp-swap-comparison-banner to avoid all the nesting inside /confirm?
There was a problem hiding this comment.
I moved it from /info to /confirm, currently all confirmation related component are there.
|
|
||
| return ( | ||
| <> | ||
| {transactionMeta.origin === DAPP_SWAP_COMPARISON_ORIGIN && ( |
There was a problem hiding this comment.
Is it cleaner to encapsulate this origin knowledge inside the DappSwapComparisonBanner itself so it can handle its own conditional rendering?
There was a problem hiding this comment.
The reason I put this condition here is that even if I keep it inside DappSwapComparisonBanner we will end up running few hooks like get decimal places for erc20, get USD rates, etc for a lot of confirmationations.
By keeping this condition here all that code execution is by-passed.
ui/pages/confirmations/components/confirm/info/dapp-swap-comparison-utils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Would this be better in shared/modules/ or shared/lib? to decouple it from the UI?
There was a problem hiding this comment.
I places it here ui/pages/confirmations/utils/dapp-swap-comparison-utils.ts it has methods which I do not think will be useful outside the hook useDappSwapComparisonInfo.
ui/pages/confirmations/components/confirm/info/hooks/useDappSwapComparisonInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/hooks/transactions/useDappSwapComparisonInfo.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/hooks/transactions/useDappSwapComparisonInfo.ts
Outdated
Show resolved
Hide resolved
| gasIncluded7702: false, | ||
| }; | ||
| } else { | ||
| const seaportIndex = commandBytes.findIndex( |
There was a problem hiding this comment.
Is this exhaustive?
Do we need to handle TRANSFER, PAY_PORTION, V2_SWAP_EXACT_OUT, or V3_SWAP_EXACT_OUT?
There was a problem hiding this comment.
These 2 command are able to provide us all the data we need to get quotes.
|
|
||
| jest.mock('../../hooks/useDappSwapComparisonInfo', () => ({ | ||
| useDappSwapComparisonInfo: jest.fn(() => undefined), | ||
| })); |
There was a problem hiding this comment.
Bug: Mock Path Mismatch Causes Test Ineffectiveness
The jest.mock path for useDappSwapComparisonInfo (../../hooks/useDappSwapComparisonInfo) doesn't match the component's actual import path (../../../hooks/transactions/useDappSwapComparisonInfo). This prevents the mock from working, causing the test to use the real implementation and making it ineffective.
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [8dba70b]
UI Startup Metrics (1295 ± 79 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
4 participants
Description
Capturing DAPP Swap metrics
Changelog
CHANGELOG entry:
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/5974
Manual testing steps
Screenshots/Recordings
NA
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds a hidden banner and hook to collect dapp swap comparison metrics (for Uniswap origin), parsing swap tx data, fetching quotes via new background action, and reuses a new token decimals helper, with comprehensive tests.
DappSwapComparisonBanner(non-visual) that invokesuseDappSwapComparisonInfo; conditionally rendered inbase-transaction-infofor originhttps://app.uniswap.org.useDappSwapComparisonInfoto parse swap tx data, fetch quotes, compute USD values, and capture metrics.dapp-swap-comparison-utils(parse sweep/seaport data, pick best quote, map token values, read simulated balance changes).fetchAllErc20Decimalsinutils/tokenand use it inuseBalanceChanges.${BRIDGE_CONTROLLER_NAME}:FETCH_QUOTES; addfetchQuotesstore action calling backgroundfetchQuotes.Written by Cursor Bugbot for commit 8dba70b. This will update automatically on new commits. Configure here.