Add `gasPrice` to 0x transactions #2084

abandeali1 · 2019-08-21T00:06:39Z

Description

Implements part of ZEIP-42.

Adds a gasPrice field to 0x transactions. Transactions can now only be executed if the Ethereum tx gasPrice matches that of the 0x tx.
Breaks out all of the validation in executeTransaction into an _assertExecutableTransaction function.
Adds to and refactors MixinTransaction Unit Tests #2069 to reduce some code duplication.
Introduces a new TransactionInvalidContextError that replaces the previous reentrancy error.

Testing instructions

Types of changes

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2019-08-21T02:45:29Z

Coverage remained the same at 75.804% when pulling 798fb18 on feat/3.0/transactionGasPrice into 6bb3992 on 3.0.

jalextowle

Nice job, and you finished this fast! I think there is still uncertainty about how reentrancy should be handled, but I'll approve once we've figured that out.

jalextowle · 2019-08-21T18:23:52Z

contracts/exchange/test/transactions_unit_tests.ts

+        returnData?: string;
+    }
+
+    async function generateZeroExTransactionAsync(


This is a very nice refactor 👍This really improves readability.

jalextowle · 2019-08-21T18:24:49Z

contracts/exchange/test/transactions_unit_tests.ts

+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
+            expect(
+                result[0] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(


I think that we could likely make this logic nicer by fixing an issue in the AbiEncoder. I'll create a backlog task for this.

Maybe refactor out the encode() + hex voodoo and use hexConcat() from @0x/contracts-test-utils: hexConcat( '0x0000000000000000000000000000000000000000000000000000000000000020', encodedDeadBeef)

Found a super clean solution to this :)

jalextowle · 2019-08-21T18:27:05Z

contracts/exchange/test/transactions_unit_tests.ts

+        it('should revert if the current time is past the expiration time', async () => {
+            const currentTimestamp = await getLatestBlockTimestampAsync();
+            const transaction = await generateZeroExTransactionAsync({
+                expirationTimeSeconds: new BigNumber(currentTimestamp).minus(10), // Set the expiration time to before the current timestamp


What do you think about using a expirationOffsetFromCurrentTime parameter instead of this? Then we could remove the call to currentTimestamp in our test cases (making them a bit cleaner), and it would still be very easy to read and understand.

I don't have a strong preference. We aren't using it a ton right now, so it might not be worth the refactor.

jalextowle · 2019-08-21T18:28:35Z

contracts/exchange/test/transactions_unit_tests.ts

+            return expect(tx).to.revertWith(expectedError);
+        });
+        // FIXME - This should be unskipped when the contracts have been updated to fix this problem.
+        it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer for both calls', async () => {


Can this be unskipped before merging this PR?

This test will be removed (it should fail).

jalextowle · 2019-08-21T18:28:51Z

contracts/exchange/test/transactions_unit_tests.ts

+            return expect(tx).to.revertWith(expectedError);
+        });
+        // FIXME - This should be unskipped when the contracts have been updated to fix this problem.
+        it.skip('should revert if reentrancy occurs in the middle of an executeTransaction call and msg.sender == signer and then msg.sender != sender', async () => {


Is this actually desirable? Your offline comment about nested reentrancy makes me wonder whether or not this should actually fail.

This test will also be removed (it should fail).

jalextowle · 2019-08-21T20:09:13Z

contracts/exchange/contracts/src/MixinTransactions.sol

+                requiredGasPrice
+            ));
+        }
+
        // Prevent reentrancy
        if (currentContextAddress != address(0)) {


I think that there is a way for us to support nested executeTransaction calls. If we kept a stack variable called startingContextAddress, we could then replace our erasure of the currentContextAddress with the startingContextAddress. In this way, we'd get the best of both worlds by allowing users to submit metatransactions that submit other metatransactions (relayers could possibly make use of this), but they are always executed in the right context. Thoughts?

dorothy-zbornak · 2019-08-21T19:59:11Z

contracts/exchange/test/transactions_unit_tests.ts

+        const _domain = opts.domain === undefined ? domain : opts.domain;
+        const expirationTimeSeconds =
+            opts.expirationTimeSeconds === undefined
+                ? new BigNumber(await getLatestBlockTimestampAsync()).plus(10)


If we just set this far enough in the future, this function won't need to be async or do an RPC call.

I guess we could just use 0 and MAX_UINT256 for all of these values. I can make that change.

dorothy-zbornak · 2019-08-21T20:11:06Z

contracts/exchange/test/transactions_unit_tests.ts

+        it('should revert if signer != msg.sender and the signature is invalid', async () => {
+            const transaction = await generateZeroExTransactionAsync({ signerAddress: accounts[0] });
+            const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            const invalidSignature = '0x0000';


Might as well pop this bad boy into a constant.

dorothy-zbornak · 2019-08-21T21:12:23Z

.circleci/config.yml

@@ -91,7 +91,7 @@ jobs:
                  keys:
                      - repo-{{ .Environment.CIRCLE_SHA1 }}
            - run: yarn wsrun test:circleci @0x/contracts-multisig @0x/contracts-utils @0x/contracts-exchange-libs @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-extensions @0x/contracts-asset-proxy @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-coordinator @0x/contracts-dev-utils @0x/contracts-staking
-    test-contracts-ganache-3.0:
+    test-exchange-ganache-3.0:


As requested.

dorothy-zbornak · 2019-08-21T21:17:04Z

contracts/exchange/test/transactions_unit_tests.ts

+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
+            expect(
+                result[0] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(


Equality should be moved outside into a to.eq(...) so we can see the values if it fails.

dorothy-zbornak · 2019-08-21T21:20:09Z

contracts/exchange/test/transactions_unit_tests.ts

+            const encodedDeadbeef = abiEncoder.encode('0xdeadbeef');
+            expect(
+                result[0] ===
+                    '0x0000000000000000000000000000000000000000000000000000000000000020'.concat(


Maybe refactor out the encode() + hex voodoo and use hexConcat() from @0x/contracts-test-utils: hexConcat( '0x0000000000000000000000000000000000000000000000000000000000000020', encodedDeadBeef)

dorothy-zbornak · 2019-08-21T21:27:31Z

contracts/exchange/test/transactions_unit_tests.ts

+            });
+            const transactionHash = transactionHashUtils.getTransactionHashHex(transaction);
+            // Verify that the returndata of the transaction is 0xDEADBEEF
+            const result = await transactionsContract.executeTransaction.callAsync(transaction, validSignature, {


The TransactionHelper class in @0x/contracts-test-utils might reduce some of the code in blocks where you do a callAsync, sendTransactionAsync, and getTxWithDecodedLogsAsync since it does all three for you.

dorothy-zbornak · 2019-08-21T21:28:50Z

contracts/exchange/contracts/test/TestTransactions.sol

+        public
+        returns (bytes memory)
+    {
+        emit ExecutableCalled(data, returnData);


I think it might be super cool to log the current context address (and assert it), too.

dorothy-zbornak · 2019-08-21T21:40:16Z

contracts/exchange/test/transactions_unit_tests.ts

+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(transaction2, validSignature, {
+                from: accounts[0],
+            });
+            return expect(tx).to.revertWith(expectedError);


I guess now we should check that this case doesn't revert, right?

Do we want to test for that or just remove the test?

dorothy-zbornak · 2019-08-21T21:52:51Z

contracts/exchange/test/transactions_unit_tests.ts

+            expect(logs[3].event).to.be.eq('TransactionExecution');
+            expect(logs[3].args.transactionHash).to.eq(transactionHash2);
+        });
+    });


Should we be testing recursion? Here's a weird edge case that seems like it would pass:

batchExecuteTransactions( [ executeTransaction( batchExecuteTransaction( [ executeTransaction(executable(...), from=Bob), executeTransaction(executable(...), from=Charlie), ], from=Alice, ), from=Alice, ) ], from=Alice, )

This should pass. Why is it an edge case?

Because it's an ultra specific scenario that takes advantage of all the reentrancy and context management features in the system. Basically, if we changed any one of them this would probably fail so it might be a good way to check for regressions.

@dorothy-zbornak Do you think it would make sense to add a simpler test for this? In this case, we could nest in an equally valid way by excluding the batchExecuteTransaction and just adding a single call to executeTransaction. Not to say that we shouldn't include the test case that you describe, but I think it would also be good to have a super simple case.

Yeah, we could probably afford to peel off one layer. I think all the other stuff kind of has to go together to hit all the paths at once, though. 🤔 I do think we're pretty well covered for now.

dorothy-zbornak · 2019-08-21T21:57:18Z

contracts/exchange/test/transactions_unit_tests.ts

+            const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] });
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
+            const transaction2 = await generateZeroExTransactionAsync({
+                signerAddress: accounts[0],


Wait, does this align with what the description says? Shouldn't it be accounts[1] here?

Also, we should have a test that's similar but where the nested tx doesn't have a valid signature, just to assert that we are checking signatures on inner transactions.

I think that this is correct. The transaction is sent with accounts[0], which is the signerAddress for the outerTransaction. The signer of the inner transaction is accounts[1], so it seems like the description is correct. Not sure if this comment still applies after the changes though.

moodlezoup

A few nits but otherwise everything has either already been covered or lgtm

moodlezoup · 2019-08-21T22:05:43Z

contracts/exchange-libs/contracts/src/LibExchangeRichErrors.sol

@@ -125,6 +125,10 @@ library LibExchangeRichErrors {
    // bytes4(keccak256("TransactionExecutionError(bytes32,bytes)"))
    bytes4 internal constant TRANSACTION_EXECUTION_ERROR_SELECTOR =
        0x20d11f61;
+
+    // bytes4(keccak256("TransactionGasPriceError(bytes32,unt256,uint256)"))


nit: uint256

moodlezoup · 2019-08-21T22:43:22Z

contracts/exchange/contracts/test/TestTransactions.sol

+        currentContextAddress = context;
+    }
+
+    function setTransactionHash(bytes32 hash)


maybe rename to setTransactionExecuted

moodlezoup · 2019-08-21T22:48:15Z

packages/order-utils/src/exchange_revert_errors.ts

@@ -207,6 +207,20 @@ export class TransactionExecutionError extends RevertError {
    }
 }

+export class TransactionGasPriceError extends RevertError {
+    constructor(transactionHash?: string, actualGasPrice?: BigNumber, requiredGasPrice?: BigNumber) {


elsewhere we type uint256 parameters as BigNumber | number | string, we should stick to one convention or the other

We just use BigNumber in more of the constructors than not, so I will remove the number | string in any other constructors.

moodlezoup · 2019-08-21T23:34:54Z

contracts/exchange/test/transactions_unit_tests.ts

+                    ),
+            ).to.be.true();
+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"


nit: 0xbeefdead

moodlezoup · 2019-08-21T23:35:02Z

contracts/exchange/test/transactions_unit_tests.ts

+
+            // Ensure that the result contains the abi-encoded bytes "0xdeadbeef"
+            const encodedBeefdead = abiEncoder.encode('0xbeefdead');
+            expect(result.length).to.be.eq(2);


already checked above

…on to not require state

…hauling transaction_unit_tests.ts

…d the `Killed` error

…rease speed

dorothy-zbornak

One-ish change then g2g! Well done!

dorothy-zbornak · 2019-08-23T16:59:53Z

contracts/exchange/test/transactions_unit_tests.ts

            );
+            expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result[1])).to.equal(returnData2);


