-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add Metamask Snap integration #8070
base: master
Are you sure you want to change the base?
Conversation
@jacogr is there anything else you need from us before running these workflows? This work was done as part of a W3F grant and it would be great to get it out there for the community! |
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.
@BeroBurny @danforbes thanks for your PR which is part of this delivery.
I added some comments, feel free to check them out.
export function injectExtensions (): Promise<boolean[]> { | ||
return Promise.all([ | ||
initPolkadotSnap | ||
].map((method) => { | ||
// Timeout injecting extension | ||
return Promise.race([ | ||
method(), | ||
new Promise<false>((resolve) => { | ||
setTimeout((): void => { | ||
resolve(false); | ||
}, 1000 /* 1 sec */); | ||
}) | ||
]); | ||
})); | ||
} |
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.
-
Why does the return time have to be
Promise<boolean[]>
? After all,initPolkadotSnap
is just returning a singlePromise<boolean>
. -
In general, this code could be simplified when using
rxjs
which already is a dependency:
export function injectExtensions (): Promise<boolean[]> { | |
return Promise.all([ | |
initPolkadotSnap | |
].map((method) => { | |
// Timeout injecting extension | |
return Promise.race([ | |
method(), | |
new Promise<false>((resolve) => { | |
setTimeout((): void => { | |
resolve(false); | |
}, 1000 /* 1 sec */); | |
}) | |
]); | |
})); | |
} | |
export const injectExtensions = () => | |
firstValueFrom(from(initPolkadotSnap()) | |
.pipe( | |
timeout(1000), // timeout if initPolkadotSnap() doesn't resolve after 1s | |
catchError(() => of(false)), | |
)); |
(Please note that I haven't actually tested this code)
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.
-
Idea is to make it future proof it there will be another wallet like MM snaps so users can easily add it to apps
for example@polkadot/extension-compat-metamask
you can just importinitMetaMask
and add it to the list to support it -
did the same solution with
RxJs
(personally for me is more complex as I'm uset to async/await js)
@BeroBurny / @danforbes, could you resolve the conflicts or resubmit, so this has a chance of being merged? |
Thank you for the feedback, @semuelle! I have informed @BeroBurny and @irubido that this Issue should take priority over the other one you recently commented on - please let me know if you disagree 🙏🏻 |
@semuelle - this PR should be up-to-date 🚀 Please let us know if there's anything else we can do to help the team evaluate things. We're getting started on support for parachains, etc now 💪🏻 |
Integrate Metamask Snaps Accounts into Polkadot JS Apps