-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update proposal for SIP-20 to enable two-way communication between Snap and WebSockets #149
base: main
Are you sure you want to change the base?
Conversation
@@ -1,22 +1,23 @@ | |||
--- | |||
sip: 20 | |||
title: External data entry point | |||
title: Advanced external communication and events |
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.
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.
SIPS/sip-20.md
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all external sources necessarily support two-way communication.
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.
Updated as suggested.
SIPS/sip-20.md
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
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.
Updated.
SIPS/sip-20.md
Outdated
|
||
## 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Updated.
SIPS/sip-20.md
Outdated
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
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.
Updated.
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory the same URL can be used twice, in which case it would be ambiguous which one to use.
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.
Should we disallow that in our validation steps or you think we need to design it in a different way? 🤔
SIPS/sip-20.md
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary when this is already specified by the endowment:external-data
dataType
?
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.
Hmmm, probably something needs to be mentioned about it 🤔
Made some changes to simplify it. Let me know if you have better idea, I can update.
SIPS/sip-20.md
Outdated
#### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just use endowment:external-data
for this RPC method as well.
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.
Simplified this specification for a little bit. Let me know if that works better.
SIPS/sip-20.md
Outdated
} | ||
``` | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it throw an error instead if delivery failed?
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.
Yea, good idea. Updated. I'm still thinking what should be the best to return on successful delivery 🤔
SIPS/sip-20.md
Outdated
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified:
type WebSocketIncomingMessage = WebSocketIncomingTextMessage | WebSocketIncomingBinaryMessage;
type WebSocketOutgoingMessage = WebSocketIncomingMessage & {
destination: string;
};
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. I tried to simplify it more by making names more generic. Let me know if you think something like that would work or if you have any other ideas.
This PR is proposal to update SIP-20 in order to support two-way communication between Snap and WebSockets.
Fixes: MetaMask/snaps#2728
Potentially might resolve: https://github.com/MetaMask/MetaMask-planning/issues/3665