dorothy-zbornak · 2019-08-23T17:19:27Z

contracts/exchange/test/transactions_unit_tests.ts

+                accounts[0],
+            ).encode();
+            const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData);
+            const tx = transactionsContract.executeTransaction.sendTransactionAsync(outerTransaction, validSignature, {


These should all be awaitTransactionSuccessAsync() calls, even if they revert.

jalextowle

Good stuff (ง ͡ʘ ͜ʖ ͡ʘ)ง

Looks ready to merge after thinking a bit about the comments on test cases. At one point, I recommend removing a test case, which makes me think that at some point it might be a good idea to generalize ReentrancyTester to check for statements like assertExecutableTransaction in functions rather than using redundant test cases. Definitely out of scope for this PR, but it's something to think about long term.

jalextowle · 2019-08-23T17:30:52Z

contracts/exchange/test/transactions_unit_tests.ts

+            const transaction1 = await generateZeroExTransactionAsync({ signerAddress: accounts[0] });
+            const transactionHash1 = transactionHashUtils.getTransactionHashHex(transaction1);
+            const transaction2 = await generateZeroExTransactionAsync({
+                signerAddress: accounts[0],


I think that this is correct. The transaction is sent with accounts[0], which is the signerAddress for the outerTransaction. The signer of the inner transaction is accounts[1], so it seems like the description is correct. Not sure if this comment still applies after the changes though.

jalextowle · 2019-08-23T20:30:08Z

contracts/exchange/test/transactions_unit_tests.ts

+                { from: accounts[0] },
+            );
+
+            expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result)).to.equal(


jalextowle · 2019-08-23T20:35:03Z

contracts/exchange/test/transactions_unit_tests.ts

+                }),
+            ).to.revertWith(expectedError);
+        });
+        it('should be successful if signer == msg.sender and the signature is invalid', async () => {


I think that we should also check that the tx should be successful if signer == msg.sender and the signature is valid. It's a bit pedantic, but it would make the suite more complete.

jalextowle · 2019-08-23T20:42:11Z

contracts/exchange/test/transactions_unit_tests.ts

+            expect(logs[3].event).to.be.eq('TransactionExecution');
+            expect(logs[3].args.transactionHash).to.eq(transactionHash2);
+        });
+    });


