Conversation
Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
| }; | ||
| export type WebSocketBinaryMessage = { | ||
| type: "binary"; | ||
| message: number[]; |
There was a problem hiding this comment.
Wondering if this should be a base64 string, since we use that in other places for binary data.
There was a problem hiding this comment.
What would be the reasoning? When I proposed this I was thinking we wanted to mirror the browser WebSocket API as much as possible
There was a problem hiding this comment.
Just consistency with other APIs. Even better if we could have a translation layer on the execution side turning it into Uint8Array before passing to the Snap, but 🤷
GuillaumeRx
requested review from
GuillaumeRx and
Mrtenz
and removed request for
GuillaumeRx
Co-authored-by: Daniel Rocha <68558152+danroc@users.noreply.github.com>
| #### Example | ||
| `id` - The unique identifier of the WebSocket connection associated with the event. | ||
|
|
||
| `origin` - The origin of the WebSocket connection. |
There was a problem hiding this comment.
What will be the origin in this context? The Snap ID?
There was a problem hiding this comment.
The WebSocket origin for easy access to what origin you have open, similar to https://developer.mozilla.org/en-US/docs/Web/API/MessageEvent/origin
There was a problem hiding this comment.
It's clear in the case of a MessageEvent:
... origin of the message emitter.
But in the case of a WebSocketOpenEvent event, the origin is the Snap. Is it correct?
There was a problem hiding this comment.
The idea is to just mirror that property across all events, so it would be the WebSocket origin for all cases.
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
|
LGTM but I cannot approve my own PR 😅 |
5 participants
Rewrites SIP-20 with a more narrow WebSockets scope, to be implemented in the very near future.
Closes https://github.com/MetaMask/MetaMask-planning/issues/3665