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

bugfix: fix dapp compat issues #1021

Merged
merged 4 commits into from
Aug 27, 2019
Merged

bugfix: fix dapp compat issues #1021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1c170ae
bugfix: fix dapp compat issues
bitpshr Aug 23, 2019
e685b86
feedback: move resemblesAddress to utility file
bitpshr Aug 23, 2019
3c2c98d
feedback: bump CI cache version
bitpshr Aug 23, 2019
6578438
Merge branch 'develop' into bugfix/dapp-compat
bitpshr Aug 27, 2019
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
12 changes: 6 additions & 6 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
aliases:
- &restore-cache
keys:
- v1.0.4-{{ .Branch }}-{{ checksum "package.json" }}
- v1.0.4-{{ .Branch }}
- v1.0.5-{{ .Branch }}-{{ checksum "package.json" }}
- v1.0.5-{{ .Branch }}

- &save-cache
key: v1.0.4-{{ .Branch }}-{{ checksum "package.json" }}
key: v1.0.5-{{ .Branch }}-{{ checksum "package.json" }}
paths:
- node_modules
- &restore-node-cache
keys:
- v1.0.4-node-{{ .Branch }}-{{ checksum "package.json" }}
- v1.0.4-node-{{ .Branch }}
- v1.0.5-node-{{ .Branch }}-{{ checksum "package.json" }}
- v1.0.5-node-{{ .Branch }}
- &save-node-cache
key: v1.0.4-node-{{ .Branch }}-{{ checksum "package.json" }}
key: v1.0.5-node-{{ .Branch }}-{{ checksum "package.json" }}
paths:
- node_modules

Expand Down
30 changes: 28 additions & 2 deletions app/components/Views/BrowserTab/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import Branch from 'react-native-branch';
import WatchAssetRequest from '../../UI/WatchAssetRequest';
import Analytics from '../../../core/Analytics';
import ANALYTICS_EVENT_OPTS from '../../../util/analytics';
import { resemblesAddress } from '../../../util/address';
import { toggleNetworkModal } from '../../../actions/modals';
import setOnboardingWizardStep from '../../../actions/wizard';
import OnboardingWizard from '../../UI/OnboardingWizard';
Expand Down Expand Up @@ -470,10 +471,19 @@ export class BrowserTab extends PureComponent {
personal_sign: async payload => {
const { PersonalMessageManager } = Engine.context;
try {
const firstParam = payload.params[0];
const secondParam = payload.params[1];
const params = {
data: firstParam,
from: secondParam
};
if (resemblesAddress(firstParam) && !resemblesAddress(secondParam)) {
params.data = secondParam;
params.from = firstParam;
}
const pageMeta = await this.getPageMeta();
const rawSig = await PersonalMessageManager.addUnapprovedMessageAsync({
data: payload.params[0],
from: payload.params[1],
...params,
...pageMeta
});
return (
Expand Down Expand Up @@ -512,6 +522,22 @@ export class BrowserTab extends PureComponent {
},
eth_signTypedData_v3: async payload => {
const { TypedMessageManager } = Engine.context;

let data;
try {
data = JSON.parse(payload.params[1]);
} catch (e) {
throw new Error('Data must be passed as a valid JSON string.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you throw here it won't get back to the dapp context.
You should do Promise.reject({ error: error.message, jsonrpc: payload.jsonrpc, id: payload.id });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing here does get back to the dapp context (the thrown error is caught and shuffled as the RPC response up the chain.) I verified this only in a dapp context using Dan's test page, but I'm happy to explicitly reject if that's preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this as-is for now to stay in-line with the way we handle invalid data above.

}
const chainId = data.domain.chainId;
const activeChainId =
this.props.networkType === 'rpc' ? this.props.network : Networks[this.props.networkType].networkId;

// eslint-disable-next-line eqeqeq
if (chainId && chainId != activeChainId) {
throw new Error(`Provided chainId (${chainId}) must match the active chainId (${activeChainId})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as previous comment

}

try {
const pageMeta = await this.getPageMeta();
const rawSig = await TypedMessageManager.addUnapprovedMessageAsync(
Expand Down
4 changes: 2 additions & 2 deletions app/core/InpageBridge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,8 @@ window.ethereum._metamask = new Proxy(
* @returns {Promise<boolean>} - Promise resolving to true if accounts have been previously enabled for this domain
*/
isApproved: async () => {
const { isApproved } = await window.ethereum.send('metamask_isApproved');
return isApproved;
const response = await window.ethereum.send('metamask_isApproved');
return response ? response.isApproved : false;
},

/**
Expand Down
9 changes: 9 additions & 0 deletions app/util/address.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,12 @@ export function isENS(name) {
}
return true;
}

/**
* Determines if a given string looks like a valid Ethereum address
*
* @param {address} string
*/
export function resemblesAddress(address) {
return address.length === 2 + 20 * 2;
}