@dorothy-zbornak Do you think it would make sense to add a simpler test for this? In this case, we could nest in an equally valid way by excluding the batchExecuteTransaction and just adding a single call to executeTransaction. Not to say that we shouldn't include the test case that you describe, but I think it would also be good to have a super simple case.

jalextowle · 2019-08-23T20:44:10Z

contracts/exchange/test/transactions_unit_tests.ts

+            );
+            return expect(tx).to.revertWith(expectedError);
+        });
+        it('should revert if the transaction is submitted with a gasPrice that does not equal the required gasPrice', async () => {


Since you already test this behavior in assertExecutableTransaction's test, I'd be fine if we want to remove this. Any reason why we shouldn't?

I feel like most of the other tests are actually covered by the assertExecutableTransaction tests. We have extra tests for expired transactions, valid/invalid signatures, already executed transactions, etc. If we remove this, it probably makes sense to remove the other redundant tests as well.

I guess out of these options, it might make sense to just keep them to prevent regressions. Longer term, I do think we should try to reduce test duplication, but it's easy to understand which are duplicates in this file.

abandeali1 force-pushed the feat/3.0/transactionGasPrice branch from bd5adb0 to 928c5ba Compare August 21, 2019 05:16

abandeali1 changed the title ~~[WIP] Add gasPrice to 0x transactions~~ Add gasPrice to 0x transactions Aug 21, 2019

abandeali1 marked this pull request as ready for review August 21, 2019 15:50

abandeali1 requested review from fabioberger and hysz as code owners August 21, 2019 15:50

abandeali1 requested review from dorothy-zbornak, jalextowle and moodlezoup and removed request for fabioberger August 21, 2019 15:50

jalextowle reviewed Aug 21, 2019

View reviewed changes

mintcloud assigned jalextowle, dorothy-zbornak and moodlezoup Aug 21, 2019

dorothy-zbornak suggested changes Aug 21, 2019

View reviewed changes

moodlezoup reviewed Aug 22, 2019

View reviewed changes

jalextowle and others added 14 commits August 22, 2019 17:01

@0x:contracts-exchange Added unit tests for getCurrentContextAddress

74b9ad5

@0x:contracts-exchange Added unit tests for executeTransaction

1724ecd

@0x:contracts-exchange Added unit tests for batchExecuteTransactions

907771f

@0x:contracts-exchange Removed code written to transactions

4b97090

@0x:contracts-exchange Changed the signature validation stub functi…

d845b31

…on to not require state

@0x:contracts-exchange Addressed review comments by completely over…

0253bba

…hauling transaction_unit_tests.ts

@0x:contracts-exchange Fixed issues caused by rebase

cd147db

CI: Changed the test-contracts-ganache-3.0 to run in stages to avoi…

c1ed836

…d the `Killed` error

CI: Split contracts tests up into two seperate jobs to attempt to inc…

5733805

…rease speed

Add gasPrice to ZeroExTransaction struct

67f9126

Add TransactionGasPriceError to LibExchangeRichErrors

2134537

Add _assertExecutableTransaction function and add gasPrice check

47da971

Update remaining contracts to use new transaction schema

27e2a76

Update ZeroExTransaction interface

1dd216b

abandeali1 added 15 commits August 22, 2019 17:04

Update ZeroExTransaction schema

e6b81a8

Update transaction hashing and tests

59369ce

Update ZeroExTransaction tests

23dd711

Add default gasPrice to web3Wrapper instance and TransactionFactory

2b1e0be

Fix weth tests

f41a29c

Add TransactionGasPriceError rich revert class

f32732d

Add transaction gasPrice tests

9d38bf7

Reduce code duplication in unit tests

a114bbb

Add public version of _assertExecutableTransaction

eb6637a

Add _assertExecutableTransaction unit tests

ca35eed

Add more unit tests

7b96fa8

Add TransactionInvalidContextError and remove NO_REENTRANCY errorCode

44753bb

Use TransactionInvalidContextError in _assertExecutableTransaction

edb923b

Add TransactionInvalidContextError class

37cc948

Update tests to use new TransactionInvalidContextError

890bfd1

abandeali1 force-pushed the feat/3.0/transactionGasPrice branch from b26d470 to 5986577 Compare August 23, 2019 00:10

Address PR feedback

5e51233

abandeali1 force-pushed the feat/3.0/transactionGasPrice branch from 5986577 to 5e51233 Compare August 23, 2019 00:12

abandeali1 added 3 commits August 23, 2019 08:51

Fix config.yml

76c0708

Add more recursion tests

8f8c16b

Use default gasPrice in Forwarder tests

830d6f7

dorothy-zbornak approved these changes Aug 23, 2019

View reviewed changes

jalextowle approved these changes Aug 23, 2019

View reviewed changes

Address remaining PR feedback

798fb18

abandeali1 force-pushed the feat/3.0/transactionGasPrice branch from 106b171 to 798fb18 Compare August 23, 2019 22:14

abandeali1 merged commit 8ef0a59 into 3.0 Aug 24, 2019

		);
		expect(transactionsContract.executeTransaction.getABIDecodedReturnData(result[1])).to.equal(returnData2);

Add gasPrice to 0x transactions #2084

Add gasPrice to 0x transactions #2084

Conversation

abandeali1 commented Aug 21, 2019 • edited by jalextowle Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Aug 21, 2019 • edited Loading

jalextowle left a 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

abandeali1 Aug 22, 2019 • 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

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

moodlezoup left a 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

dorothy-zbornak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalextowle left a 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

Add `gasPrice` to 0x transactions #2084

Add `gasPrice` to 0x transactions #2084

abandeali1 commented Aug 21, 2019 •

edited by jalextowle

Loading

coveralls commented Aug 21, 2019 •

edited

Loading

abandeali1 Aug 22, 2019 •

edited

Loading