Reorganize and modularize generated contract wrappers and artifacts #1105

albrow · 2018-10-02T00:02:07Z

Description

This PR is a huge reorganization of how we generate, store, and copy generated files related to contracts (specifically artifacts and contract wrappers generated by abi-gen).

Here's a summary of the biggest changes (some of which are not fully complete yet):

1. The contracts package is now a public package that will be published with the name @0xproject/contracts
2. All artifacts have been moved out of @0xproject/migrations and into @0xproject/contracts. Artifacts are encapsulated in an npm package and have a version number indicated by a package.json file. You can import artifacts with import { artifacts } from '@0xproject/contracts'.
3. All contract wrappers have been moved into @0xproject/contracts. Like artifacts, they are encapsulated in an npm package. You can import them with import { wrappers } from '@0xproject/contracts'.
4. Addresses of deployed contracts are no longer stored in artifact files. For tests, you can get the addresses of contracts that were deployed during migrations with the new getContractAddresses function in @0xproject/migrations. (I'm still not sure if I'm happy with this exact interface, but in any case there will be some mechanism to get the contract addresses after you run runMigrationsAsync). For contracts that have been deployed to the mainnet or various testnets, the addresses will be exposed as an object in one of our public packages (exact details TBD). This approach leads to a better separation between things that are temporary (addresses of contracts that were deployed for testing purposes) and permanent (addresses, abis, and source code of contracts that have already been deployed to mainnet or a testnet).

(A lot has changed. See below for an updated high-level description).

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.
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-02T00:11:35Z

Coverage decreased (-0.3%) to 84.688% when pulling 5938e8a on feature/contracts-artifacts-re-org into 05bf7a8 on development.

LogvinovLeon · 2018-10-02T08:25:22Z

What is the generated artifacts folder? Why can't we just use the artifacts from migrations?

fabioberger · 2018-10-02T08:33:17Z

packages/contracts/package.json

-    "name": "contracts",
-    "version": "2.1.48",
+    "name": "@0xproject/contracts",
+    "version": "2.0.0",


Hm, what's the rationale here? Lerna is going to publish patch updates of this package whenever it's deps update. Don't think it's possible to have this version track the "contract" version. Rather we should maybe add a second key called "0xVersion: "2.0.0"

I was thinking it would be cleaner to start at 2.0.0 and it shouldn't matter since this package has never been published before.

That said, you're right that in the long-term this version will probably not track the "contract" version exactly. I changed my mind and I think it's probably better to keep the same version number we had before. I'll change it back to 2.1.48.

fabioberger · 2018-10-02T08:36:12Z

packages/contracts/src/index.ts

@@ -0,0 +1,3 @@
+export * from './artifacts';
+import * as wrappers from './wrappers';
+export { wrappers };


Also, in contracts let's add a .npmignore file and make sure the bundle published to npmjs.com repository only includes these files and not the tests and Solidity source dirs.

👍 I was planning to sort what to include in the published package out at the end. Want to make sure everything works inside the monorepo first.

albrow · 2018-10-02T17:54:25Z

@LogvinovLeon the goal of this PR is to have all artifacts and generated contract wrappers in a single canonical location. Other packages that need those things will import the canonical package instead of copying files or calling abi-gen manually.

I'm planning to have the contracts package be that canonical location and will remove the actual artifacts from the migrations package. The reasoning for putting them in contracts instead of migrations is that it keeps the boundaries between packages more clear. If the artifacts were stored in migrations, we would need to have build and/or clean steps that either modify files in other packages or call scripts or reference files in other packages, which can lead to some unintuitive results and confusing build-order dependencies.

fabioberger · 2018-10-03T09:11:32Z

Reminder: make sure to add CHANGELOG.json and CHANGELOG.md files to the contracts package.

albrow · 2018-10-03T23:37:45Z

I added a summary of major changes to this PR description. In terms of progress:

