-
-
Notifications
You must be signed in to change notification settings - Fork 255
feat: Adding requestId field to dapp confirmations #7415
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
base: main
Are you sure you want to change the base?
Conversation
| const securityAlertId = uuid(); | ||
| const validateSecurity = validateSecurityHook.bind(null, securityAlertId); | ||
|
|
||
| const requestId = String(req.id); |
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.
What if we use JsonRpcId type from @metamask/utils in the TransactionController types? As v2 JSONRpcEngine referring the same types https://github.com/MetaMask/core/blob/main/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts#L4
Then we wouldn't really need to cast here and just pass as is?
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.
String is I think more useful even later when we use it in the UI
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.
Sorry not sure I follow, what makes string to be useful and how's that related with UI?
It's a minor thing for sure, but am not completely convinced if casting here necessary. So I will defer this to @dan437
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.
Do we want to note additions in changelog on this package and transaction-controller?
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.
Good point, let me do that.
…into request_id_support
…into request_id_support
Explanation
The PR adds id from the JSON RPC request to the transaction meta object. Thus change is done at 2 places:
References
Related to #67890
Checklist
Note
Propagates the JSON-RPC request id into transaction metadata for single and batched flows, wiring through EIP-5792 middleware and TransactionController with updated types, tests, and changelogs.
requestIdtoTransactionMeta,TransactionBatchMeta,AddTransactionOptions, andTransactionBatchRequest.requestIdthroughaddTransaction,addTransactionBatch, and batch utilities (utils/batch.ts).req.idand pass asrequestIdtoaddTransaction/addTransactionBatchinprocessSendCalls.requestIdpropagation.requestIdacross both packages.Written by Cursor Bugbot for commit e854e88. This will update automatically on new commits. Configure here.