Update proposal for SIP-20 to enable two-way communication between Snap and WebSockets #149
Conversation
This file contains 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
david0xd
commented
Oct 1, 2024
| @@ -1,22 +1,23 @@ | |||
| --- | |||
| sip: 20 | |||
| title: External data entry point | |||
| title: Advanced external communication and events | |||
There was a problem hiding this comment.
Feels like the name is not reflecting what this SIP really is anymore? Looking for advice to change it if possible. Updated with one of the ideas I had.
Mrtenz
reviewed
Oct 3, 2024
| status: Draft | ||
| author: David Drazic (@david0xd) | ||
| created: 2023-12-15 | ||
| --- | ||
|
|
||
| ## Abstract | ||
|
|
||
| This SIP proposes a new permission that enables Snaps to receive data from external sources. | ||
| This SIP proposes a new permission that enables Snaps to receive data from external sources and send data back to them. |
There was a problem hiding this comment.
Not all external sources necessarily support two-way communication.
Mrtenz
reviewed
Oct 3, 2024
Comment on lines
+15
to
+16
| The Snap can then do whatever it wants with the data. | ||
| The Snap can use RPC method to send data back to the external connection (e.g. websocket connection). |
There was a problem hiding this comment.
Suggested change
| The Snap can then do whatever it wants with the data. | |
| The Snap can use RPC method to send data back to the external connection (e.g. websocket connection). | |
| The Snap can then do whatever it wants with the data, and can send data back to the external source if supported (e.g., a WebSocket connection). |
Mrtenz
reviewed
Oct 3, 2024
|
|
||
| ## Motivation | ||
|
|
||
| Snaps are currently limited in their ability to receive data from external sources; they have to rely on user actions or cron jobs to fetch data, so they can't react to events in real time. Snaps also cannot use WebSocket connections to receive data from external sources, and are limited to HTTP requests. | ||
| Snaps are currently limited in their ability to both receive and send data from and to external sources; they have to rely on user actions or cron jobs to fetch data, so they can't react to events in real time. Snaps also cannot use WebSocket connections for bidirectional communication, and are limited to HTTP requests. |
There was a problem hiding this comment.
Suggested change
| Snaps are currently limited in their ability to both receive and send data from and to external sources; they have to rely on user actions or cron jobs to fetch data, so they can't react to events in real time. Snaps also cannot use WebSocket connections for bidirectional communication, and are limited to HTTP requests. | |
| Snaps are currently limited in their ability to communicate with external sources; they have to rely on user actions or cron jobs to fetch data, so they can't react to events in real time. Snaps also cannot use WebSocket connections for bidirectional communication, and are limited to HTTP requests. |
Mrtenz
reviewed
Oct 3, 2024
| @@ -111,6 +112,35 @@ The caveats MUST NOT have duplicate objects with the same `url` properties. | |||
|
|
|||
| The `url` MUST start with `wss://` which is a protocol indicator for secure WebSocket connection. | |||
|
|
|||
| ### RPC Methods | |||
|
|
|||
| This SIP introduces new RPC method for sending data to the external connections. | |||
There was a problem hiding this comment.
Suggested change
| This SIP introduces new RPC method for sending data to the external connections. | |
| This SIP introduces a new RPC method for sending data to the external connections (if supported). |
Mrtenz
reviewed
Oct 3, 2024
|
|
||
| Snap can use `snap_sendData` RPC method to send data specified within request params. | ||
| The proposed RPC method `snap_sendData` SHOULD be a restricted RPC method requiring user consent before usage via the permission system. The RPC method SHOULD only be available to Snaps. | ||
| Snap MUST specify destination which is URL identifier of an external connection (e.g. websocket). Destination MUST be exact match of some of the URLs defined in manifest's permission caveats within `endowment:external-data`. |
There was a problem hiding this comment.
In theory the same URL can be used twice, in which case it would be ambiguous which one to use.
Mrtenz
reviewed
Oct 3, 2024
| Snap MUST specify destination which is URL identifier of an external connection (e.g. websocket). Destination MUST be exact match of some of the URLs defined in manifest's permission caveats within `endowment:external-data`. | ||
|
|
||
| The `snap_sendData` JSON-RPC method takes an object as parameters, which has the following properties: | ||
| - `data` (required, `string` or `binary`): A data, which MUST NOT be empty. |
There was a problem hiding this comment.
Is this necessary when this is already specified by the endowment:external-data dataType?
Mrtenz
reviewed
Oct 3, 2024
| #### snap_sendData | ||
|
|
||
| Snap can use `snap_sendData` RPC method to send data specified within request params. | ||
| The proposed RPC method `snap_sendData` SHOULD be a restricted RPC method requiring user consent before usage via the permission system. The RPC method SHOULD only be available to Snaps. |
There was a problem hiding this comment.
I think we should just use endowment:external-data for this RPC method as well.
Mrtenz
reviewed
Oct 3, 2024
| } | ||
| ``` | ||
|
|
||
| The method returns a `boolean`, `true` in case of successful delivery of a message to the WebSocket, `false` if delivery was unsuccessful. |
There was a problem hiding this comment.
Should it throw an error instead if delivery failed?
Mrtenz
reviewed
Oct 3, 2024
Comment on lines
+177
to
+201
| /** | ||
| * A text message sent to a WebSocket. | ||
| * | ||
| * @property type - The type of the message. | ||
| * @property message - The message as a string. | ||
| * @property destination - The destination web socket URL as a string. | ||
| */ | ||
| type WebSocketOutgoingTextMessage = { | ||
| type: 'text'; | ||
| message: string; | ||
| destination: string; | ||
| }; | ||
|
|
||
| /** | ||
| * A binary message sent to a WebSocket. | ||
| * | ||
| * @property type - The type of the message. | ||
| * @property message - The message as an ArrayBuffer. | ||
| * @property destination - The destination web socket URL as a string. | ||
| */ | ||
| type WebSocketOutgoingBinaryMessage = { | ||
| type: 'binary'; | ||
| message: ArrayBuffer; | ||
| destination: string; | ||
| }; |
There was a problem hiding this comment.
This can be simplified:
type WebSocketIncomingMessage = WebSocketIncomingTextMessage | WebSocketIncomingBinaryMessage;
type WebSocketOutgoingMessage = WebSocketIncomingMessage & {
destination: string;
};
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is proposal to update SIP-20 in order to support two-way communication between Snap and WebSockets.
Fixes: MetaMask/snaps#2728