The @0xproject/contracts package has been created with the structure described above.
@0xproject/migrations has been updated as described above. I removed the artifacts and added the new getContractsAddress function. Migrations also operate on only one version of the contracts at a time so I removed the v1 artifacts and related migrations.
@0xproject/contract-wrappers has been updated to use the new locations for artifacts and abi-gen'ed contract wrappers.

The biggest things I still need to do are:

Update any other packages that rely on old locations for artifacts or abi-gen'ed contract wrappers.
Clean up @0xproject/contracts and get it ready to publish. This means only exporting what we need to export, adding things to .npmignore and other config files, creating CHANGELOG.json, etc. I also want to double check bundle sizes and make sure they're manageable.
Make sure all the documentation is up to date.

albrow · 2018-10-09T23:33:58Z

Updated high-level summary of changes:

All artifacts have been moved out of @0xproject/migrations. Bleeding edge development artifacts have been moved to the contracts package, where they are kept private and are not imported by any other packages. On the other hand, permanent, versioned artifacts are available in the new @0xproject/contract-artifacts package. They are encapsulated in an npm package and have a version number indicated by a package.json file. You can import artifacts with import * as artifacts from '@0xproject/contract-artifacts'. (You can also import individual artifacts directly). This approach allows us to stop manually copying around artifact files to the packages that need them.
Abi-gen'ed contract wrappers have been moved into the new@0xproject/abi-gen-wrappers package. Like artifacts, they are encapsulated in an npm package. You can import them with import * as wrappers from '@0xproject/abi-gen-wrappers'. (You can also import individual wrappers directly). This approach allows us to avoid re-generating the same wrappers across many different packages.
Addresses of deployed contracts are no longer stored in artifact files. For tests, the runMigrationsAsync function in @0xproject/migrations simply returns the contract addresses. For contracts that have been deployed to the mainnet or various testnets, the addresses are exposed via the new @0xproject/contract-addresses package. This approach leads to a better separation between things that are temporary (addresses of contracts that were temporarily deployed, e.g., to Ganache, for testing purposes) and permanent (addresses of contracts that have already been deployed to an external network).

albrow · 2018-10-10T06:17:48Z

Okay I think I have an approach that works now. Most of what's left to do is bookkeeping:

Resolve merge conflicts.
Add README.md and CHANGELOG.json to all three new packages.
Update all CHANGELOG.json for any changed packages.
Get CI passing.
~~Move v1 contracts back into contracts package (required for sol-doc?).~~ (Not required according to Gene).
Update .prettierignore, .gitignore, and other ignore files.

albrow · 2018-10-10T06:28:03Z

packages/contract-artifacts/src/index.ts

+import * as ZRXToken from '../artifacts/ZRXToken.json';
+
+export {
+    AssetProxyOwner,


Note that we don't have to do the (as any) as ContractArtifact hack anymore! With "resolveJsonModule": true in tsconfig.json TypeScript supports typed JSON imports. It also works with Project References, so when you watch, it will recompile when the JSON file(s) change.

albrow · 2018-10-10T06:29:18Z

packages/contract-wrappers/src/contract_wrappers/erc20_token_wrapper.ts

-            artifacts.ERC20Token,
-            normalizedTokenAddress,
-        );
+        // TODO(albrow): Do we really still need this check? The default error


@LogvinovLeon or @fabioberger thoughts on this TODO? (Also appears a few other places).

What is the default error? doesContractExistAtAddressAsync returns a boolean.

What I'm asking is whether we really need the ContractWrappersError.ERC20TokenContractDoesNotExist error. If we remove this check, Web3Wrapper throws an error along the lines of "Tried to send a transaction to 0xabcde... but it is not a contract address.". Together with a stack trace, I think this makes it pretty clear what went wrong. What do you think?

Yep, let's get rid of the mapping.

albrow · 2018-10-10T06:34:50Z

