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.

Sign up for GitHub

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

Jump to bottom

Fix accountsChanged notification #1413

Merged
merged 3 commits into from
Mar 10, 2020
Merged

Fix accountsChanged notification #1413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e92b910
use rpc-engine notifications for accountsChanged
rekmarks Mar 10, 2020
078d13a
only notify current web page
rekmarks Mar 10, 2020
f8d6a1e
Merge branch 'develop' into accountsChanged-fix
estebanmino Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 51 additions & 12 deletions app/components/Views/BrowserTab/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ import { resemblesAddress } from '../../../util/address';
import createAsyncMiddleware from 'json-rpc-engine/src/createAsyncMiddleware';
import { ethErrors } from 'eth-json-rpc-errors';

const { HOMEPAGE_URL, USER_AGENT } = AppConstants;
const { HOMEPAGE_URL, USER_AGENT, NOTIFICATION_NAMES } = AppConstants;
const HOMEPAGE_HOST = 'home.metamask.io';

const styles = StyleSheet.create({
Expand Down Expand Up @@ -454,15 +454,34 @@ export class BrowserTab extends PureComponent {
webview: this.webview,
url,
getRpcMethodMiddleware: this.getRpcMethodMiddleware.bind(this),
shouldExposeAccounts: hostname => {
const { approvedHosts, privacyMode } = this.props;
return !privacyMode || approvedHosts[hostname];
},
isMainFrame
});
this.backgroundBridges.push(newBridge);
};

notifyConnection = (payload, hostname, restricted = true) => {
const { privacyMode, approvedHosts } = this.props;

// TODO:permissions move permissioning logic elsewhere
this.backgroundBridges.forEach(bridge => {
if (bridge.hostname === hostname && (!restricted || !privacyMode || approvedHosts[bridge.hostname])) {
bridge.sendNotification(payload);
}
});
};

notifyAllConnections = (payload, restricted = true) => {
const { privacyMode, approvedHosts } = this.props;
const { fullHostname } = this.state;

// TODO:permissions move permissioning logic elsewhere
this.backgroundBridges.forEach(bridge => {
if (bridge.hostname === fullHostname && (!privacyMode || !restricted || approvedHosts[bridge.hostname])) {
bridge.sendNotification(payload);
}
});
};

