Skip to content

Commit 5a3afaf

Browse files
OGPoyrazweitingsun
authored andcommitted
fix: Fix simulation total value to be settled in properties instead of sensitiveProperties (#20840)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR aims to change simulation total value property to be settled in `properties` instead of `sensitiveProperties`. The aim here is to have 0 `sensitiveProperties` in the transaction life cycle events so we wont have duplicated events. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: MetaMask/MetaMask-planning#5937 ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Moves simulation total value metrics into `properties`, removes `sensitiveProperties`, and updates tests accordingly. > > - **Metrics (useSimulationMetrics)** > - Move `simulation_receiving_assets_total_value` and `simulation_sending_assets_total_value` into `properties` via new `getTotalValueProperty`. > - Remove `sensitiveProperties` from params; delete `getSensitiveProperties` usage. > - Aggregate total values with existing asset metrics in `properties`. > - **Tests** > - Update expectations to assert total value under `properties` instead of `sensitiveProperties`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e497be3. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 6184055 commit 5a3afaf

2 files changed

Lines changed: 5 additions & 8 deletions

File tree

app/components/UI/SimulationDetails/useSimulationMetrics.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ describe('useSimulationMetrics', () => {
367367
balanceChanges: [balanceChange1, balanceChange2],
368368
},
369369
expect.objectContaining({
370-
sensitiveProperties: expect.objectContaining({
370+
properties: expect.objectContaining({
371371
[property]: 2.46,
372372
}),
373373
}),

app/components/UI/SimulationDetails/useSimulationMetrics.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,14 +90,11 @@ export function useSimulationMetrics({
9090
simulation_latency: simulationLatency,
9191
...getProperties(receivingAssets, 'simulation_receiving_assets_'),
9292
...getProperties(sendingAssets, 'simulation_sending_assets_'),
93+
...getTotalValueProperty(receivingAssets, 'simulation_receiving_assets_'),
94+
...getTotalValueProperty(sendingAssets, 'simulation_sending_assets_'),
9395
};
9496

95-
const sensitiveProperties = {
96-
...getSensitiveProperties(receivingAssets, 'simulation_receiving_assets_'),
97-
...getSensitiveProperties(sendingAssets, 'simulation_sending_assets_'),
98-
};
99-
100-
const params = { properties, sensitiveProperties };
97+
const params = { properties };
10198

10299
const shouldSkipMetrics =
103100
!enableMetrics ||
@@ -169,7 +166,7 @@ function getProperties(changes: BalanceChange[], prefix: string) {
169166
return getPrefixProperties({ quantity, type, value }, prefix);
170167
}
171168

172-
function getSensitiveProperties(changes: BalanceChange[], prefix: string) {
169+
function getTotalValueProperty(changes: BalanceChange[], prefix: string) {
173170
const fiatAmounts = changes.map((change) => change.usdAmount);
174171
const totalFiat = calculateTotalFiat(fiatAmounts);
175172
const totalValue = totalFiat ? totalFiat.abs().toNumber() : undefined;

0 commit comments

Comments
 (0)