packages/contract-wrappers/test/utils/token_utils.ts

 import { provider, txDefaults, web3Wrapper } from './web3_wrapper';

 // Those addresses come from migrations. They're deterministic so it's relatively safe to hard-code them here.
 // Before we were fetching them from the TokenRegistry but now we can't as it's deprecated and removed.
+// TODO(albrow): Import these from the migrations package instead of hard-coding them.


I'm going to leave this for now. We're no worse off than we were before this PR and can always fix it later. This PR is already arguably too big as it is.

albrow · 2018-10-10T06:44:43Z

packages/contract-wrappers/test/revert_validation_test.ts

@@ -112,7 +107,7 @@ describe('Revert Validation ExchangeWrapper', () => {
                makerTokenBalance,
            );
            await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS);
-            expect(
+            return expect(


This is subtle, but there was a race condition here before adding the return.

Can we add a TSLint rule for this? If not now, let's add a task to the backlog.

➕ Good idea. I believe we already have a check for unhandled promise rejections. The problem is that expect().to.be.rejectedWith doesn't return a real Promise. Instead it returns a Chai.PromisedAssertion. There must be some way to check for it.

albrow · 2018-10-10T06:46:02Z

packages/contract-wrappers/test/exchange_wrapper_test.ts

@@ -454,13 +458,6 @@ describe('ExchangeWrapper', () => {
            })().catch(done);
        });
    });
