-
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
Adapt app for metamask integration extension #5216
base: master
Are you sure you want to change the base?
Changes from 7 commits
831bd80
2d5f9ee
22a7803
72b0f6d
3da5017
870155f
6ca0b6d
ec7ec25
731f9d7
100ce6d
49dcb67
114b178
9e991e8
24e78c3
2a5e72a
f74b901
a6cd8dd
1ef3ddb
62ea529
053b2a1
a22f56f
54aca19
87f54be
e18f1bd
77e34f9
14285f2
affbf9c
e49f10f
7f811f1
82a9c14
64b9eda
21588be
d68cd99
98def48
5367b72
a9d20aa
cae1b3d
c0745f9
ebc9883
0a0ce3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import type BN from 'bn.js'; | |
import type { InjectedExtension } from '@polkadot/extension-inject/types'; | ||
import type { ChainProperties, ChainType } from '@polkadot/types/interfaces'; | ||
import type { KeyringStore } from '@polkadot/ui-keyring/types'; | ||
import type { KeypairType } from '@polkadot/util-crypto/types'; | ||
import type { ApiProps, ApiState } from './types'; | ||
|
||
import React, { useContext, useEffect, useMemo, useState } from 'react'; | ||
|
@@ -40,6 +41,7 @@ interface InjectedAccountExt { | |
source: string; | ||
whenCreated: number; | ||
}; | ||
type: KeypairType | ||
} | ||
|
||
interface ChainData { | ||
|
@@ -82,13 +84,14 @@ async function getInjectedAccounts (injectedPromise: Promise<InjectedExtension[] | |
|
||
const accounts = await web3Accounts(); | ||
|
||
return accounts.map(({ address, meta }, whenCreated): InjectedAccountExt => ({ | ||
return accounts.map(({ address, meta, type }, whenCreated): InjectedAccountExt => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On line 85 above it should explicitly ask for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EDIT: Since we do filter on the type in the keyring anyway, it is most probably not needed, since we do have the same effect. (Probably just more descriptive always being explicit, i.e. no need to even pass these through unless needed) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what do you think I should do? Leave as is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhhh.... ok this links through to this comment as well polkadot-js/extension#566 (comment) In the keyring itself, with the injected load, we don't actually check the type at all, e.g. https://github.com/polkadot-js/ui/blob/master/packages/ui-keyring/src/Keyring.ts#L249-L260 - we just inject what is there. The best place to add a solution is either -
Would not add a solution inside the apps UI since there are a lot of extension-dapp users, and don't want them to have to make all the same changes. Anyway, we can test this once all ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the filter in the extension dapp (in web3accounts), so I will pass down the type flag here |
||
address, | ||
meta: { | ||
...meta, | ||
name: `${meta.name || 'unknown'} (${meta.source === 'polkadot-js' ? 'extension' : meta.source})`, | ||
whenCreated | ||
} | ||
}, | ||
type | ||
})); | ||
} catch (error) { | ||
console.error('web3Enable', error); | ||
|
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.
These will just need to drop when the extension is included.