Skip to content
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

Initial work to implement Sapphire snap connection #431

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Oct 9, 2024

fixes #389

This provides the decryption keys to snap.

Warning

If an RPC server pretends to implement the MetaMask snap protocol it could trick users into revealing the transaction encryption key.

For this reason, we have to explicitly enable Snap support in the dApp, by passing the enableSapphireSnap option.

Usage:

wrapEthereumProvider(window.ethereum, {enableSapphireSnap:true})

This must only be done if the dApp is sure that the provider it's connecting to is MetaMask.

TODO:

Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 1b3fab0
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/6708f72c25062e000805cb82

@CedarMist CedarMist force-pushed the CedarMist/sapphire-snap branch from 47c82fc to 9d74f0f Compare October 9, 2024 11:24
@CedarMist CedarMist self-assigned this Oct 9, 2024
@CedarMist CedarMist added client javascript Pull requests that update JavaScript code labels Oct 9, 2024
@buberdds buberdds self-assigned this Nov 6, 2024
@@ -43,6 +44,7 @@ export function isLegacyProvider<T extends object>(

export interface SapphireWrapOptions {
fetcher: KeyFetcher;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options object is optional. When provided, TS requires fetcher, but we should be able just to set enableSapphireSnap flag. As fetcher always defaults to new KeyFetcher() should it now be fetcher: KeyFetcher | undefined; ?

@@ -80,10 +82,10 @@ export function isWrappedEthereumProvider<P extends EIP2696_EthereumProvider>(
* @param options (optional) Re-use parameters from other providers
* @returns Sapphire wrapped provider
*/
export function wrapEthereumProvider<P extends EIP2696_EthereumProvider>(
export async function wrapEthereumProvider<P extends EIP2696_EthereumProvider>(
Copy link

@buberdds buberdds Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it affect all integrations? For example I don't see ether-v6 is relying on wrapEthereumProvider. It has it's own logic. Do we plan to apply the same code in a few places, or share some snap utils across packages?
I guess for ether-v6 we should call notifySapphireSnap right after https://github.com/oasisprotocol/sapphire-paratime/blob/main/integrations/ethers-v6/src/index.ts#L96

params: {
id: transactionData,
ephemeralSecretKey: hexlify(secretKey),
peerPublicKey: peerPublicKey.key,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hexlify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client javascript Pull requests that update JavaScript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sapphire-snap integration
2 participants