getRpcMethodMiddleware = ({ hostname }) =>
// all user facing RPC calls not implemented by the provider
createAsyncMiddleware(async (req, res, next) => {
Expand Down Expand Up @@ -492,11 +511,6 @@ export class BrowserTab extends PureComponent {

if (approved) {
res.result = [selectedAddress.toLowerCase()];
this.backgroundBridges.forEach(bridge => {
if (bridge.hostname === hostname) {
bridge.emit('update');
}
});
} else {
throw ethErrors.provider.userRejectedRequest('User denied account authorization.');
}
Expand Down Expand Up @@ -845,8 +859,12 @@ export class BrowserTab extends PureComponent {
}

componentDidUpdate(prevProps) {
const prevNavigation = prevProps.navigation;
const { navigation } = this.props;
const {
approvedHosts: prevApprovedHosts,
navigation: prevNavigation,
selectedAddress: prevSelectedAddress
} = prevProps;
const { approvedHosts, navigation, selectedAddress } = this.props;

// If tab wasn't activated and we detect an tab change
// we need to check if it's time to activate the tab
Expand All @@ -864,6 +882,27 @@ export class BrowserTab extends PureComponent {
this.loadUrl();
}
}

const numApprovedHosts = Object.keys(approvedHosts).length;
const prevNumApprovedHosts = Object.keys(prevApprovedHosts).length;
Copy link
Contributor

Choose a reason for hiding this comment

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

approvedHosts and prevApprovedHosts are always defined?

Copy link
Member Author

@rekmarks rekmarks Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, approvedHosts is set in the privacy reducer. Its initial state is {}, and the clear action resets it to that.


// this will happen if the approved hosts were cleared
if (numApprovedHosts === 0 && prevNumApprovedHosts > 0) {
this.notifyAllConnections(
{
method: NOTIFICATION_NAMES.accountsChanged,
result: []
},
false
); // notification should be sent regardless of approval status
}

if (numApprovedHosts > 0 && selectedAddress !== prevSelectedAddress) {
this.notifyAllConnections({
method: NOTIFICATION_NAMES.accountsChanged,
result: [selectedAddress]
});
}
}

componentWillUnmount() {
Expand Down
5 changes: 4 additions & 1 deletion app/core/AppConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,8 @@ export default {
INSTAPAY_GAS_PONDERATOR: 1.2,
USER_AGENT: Device.isAndroid()
? 'Mozilla/5.0 (Linux; Android 10; Android SDK built for x86 Build/OSM1.180201.023) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.92 Mobile Safari/537.36'
: 'Mozilla/5.0 (iPhone; CPU iPhone OS 13_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/76.0.3809.123 Mobile/15E148 Safari/605.1'
: 'Mozilla/5.0 (iPhone; CPU iPhone OS 13_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/76.0.3809.123 Mobile/15E148 Safari/605.1',
NOTIFICATION_NAMES: {
accountsChanged: 'wallet_accountsChanged'
}
};
28 changes: 13 additions & 15 deletions app/core/BackgroundBridge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,20 @@ class Port extends EventEmitter {
}

export class BackgroundBridge extends EventEmitter {
constructor({ webview, url, getRpcMethodMiddleware, shouldExposeAccounts, isMainFrame }) {
constructor({ webview, url, getRpcMethodMiddleware, isMainFrame }) {
super();
this.url = url;
this.hostname = new URL(url).hostname;
this.isMainFrame = isMainFrame;
this._webviewRef = webview && webview.current;

this.createMiddleware = getRpcMethodMiddleware;
this.shouldExposeAccounts = shouldExposeAccounts;
this.provider = Engine.context.NetworkController.provider;
this.blockTracker = this.provider._blockTracker;
this.port = new Port(this._webviewRef, isMainFrame);

this.engine = null;

const portStream = new MobilePortStream(this.port, url);
// setup multiplexing
const mux = setupMultiplex(portStream);
Expand Down Expand Up @@ -81,14 +82,14 @@ export class BackgroundBridge extends EventEmitter {
* @param {*} outStream - The stream to provide over.
*/
setupProviderConnection(outStream) {
const engine = this.setupProviderEngine();
this.engine = this.setupProviderEngine();

// setup connection
const providerStream = createEngineStream({ engine });
const providerStream = createEngineStream({ engine: this.engine });

pump(outStream, providerStream, outStream, err => {
// handle any middleware cleanup
engine._middleware.forEach(mid => {
this.engine._middleware.forEach(mid => {
if (mid.destroy && typeof mid.destroy === 'function') {
mid.destroy();
}
Expand Down Expand Up @@ -146,10 +147,7 @@ export class BackgroundBridge extends EventEmitter {
* @param {*} outStream - The stream to provide public config over.
*/
setupPublicConfig(outStream) {
const configStore = this.createPublicConfigStore({
// check the providerApprovalController's approvedOrigins
checkIsEnabled: () => this.shouldExposeAccounts(this.hostname)
});
const configStore = this.createPublicConfigStore();

const configStream = asStream(configStore);

Expand All @@ -166,19 +164,15 @@ export class BackgroundBridge extends EventEmitter {
* Constructor helper: initialize a public config store.
* This store is used to make some config info available to Dapps synchronously.
*/
createPublicConfigStore({ checkIsEnabled }) {
createPublicConfigStore() {
// subset of state for metamask inpage provider
const publicConfigStore = new ObservableStore();

const selectPublicState = ({ isUnlocked, selectedAddress, network }) => {
const isEnabled = checkIsEnabled();
const isReady = isUnlocked && isEnabled;
const selectPublicState = ({ isUnlocked, network }) => {
const networkType = Engine.context.NetworkController.state.provider.type;
const chainId = Object.keys(NetworkList).indexOf(networkType) > -1 && NetworkList[networkType].chainId;
const result = {
isUnlocked,
isEnabled,
selectedAddress: isReady ? selectedAddress.toLowerCase() : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

this wasn't necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was previously, but no longer. The changes I made match what we did on the extension, and we removed isEnabled and selectedAddress from the public config store.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have already removed both tbh, but overlooked it.

networkVersion: network,
chainId: chainId ? `0x${parseInt(chainId, 10).toString(16)}` : null
};
Expand All @@ -204,6 +198,10 @@ export class BackgroundBridge extends EventEmitter {
return publicConfigStore;
}

sendNotification(payload) {
this.engine && this.engine.emit('notification', payload);
}

/**
* The metamask-state of the various controllers, made available to the UI
*
Expand Down