feat: Add transaction hash to STX in analytics for opt-in users#506
Closed
httpJunkie wants to merge 5 commits intomainfrom
Closed
feat: Add transaction hash to STX in analytics for opt-in users#506httpJunkie wants to merge 5 commits intomainfrom
httpJunkie wants to merge 5 commits intomainfrom
Conversation
- Update Smart Transactions Controller to support tx hash in analytics when feature flag is enabled - Add tx hash to sensitiveProperties with proper type definitions - Only include hash when both remote feature flag is enabled and user has opted into MetaMetrics - Update tests to support new "required" parameters
- The second check for getParticipateInMetrics is redundant since MetaMetrics events would only be sent if the user has already opted in - The MetaMetrics system handles this permission check at a higher level before any events are even tracked - Our implementation didn't need to check it
Fix TypeScript error by properly converting hex string to `Uint8Array` for `TransactionFactory.fromSerializedData`. This maintains the same functionality while providing the correct type expected by the function. The change converts the hex string to bytes using `Buffer.from()` first, then creates a `Uint8Array` from those bytes, ensuring type safety without changing the underlying data.
Replaces unsafe `Buffer` usage with `hexToBytes` utility and adds proper string typing, ensuring reliable transaction hash generation for MetaMetrics events.
The getTxHash function has been enhanced to handle hex strings more robustly: - Normalizes case (upper/lower) before processing - Properly handles 0x-prefixed and unprefixed hex strings - Prevents double-prefix issues that could cause transaction failures This change makes the function more resilient to varying hex string formats that may come from different parts of the system, while maintaining consistent hash generation regardless of input format. Tests added to verify: - Prefix handling (with/without 0x) - Case consistency (upper/lower/mixed) - Hash determinism across different string representations
Contributor
Author
|
We did not need these changes for Privacy Policy and Transaction Hash work in related PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current State and Rationale for Change
Currently, the Smart Transactions (STX) Controller doesn't include transaction hash in analytics events, unlike regular transactions which have been updated to support this functionality. The Data team requires transaction hash in specific transaction events to improve analytics capabilities, and we need to extend this support to Smart Transactions for consistency.
Solution
This PR extends transaction hash analytics support to Smart Transactions (STX), completing the implementation started in TXL-714:
transactionsTxHashInAnalyticssensitivePropertiesin STX analytics eventsThe implementation extracts transaction hash from the
minedHashproperty in the STX status metadata, ensuring consistent behavior with regular transactions.Related Links
The corresponding PR in MetaMask Extension is:
MetaMask/metamask-extension#31048
Manual Testing Steps
We need to test MetaMask Extension with the changes from this PR for
smart-transactions-controller..manifest-overrides.json:{ "_flags": { "remoteFeatureFlags": { "transactionsTxHashInAnalytics": { "name": "transactionsTxHashInAnalytics", "value": true } } } }