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

Open
wants to merge 7 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})
 wrapEthersProvider(ethProvider, { enableSapphireSnap: true })

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

Next PRs handled by Sapphire team:

Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 8844dbe
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/678a6ea78fd0ba00082ec4e7

@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;
Copy link
Contributor

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
Contributor

@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,
Copy link
Contributor

Choose a reason for hiding this comment

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

hexlify

@buberdds buberdds force-pushed the CedarMist/sapphire-snap branch 3 times, most recently from 3d04a4c to 853e9b9 Compare January 17, 2025 09:24
public override readonly epoch: number | undefined;

private cipher: deoxysii.AEAD;
private key: Uint8Array; // Stored for curious users.
public secretKey: Uint8Array; // Stored for curious users.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not a key we need, added ephemeralkey.

@buberdds buberdds force-pushed the CedarMist/sapphire-snap branch 3 times, most recently from 29746d8 to bc5b8bf Compare January 17, 2025 14:26
@buberdds buberdds force-pushed the CedarMist/sapphire-snap branch from bc5b8bf to 8844dbe Compare January 17, 2025 14:52
@@ -99,7 +101,7 @@ export function wrapEthereumProvider<P extends EIP2696_EthereumProvider>(
// if we do this, don't then re-wrap the send() function
// only wrap the send() function if there was a request() function

const request = makeSapphireRequestFn(upstream, filled_options);
const request = await makeSapphireRequestFn(upstream, filled_options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have reverted a change of making it all async, because async part can be handled in proxy

@buberdds buberdds marked this pull request as ready for review January 17, 2025 15:06
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