Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

[3.0] Add chain ID to EIP 712 domains #1742

Merged

Conversation

dorothy-zbornak
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak commented Mar 27, 2019

Description

This adds a chainId to the domain separator for the Coordinator and Exchange (as per the EIP-712 spec). For reference, the chain ID is the unique numerical ID of the network the contracts are deployed on.

There are some notable consequences that come with this change, which is why this PR spans so many packages:

  • Both the Coordinator and Exchange need to be deployed with an additional chainId constructor parameter.
  • The domain separator schema for both the Coordinator and the Exchange now looks like EIP712Domain(string name,string version,uint256 chainId,address verifyingContractAddress)
  • In @0x/types, Order, ZeroExTransaction, and signed variants have had their structures changed:
    • exchangeAddress has been removed.
    • A required domain field has been added. This field is of the existing type EIP712DomainWithDefaultSchema, which has been augmented with a required chainId field.
  • @0x/utils.providerUtils now has a getChainIdAsync() function for retrieving the current chain ID given a provider. For testing, you can either use this function or just @0x/order-utils.constants.TESTPRC_NETWORK_ID.
  • The constructor for @0x/test-utils.TransactionFactory now requires a chainId constructor parameter.
  • In @0x/order-utils.OrderFactory, createOrder() and createOrderFromPartial() now require chainId fields/parameters.

I've also taken the liberty of breaking up @0x/contracts-exchange-libs/contracts/src/LibEIP712.sol into reusable components (LibEIP712, LibEIP712ExchangeDomain, and LibEIP712ExchangeDomainConstants) so there is less code duplication between the Coordinator and Exchange.

Testing instructions

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

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.

Add `chainId` to constructors for all test contracts deriving from `LibEIP712.sol`
Split up EIP712 constants and functionality in `/contracts/exchange-libs` across 3, modular contracts.
Make coordinator inherit from the modular EIP712 contracts in `@0x\contracts-exchange`.
Update coordinator tests for new tooling.
Remove unecessary `chainId` parameter in `eip712_utils.createCoordinatorApprovalTypeData`
…s` because reentrancy guard is sort of a breaking change
@dorothy-zbornak dorothy-zbornak force-pushed the feature/contracts/3.0/add-chainId-to-eip-712-domain branch from 51cf3d4 to c0b2e1a Compare March 27, 2019 17:39
@coveralls
Copy link

coveralls commented Mar 27, 2019

Coverage Status

Coverage decreased (-0.04%) to 84.059% when pulling 968f1f9 on feature/contracts/3.0/add-chainId-to-eip-712-domain into 0fb55a3 on 3.0.

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

Looks good! Might not be necessary but it could be worthwhile to add explicit tests for known Chain ID's / expected behavior when an unrecognized/malformed Chain ID is used client-side.

@dorothy-zbornak
Copy link
Contributor Author

Looks good! Might not be necessary but it could be worthwhile to add explicit tests for known Chain ID's / expected behavior when an unrecognized/malformed Chain ID is used client-side.

@hysz Wondering if the new tests I just added to contracts/exchange-libs/test/src/exchange_libs.ts cover this requirement. Is it sufficient to prove that LibOrder and LibEIP712 produce different order hashes and domain separators given a different chainId? It implies that any logic depending on a hash produced by LibEIP712.sol (such as signature validation) would fail if the chainId does not match.

contracts/coordinator/contracts/test/TestLibs.sol Outdated Show resolved Hide resolved
contracts/coordinator/contracts/test/TestMixins.sol Outdated Show resolved Hide resolved
@@ -18,6 +18,7 @@ export interface Order {
takerAssetData: string;
salt: BigNumber;
exchangeAddress: string;
chainId: number;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than just adding the chainId, I think we should take this opportunity to add an extra eip712Domain field that looks like:

{
    verifyingContract: ...,
    chainId: ...,
    name: ...,
    version: ...
}

Alternatively, we can just make it a eip712DomainHash field and do all of the calculations beforehand. This would consume less bandwidth but make it a bit harder to develop with.

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 went with domain rather than eip712Domain for the new field in the Order and ZeroExTransaction types for brevity. Let me know if this is fine or you'd rather be more explicit.

contracts/exchange-libs/contracts/src/LibEIP712.sol Outdated Show resolved Hide resolved
…LibEIP712ExchangeDomain` rather than `LibEIP712`
Add required field `domain` to `order` and `zeroExTransaction` schemas.
…abi-gen-wrappers` to use new domain schema and `Order` format.
…e`, and `extensions` contract tests to use new order and transaction structure
};
const makerPrivateKey = devConstants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddress)];
const takerPrivateKey = devConstants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(takerAddress)];
const feeRecipientPrivateKey = devConstants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(feeRecipientAddress)];
orderFactory = new OrderFactory(makerPrivateKey, defaultOrderParams);
makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchange.address);
takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchange.address);
makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchange.address, chainId);
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but this should probably be changes later to just take a domain parameter.

@dorothy-zbornak dorothy-zbornak merged commit 0bdc44f into 3.0 Apr 1, 2019
@dekz dekz added this to the v3 development milestone Jun 21, 2019
@dorothy-zbornak dorothy-zbornak deleted the feature/contracts/3.0/add-chainId-to-eip-712-domain branch August 19, 2019 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants