[0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing #1102

dekz · 2018-10-01T11:39:13Z

Description

TODO

rename ecSignOrderHashAsync to ecSignHashAsync for use in executeTransaction etc
Expose a higher level function where the user can switch between eth_signTypedData and eth_sign if signTypedData is unsupported.
Mnemonic wallet and PrivateKey wallet support eth_signTypedData
check in with MM regarding eth_sign and their prefix. This was a TODO for them dependent on merging EIP712 support.
Detect MM if signing fails and throw a more helpful error message (i.e you should use MMSubprovider) for a graceful transition.

Note: MM now has another inconsistency with eth_signTypedData. It has published 4.12 and named the correct spec compliant implementation eth_signTypedData_v3. It also requires the typedData parameter to be JSON encoded (inconsistent with Ganache).

I propose we quarantine all the MM inconsistency in a Subprovider called MetamaskSubprovider. This allows us to handle the inconsistent eth_sign behaviour (no prefix is added) as well as handle the eth_signTypedData inconsistency. Removing this leaky abstraction from the rest of our code as much as possible (SignerType removed).

This subprovider can be used standalone or as a web3ProviderEngine subprovider.

// standalone 
const signature = await signatureUtils.ecSignOrderAsync(new MetamaskSubprovider(web3.currentProvider), order, makerAddress);

// web3 provider engine
const providerEngine = new Web3ProviderEngine();
const metamaskSubprovider = new MetamaskSubprovider(web3.currentProvider);
providerEngine.addProvider(metamaskSubprovider);
providerEngine.addProvider(new RPCSubprovider(KOVAN_RPC));
const signature = await signatureUtils.ecSignOrderAsync(providerEngine, order, makerAddress);

After merge and publish

update sandbox to use the latest versions of 0x.js
Portal to be updated with EIP712 order signing

Testing instructions

Ganache and MM share the same implementation of eth_signTypedData. We test against this implementation inside of ganache. The output of eth_signTypedData is the same as a signed message of our orderHash without the prefix (since our orderHash is EIP712).

Ganache (for automated tests)
Metamask (codesandbox and website)
Metamask + Kovan

Types of changes

Checklist:

Prefix PR title with [WIP] if necessary.
Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2018-10-02T10:47:49Z

Coverage decreased (-0.4%) to 84.87% when pulling 8620cbc213556b3961087f1eedf3ba7283494081 on feature/0x.js/eip712-sign-typed-data into 119f8c9 on development.

Create a helper back in EIP712Utils for code cleanup. Moved constants in order-utils into the constants object

Report a developer friendly error in this event to educate them on the compatability wrapper MetamaskSubprovider

fabioberger · 2018-10-08T08:00:14Z

packages/order-utils/src/eip712_utils.ts

-    pad32Buffer(buffer: Buffer): Buffer {
-        const bufferPadded = ethUtil.setLengthLeft(buffer, EIP712_VALUE_LENGTH);
-        return bufferPadded;
+    createOrderTypedData: (order: Order): EIP712TypedData => {


Hm, slight preference for createTypedDataOrder.

I have a slight preference for important information first when reading the code.

createTypedDataOrder createTypedDataZeroExTransaction

vs

createOrderTypedData createZeroExTransactionTypedData

Also, add method devdoc comment for this method.

Already added

packages/order-utils/src/order_hash.ts

fabioberger · 2018-10-08T08:13:40Z

packages/0x.js/CHANGELOG.json

+                "pr": 1102
+            },
+            {
+                "note": "Added `MetamaskSubprovider` to handle inconsistencies with signing.",


inconsistencies in Metamask's signing JSON RPC endpoints

fabioberger · 2018-10-08T08:23:31Z

packages/contract-wrappers/src/utils/transaction_encoder.ts

-        const eip721MessageBuffer = eip712Utils.createEIP712Message(executeTransactionHashBuff, exchangeAddress);
-        const messageHex = `0x${eip721MessageBuffer.toString('hex')}`;
+        const typedData = eip712Utils.createZeroExTransactionTypedData(executeTransactionData, exchangeAddress);
+        const eip712MessageBuffer = signTypedDataUtils.signTypedDataHash(typedData);


Rename signTypedDataHash to generateTypedDataHash. There is no signing going on and the current method name suggests as much.

fabioberger · 2018-10-08T08:31:44Z

packages/order-utils/src/eip712_utils.ts

-    pad32Buffer(buffer: Buffer): Buffer {
-        const bufferPadded = ethUtil.setLengthLeft(buffer, EIP712_VALUE_LENGTH);
-        return bufferPadded;
+    createOrderTypedData: (order: Order): EIP712TypedData => {


Also, add method devdoc comment for this method.

fabioberger · 2018-10-08T08:33:09Z

packages/order-utils/src/eip712_utils.ts

-        const bufferPadded = ethUtil.setLengthLeft(buffer, EIP712_VALUE_LENGTH);
-        return bufferPadded;
+    createOrderTypedData: (order: Order): EIP712TypedData => {
+        const normalizedOrder = _.mapValues(order, value => {


Let's first assert that the Order passed in conforms to the Order schema.

fabioberger · 2018-10-08T08:33:33Z

packages/order-utils/src/eip712_utils.ts

+        message: EIP712Object,
+        exchangeAddress: string,
+    ): EIP712TypedData => {
+        const typedData = {


Add assertions for the params. This is a public, exported method.

fabioberger · 2018-10-08T08:41:12Z

packages/order-utils/src/signature_utils.ts

+        }
+    },
+    /**
+     * Signs an order using `eth_signTypedData` and returns its elliptic curve signature and signature type.


Not true any more: returns its elliptic curve signature and signature type

fabioberger · 2018-10-08T08:51:17Z

packages/order-utils/src/signature_utils.ts

+        } catch (err) {
+            // Detect if Metamask to transition users to the MetamaskSubprovider
+            if ((provider as any).isMetaMask) {
+                throw new Error(`Unsupported Provider, please use MetamaskSubprovider: ${err.message}`);


Let's be more explicit: MetaMask provider must be wrapped in a MetamaskSubprovider (from the '@0xproject/subproviders' package) in order to work with this method.

fabioberger · 2018-10-08T08:51:48Z

packages/order-utils/src/signature_utils.ts

-        const signature = await web3Wrapper.signMessageAsync(normalizedSignerAddress, msgHashHex);
+    },
+    /**
+     * Signs a hash and returns its elliptic curve signature and signature type.


Let's mention eth_sign here to mirror the other method description.

fabioberger · 2018-10-08T08:57:38Z

packages/order-utils/src/signature_utils.ts

-        throw new Error(OrderError.InvalidSignature);
+        // Detect if Metamask to transition users to the MetamaskSubprovider
+        if ((provider as any).isMetaMask) {
+            throw new Error('Unsupported Provider, please use MetamaskSubprovider.');


Copy above error message.

fabioberger · 2018-10-08T09:00:59Z

packages/website/ts/blockchain.ts

@@ -161,7 +161,7 @@ export class Blockchain {
            // We catch all requests involving a users account and send it to the injectedWeb3
            // instance. All other requests go to the public hosted node.
            const provider = new Web3ProviderEngine();
-            provider.addProvider(new SignerSubprovider(injectedWeb3.currentProvider));
+            provider.addProvider(new MetamaskSubprovider(injectedWeb3.currentProvider));


Wait, we only want to do this IF provider.isMetamask is true. Let's add that check. Website should also work with other injected signers.

In web3 wrapper when a response contains an error field we throw this rather than return response.result which is often undefined. In Signature Utils we handle the error thrown when a user rejects the signing dialogue to prevent double signing. Exposed the ZeroExTransaction JSON schema. In Website only use the MetamaskSubprovider if we can detect the provider is Metamask

fabioberger · 2018-10-09T08:14:53Z

packages/order-utils/CHANGELOG.json

+        "version": "2.0.0",
+        "changes": [
+            {
+                "note": "Added `ecSignOrderAsync` to first sign an order as EIP712 and fallback to EthSign",


Let's replace as EIP712 and fallback to EthSign with via 'eth_signTypedData' and fallback to 'eth_sign'

fabioberger · 2018-10-09T08:22:40Z

packages/order-utils/src/signature_utils.ts

+            // HACK: We are unable to handle specific errors thrown since provider is not an object
+            //       under our control. It could be Metamask Web3, Ethers, or any general RPC provider.
+            //       We check for a user denying the signature request in a way that supports Metamask and
+            //       Coinbase Wallet


Add: . Unfortunately for signers with a different error message, they will receive two signature requests.

fabioberger · 2018-10-09T08:48:07Z

packages/order-utils/src/signature_utils.ts

+        // Detect if Metamask to transition users to the MetamaskSubprovider
+        if ((provider as any).isMetaMask) {
+            throw new Error(
+                `MetaMask provider must be wrapped in a MetamaskSubprovider (from the '@0xproject/subproviders' package) in order to work with this method.`,


Let's add this to OrderError enum.

dekz added the WIP label Oct 1, 2018

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch 8 times, most recently from 9336467 to 20ca573 Compare October 2, 2018 10:01

dekz requested review from BMillman19 and fragosti as code owners October 2, 2018 10:01

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch from 20ca573 to da49112 Compare October 2, 2018 10:29

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch 11 times, most recently from b867dce to 15a17fd Compare October 4, 2018 09:17

dekz changed the title ~~[WIP] [order-utils] [web3-wrapper] Expose eth_signTypedData functionality for order signing~~ [0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing Oct 4, 2018

dekz added 0x.js @0x/subproviders @0x/types @0x/utils @0x/order-utils labels Oct 4, 2018

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch 17 times, most recently from 7115285 to 050cc17 Compare October 5, 2018 06:58

Return SignedOrder from signing utils.

75d274f

Create a helper back in EIP712Utils for code cleanup. Moved constants in order-utils into the constants object

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch from 050cc17 to 75d274f Compare October 5, 2018 07:12

Detect MM on signature validation failure.

e1236a4

Report a developer friendly error in this event to educate them on the compatability wrapper MetamaskSubprovider

fabioberger reviewed Oct 8, 2018

View reviewed changes

fabioberger suggested changes Oct 8, 2018

View reviewed changes

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch from 8620cbc to c979949 Compare October 9, 2018 07:36

dekz force-pushed the feature/0x.js/eip712-sign-typed-data branch from c979949 to 9e8031d Compare October 9, 2018 08:01

Update the exported types for the packages which touch RPC providers

7f55430

fabioberger approved these changes Oct 9, 2018

View reviewed changes

Move Metamask Error to OrderErrors

75b9e63

dekz merged commit 024bcf4 into development Oct 9, 2018

dekz deleted the feature/0x.js/eip712-sign-typed-data branch October 9, 2018 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing #1102

[0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing #1102

dekz commented Oct 1, 2018 •

edited

Loading

coveralls commented Oct 2, 2018 •

edited

Loading

fabioberger Oct 8, 2018

dekz Oct 8, 2018

fabioberger Oct 8, 2018

dekz Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 8, 2018

fabioberger Oct 9, 2018

fabioberger Oct 9, 2018

fabioberger Oct 9, 2018

[0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing #1102

[0x.js][order-utils][web3-wrapper] Expose eth_signTypedData functionality for order signing #1102

Conversation

dekz commented Oct 1, 2018 • edited Loading

Description

After merge and publish

Testing instructions

Types of changes

Checklist:

coveralls commented Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekz commented Oct 1, 2018 •

edited

Loading

coveralls commented Oct 2, 2018 •

edited

Loading