-    describe('#getZRXTokenAddressAsync', () => {


Since we removed the token registry, this test isn't really doing anything.

albrow · 2018-10-10T06:49:40Z

packages/contract-wrappers/src/contract_wrappers/contract_wrapper.ts

@@ -124,39 +112,8 @@ export abstract class ContractWrapper {
        const logWithDecodedArgs = abiDecoder.tryToDecodeLogOrNoop(log);
        return logWithDecodedArgs;
    }
-    protected async _getContractAbiAndAddressFromArtifactsAsync(


Since we aren't reading addresses from a file anymore, I was able to remove this function and others like it. For ContractWrappers addresses are now known at instantiation time and are synchronous, which simplifies a lot of our code.

fabioberger · 2018-10-10T09:30:40Z

.gitignore

 packages/0x.js/src/generated_contract_wrappers/
-packages/contracts/generated_contract_wrappers/
-packages/contract-wrappers/src/contract_wrappers/generated/
+packages/contracts/generated-wrappers/
 packages/metacoin/src/contract_wrappers
 packages/fill-scenarios/src/generated_contract_wrappers/
 packages/order-watcher/src/generated_contract_wrappers/


Do these still exist? This is where the abi-gen-wrappers used to get copied to.

👍 I have a TODO item on this PR to fix .gitignore

fabioberger · 2018-10-10T09:31:46Z

packages/abi-gen-wrappers/package.json

+    "engines": {
+        "node": ">=6.12"
+    },
+    "description": "Smart contract components of 0x protocol",


"0x smart contract wrappers generated using @0xproject/abi-gen"

fabioberger · 2018-10-10T09:35:51Z

packages/abi-gen-wrappers/src/index.ts

@@ -0,0 +1,14 @@
+export * from '../wrappers/asset_proxy_owner';


Why are the wrappers not under the src dir?

IMO src should be for user-editable source code. These wrappers are generated code. Does that sound right to you?

Hm... I think of src as where all the TS code lived that needs to be compiled into lib. Those generated contracts have disclaimers in their headers, and we could also name the directory to generated_wrappers.

👍 Okay I changed it.

fabioberger · 2018-10-10T09:51:04Z

packages/contract-addresses/src/index.ts

@@ -0,0 +1,64 @@
+import * as _ from 'lodash';
+
+export interface ContractAddresses {


Can we rename this to contractToAddress? We are trying to turn this mapped naming which mimicks the left (key) and right (value) of the interface into a convention.

Hmm.. I'm not sure I agree with renaming it. In this case, I think the kind of information we are passing around (the addresses of deployed contracts) is more important to call out than the underlying data structure (a mapping of contract name to contract address). For similar reasons, it seems awkward to rename getContractAddressesForNetwork to getContractToAddressForNetwork or rename the contractAddresses parameter in the constructor for ContractWrappers to contractToAddress.

To look at it from another perspective, here are a few examples of how we use the XToY naming convention in @0xproject/types:

export interface ExportNameToTypedocNames { [exportName: string]: string[]; } export interface ExternalTypeToLink { [externalTypeName: string]: string; } export interface ExternalExportToLink { [externalExport: string]: string; }

All of these cases represent a mapping of arbitrary X to Y, and that's where the naming convention makes the most sense to me. Since we don't know ahead of time exactly what keys the type will hold (and whether other uses of the type will have the same keys), the underlying data structure is more important to call out. In other words, the data structure itself is what defines the type, not the data it holds.

All that said, if you feel strongly about it, I can be convinced.

Yeah, I see your point in this case. Thanks for the thoughtful explanation, let's leave it the way it is.

fabioberger · 2018-10-10T09:52:37Z

packages/asset-buyer/src/asset_buyer.ts

@@ -109,6 +110,7 @@ export class AssetBuyer {
        this.expiryBufferSeconds = expiryBufferSeconds;
        this._contractWrappers = new ContractWrappers(this.provider, {
            networkId,
+            contractAddresses: getContractAddressesForNetwork(networkId),


Since this is optional, and the fallback is to call this same method under-the-hood, we can omit this line and remove the contract-addresses dep from this package.

@fragosti @BMillman19 do we want people to be able to use AssetBuyer in their own private testnets? If so, we might want to expose this config in the contructor.

Omitted for now. We can add it back in if we think users want to configure it.

fabioberger · 2018-10-10T10:36:50Z

packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts

-    public getZRXTokenAddress(): string {
-        const contractAddress = this._getContractAddress(artifacts.ZRXToken, this._zrxContractAddressIfExists);
-        return contractAddress;
-    }


Hm... any reason to remove these? We could just return it no? It's a big breaking change to remove these... I know relayers rely on that. Do we want to force them to move to using contract-addresses package?

With the changes in this PR, zrxTokenAddress is simply a property of ExchangeWrapper and it will always be set after instantiation. You don't have to use the contract-addresses package if you don't want to. Removing this method simplifies our API surface and will make contract-wrappers easier to use/understand. Leaving it in would mean that there are now two ways to get the address and just creates a little bit of clutter.

Given that AFAIK we don't have any mechanism for marking functions/methods as deprecated, I think we should just remove outdated code like this.

If this was the only breaking change, I would leave it in. However, we already have some breaking changes to the contract-wrappers package in this PR and this one is definitely on the easy side in terms of updating.

fabioberger · 2018-10-10T10:37:40Z

packages/contract-wrappers/src/contract_wrappers/order_validator_wrapper.ts

-     * @param networkId Desired networkId
+     * @param web3Wrapper Web3Wrapper instance to use.
+     * @param networkId Desired networkId.
+     * @param address (Optional) The address of the OrderValidator contract. If


remove (optional)

fabioberger · 2018-10-10T10:40:44Z

packages/contract-wrappers/test/revert_validation_test.ts

@@ -112,7 +107,7 @@ describe('Revert Validation ExchangeWrapper', () => {
                makerTokenBalance,
            );
            await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS);
-            expect(
+            return expect(


Can we add a TSLint rule for this? If not now, let's add a task to the backlog.

fabioberger · 2018-10-10T10:50:21Z

packages/website/ts/blockchain.ts

-            this._contractWrappers = new ContractWrappers(provider, { networkId });
+            const contractWrappersConfig = {
+                networkId,
+                contractAddresses: getContractAddressesForNetwork(networkId),


Omit this. It defaults to doing this internally.

fabioberger · 2018-10-10T10:50:58Z

tsconfig.json

        { "path": "./packages/assert" },
        { "path": "./packages/asset-buyer" },
        { "path": "./packages/base-contract" },
        { "path": "./packages/connect" },
+        { "path": "./packages/contract-artifacts" },


Do we need an entry here for contract-addressses?

LogvinovLeon · 2018-10-10T11:57:38Z

packages/contract-artifacts/tsconfig.json

+    "compilerOptions": {
+        "outDir": "lib",
+        "rootDir": ".",
+        "resolveJsonModule": true


When I tried to do that I've hit a problem that TSC auto-generates types for JSON files from their content and we had some partial artifacts in some modules, but I guess now that you standardized it - it's not a problem any more. Happy about that.

Yes, I still had to include some type assertions in the contracts package for the reasons you mentioned. I think it only works for contract-artifacts right now because we are using the trimmed down contract artifacts there.

albrow · 2018-10-15T20:32:04Z

packages/monorepo-scripts/src/utils/doc_generate_and_upload_utils.ts

@@ -103,6 +103,9 @@ export class DocGenerateAndUploadUtils {
            switch (node.kind) {
                case ts.SyntaxKind.ExportDeclaration: {
                    const exportClause = (node as any).exportClause;
+                    if (_.isUndefined(exportClause)) {


@fabioberger commenting here to make sure you get a chance to review the changes I made to this file. It's easy to miss in a PR of this size.

BMillman19

Approve for website and asset-buyer

albrow · 2018-10-16T07:33:56Z

Removed WIP label. Ready for final review.

…directory OUTSIDE of the monorepo root, so that the installed packages can't reference the hoisted node_modules folder

…ailable to users of the library

…s available to users of the library

… available to users of the library

…it's available to users of the library

…epends on it

fabioberger · 2018-10-16T09:33:59Z

packages/abi-gen-wrappers/README.md

+
+You may also be interested in the
+[@0xproject/contract-wrappers](../contract-wrappers/README.md) package which
+includes some higher-level features.


albrow added the WIP label Oct 2, 2018

fabioberger requested review from LogvinovLeon and fabioberger October 2, 2018 08:18

fabioberger reviewed Oct 2, 2018

View reviewed changes

albrow force-pushed the feature/contracts-artifacts-re-org branch 2 times, most recently from 4df5bb4 to 1c09da9 Compare October 4, 2018 23:42

albrow requested review from BMillman19 and fragosti as code owners October 4, 2018 23:58

albrow force-pushed the feature/contracts-artifacts-re-org branch from 2f66ae8 to 66f407f Compare October 5, 2018 00:36

albrow force-pushed the feature/contracts-artifacts-re-org branch from 608418f to 4326757 Compare October 10, 2018 06:23

albrow commented Oct 10, 2018

View reviewed changes

fabioberger suggested changes Oct 10, 2018

View reviewed changes

LogvinovLeon reviewed Oct 10, 2018

View reviewed changes

albrow force-pushed the feature/contracts-artifacts-re-org branch 2 times, most recently from 653ef71 to 462bf02 Compare October 12, 2018 20:55

albrow mentioned this pull request Oct 15, 2018

[order-utils] fix: add missing devDependency #1137

Closed

5 tasks

albrow requested a review from steveklebanoff as a code owner October 15, 2018 20:30

albrow commented Oct 15, 2018

View reviewed changes

Fix failing doc generation tests

f0e4837

albrow force-pushed the feature/contracts-artifacts-re-org branch from 5472bf2 to f0e4837 Compare October 15, 2018 20:39

albrow added 5 commits October 15, 2018 13:44

Fix bug introduced during rebase

00db096

Remove ContractAddresses from packages/types (mistake after rebase)

be1a597

Update CHANGELOG.json for all changed packages

e3af06a

Add OrThrow suffix to getContractAddressesForNetwork

38b146c

Update json-schemas for contract-wrappers

e093864

albrow force-pushed the feature/contracts-artifacts-re-org branch from ca26a2b to e093864 Compare October 16, 2018 00:09

albrow added 3 commits October 15, 2018 17:35

In contract-wrappers, remove setProvider and add unsubscribeAll method.

6f8e98e

In abi-gen-wrappers, ./wrappers -> ./src/generated-wrappers

2c63909

Update website to use the new unsubscribeAll method in contract-wrappers

5509bf5

BMillman19 approved these changes Oct 16, 2018

View reviewed changes

albrow added 2 commits October 16, 2018 00:00

Update prettierignore

3c5812c

Remove ContractNotFound errors in contract-wrappers

5bdfad9

albrow force-pushed the feature/contracts-artifacts-re-org branch from 7e0e126 to 5bdfad9 Compare October 16, 2018 07:24

albrow changed the title ~~[WIP] Reorganize and modularize generated contract wrappers and artifacts~~ Reorganize and modularize generated contract wrappers and artifacts Oct 16, 2018

albrow removed the WIP label Oct 16, 2018

fabioberger added 8 commits October 16, 2018 13:22

fix(monorepo-scripts): Move the creation of the .installation-test …

8cffe65

…directory OUTSIDE of the monorepo root, so that the installed packages can't reference the hoisted node_modules folder

fix(0x.js): make web3-provider-engine types a 'dependency' so it's av…

2e56e2c

…ailable to users of the library

fix(dev-utils): make web3-provider-engine types a 'dependency' so it'…

eab88cb

…s available to users of the library

fix(sol-cov): make @types/solidity-parser-antlr a 'dependency' so it'…

75b0781

…s available to users of the library

fix(sra-spec): make @loopback/openapi-v3-types a 'dependency' so it's…

6c92778

… available to users of the library

fix(subproviders): make web3-provider-engine types a 'dependency' so …

0210411

…it's available to users of the library

Add changelog entries

fca5260

fix(dev-utils): Make chai a dev dependency since exported interface d…

5938e8a

…epends on it

fabioberger approved these changes Oct 16, 2018

View reviewed changes

fabioberger merged commit e624587 into development Oct 16, 2018

fabioberger deleted the feature/contracts-artifacts-re-org branch October 16, 2018 13:21

albrow mentioned this pull request Oct 18, 2018

Update migrations README. #1151

Merged

5 tasks

		@@ -0,0 +1,14 @@
		export * from '../wrappers/asset_proxy_owner';

		@@ -0,0 +1,64 @@
		import * as _ from 'lodash';

		export interface ContractAddresses {

Reorganize and modularize generated contract wrappers and artifacts #1105

Reorganize and modularize generated contract wrappers and artifacts #1105

Conversation

albrow commented Oct 2, 2018 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Oct 2, 2018 • edited Loading

LogvinovLeon commented Oct 2, 2018

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

albrow commented Oct 2, 2018 • edited Loading

fabioberger commented Oct 3, 2018

albrow commented Oct 3, 2018 • edited Loading

albrow commented Oct 9, 2018 • edited Loading

albrow commented Oct 10, 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

albrow Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

fabioberger Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

albrow Oct 16, 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

albrow Oct 10, 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

BMillman19 left a comment

Choose a reason for hiding this comment

albrow commented Oct 16, 2018

Choose a reason for hiding this comment

albrow commented Oct 2, 2018 •

edited

Loading

coveralls commented Oct 2, 2018 •

edited

Loading

albrow commented Oct 2, 2018 •

edited

Loading

albrow commented Oct 3, 2018 •

edited

Loading

albrow commented Oct 9, 2018 •

edited

Loading

albrow commented Oct 10, 2018 •

edited

Loading

albrow Oct 10, 2018 •

edited

Loading

fabioberger Oct 15, 2018 •

edited

Loading

albrow Oct 16, 2018 •

edited

Loading

albrow Oct 10, 2018 •

edited

Loading