-
-
Notifications
You must be signed in to change notification settings - Fork 267
feat(transactionMeta): add feature in metadata #7726
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
Conversation
5e80591 to
01200b2
Compare
01200b2 to
d618c6f
Compare
d618c6f to
65c2565
Compare
65c2565 to
8576881
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
| /** | ||
| * Feature identifier transaction metadata. | ||
| */ | ||
| export enum Feature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this is conceptually a duplication of the existing TransactionType which has almost the exact same purpose.
Can we not already infer all this from the existing transaction types and origin?
We also already support nested transaction types in batches that we can also include in the metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the same feeling but as I understand the ask the list of Feature matches what we want to report for the IPO in term of user facing features while the current type is used in a more granular, somewhat related to the transaction implementation use case way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it's still duplicated data, and we could just generate this in the report itself?
Or just generate this data in the metrics in the clients using the transaction types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll spend some time today seeing what I can achieve with origin and type. From my perspective not having a new feature property would be indeed better but I am not sure how far we can move away from the feature list the data team wrote down in case some features cannot be derived. cc. @klejeune for visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two different purposes. They may be compatible with TxType, but they are still two different ones:
- Mapping the transactions to the types listed here.
- Reusing the somehow more technical transaction type
Having everything in TxType, if possible, would be good enough, if we can accurately define the "feature" part. I see two potential issues:
- A txType defined by a team/experience (ex: Send), reused by another team not aware they need to define a new txType for themselves (ex: the Send txType in the context of the Ramp/Sell feature, which is not the same as the Send txType in the Send feature). MM Pay is also at risk of that, we need to know if the funding is for Perps or Predictions.
- Each time a team/experience uses a new txType they didn't use before, they need to declare a new txType if there is no feature field on the client. With a feature field, the feature is defined once for the experience, and all transactions are associated to it. Ex: the Ramps team implements a swap feature for some reason, they need to declare a new RampsSwap, RampsSwapApproval, possibly RampsSwapAndSend... Maybe the deprecated Predict txTypes were an early try at that?
If txType allows to tackle these issues, this is perfectly fine with me.
What do you think @matthewwalsh0 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made an assessment of the situation for Perps, Prediction, Earn and Card on mobile as they are the priorities for now
| Feature | Transaction Type | How It's Submitted | Type Set By | Sentinel receives tx_type? | Mitigation |
|---|---|---|---|---|---|
| Prediction | Deposit | via transaction-pay-controller |
predict_deposit by predictController then relayDeposit / approve (overwritten) by transaction-pay-controller |
❌ No (receives relay_deposit) | Modify transaction-pay-controller to either preserve original type (predict_deposit) or use feature field see proposal PR |
| Prediction | Withdraw | predict_withdraw by predictController and sent to transaction-controller (no STX on Polygon) |
predict_withdraw |
❌ No | Enable STX for withdrawals |
| Perps | Deposit | via transaction-pay-controller |
perps_deposit by predictController then relayDeposit / approve (overwritten) by transaction-pay-controller |
❌ No (receives relay_deposit) | Modify transaction-pay-controller to either preserve original type or use feature field PR |
| Perps | Withdraw | Direct to Hyperliquid | N/A | ❌ No | None - bypasses MetaMask transaction flow |
| Perps | Long/Short Orders | Direct to Hyperliquid | N/A | ❌ No | None - bypasses MetaMask transaction flow |
| Earn | Deposit/Withdraw | Direct via transaction-controller |
lendingDeposit, stakingDeposit, etc. |
✅ Yes | None needed - type preserved ( |
| Card | Delegation/Approval | Direct via transaction-controller |
tokenMethodApprove (generic) |
Create dedicated card_approval type |
|
| Card | Add Funds (Swap/Bridge) | Via BridgeStatusController |
bridge / swap (generic) |
Add type and/or feature to BridgeRouteParams, flow through navigation → submitBridgeTx → BridgeStatusController (feature or more types needed if we want to propagate swap + card) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now focusing on perps/prediction deposit, as the teams already set the types properly, we can get the types in metadata by preserving it in the transaction-pay-controller(see proposal here: #7745). It does not impact the transaction routing AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach for Populating feature |
Pros | Cons |
|---|---|---|
Add explicit feature field |
Clear, unambiguous attribution; No derivation logic needed; Self-documenting; Features set their own identity | Requires propagating new field through all flows; More fields to maintain; Features must remember to set it, some semantic overlap with type and origin (dAppTransaction) |
Derive feature from type + origin |
No new field needed; Works with existing data; Centralized derivation logic | Ambiguous for generic types (swap, bridge); Need to extend origin or type anyway for ambiguous cases |
Pull request was closed
Explanation
The goal of this PR is to allow teams submitting transactions to set the feature property on the transaction metadata. This will enable finer grained monitoring and analytics.
Per-transaction feature always overrides. Batch feature only applies if transaction has no feature. Existing transaction's feature is preserved otherwise.
References
Checklist
Note
Introduces feature tagging to transactions and ensures correct propagation in batch flows.
Featureenum and optionalfeaturetoTransactionMeta,AddTransactionOptions,TransactionBatchRequest, andTransactionBatchSingleRequest; exportsFeatureTransactionController.addTransactionnow accepts/persistsfeaturebatch.ts,ExtraTransactionsPublishHook):featureoverrides batch-level and existing transaction valuesfeatureapplies only when a transaction has nofeaturefeatureunless a per-transaction value is provided; propagates during EIP-7702 conversion and publishWritten by Cursor Bugbot for commit 8576881. This will update automatically on new commits. Configure here.