Add workflow for copying compiled artifacts to 0x/contract-artifacts #1842

xianny · 2019-05-31T21:08:49Z

Description

Currently we have to copy compiled artifacts manually into @0x/contract-artifacts. This is tedious and error-prone.

I added a contracts:postcompile step to copy the compiled artifacts into the packages/contract-artifacts/artifacts. This will only copy files that are exported from the @0x/contract-artifacts package.

I also added a linter script to remove the unneeded properties from the compiler output, so that we don't unnecessarily bloat bundle size.

The workflow is as follows:

Run yarn contracts:compile as usual. This compiles and then copies the files to contract-artifacts. There will be a prompt to lint contract-artifacts.
run yarn lint inside contract-artifacts, then run yarn test to confirm it works.

Question: Should we add the lint step to the compile+postcompile step?

Part II

This PR also updates the artifacts to include deployedBytecode which is needed for the multilang project.

Testing instructions

Follow above workflow.

Types of changes

New feature (non-breaking change which adds functionality)

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.

albrow · 2019-05-31T21:12:00Z

Let's make sure that @0x/contract-artifacts only contains artifacts that match up with the currently deployed smart contracts. If we change any smart contracts in preparation for v3 we should not update @0x/contract-artifacts until those new contracts are fully deployed.

albrow · 2019-05-31T21:17:47Z

This PR does change a lot of the artifacts, but for the most part they are just formatting changes that remove excess whitespace. However, there are changes to the "bytecode" and "deployedBytecode" sections. I'm not familiar with exactly what these are used for. Can someone confirm that these changes are okay and aren't going to break anything?

packages/contract-artifacts/artifacts/ZRXToken.json

packages/monorepo-scripts/src/copy_contract_artifacts.ts

packages/sol-compiler/src/compiler.ts

fabioberger · 2019-06-03T10:04:01Z

I agree with @albrow. Copying of the new artifacts into @0x/contract-artifacts should not happen everytime the contracts are re-built. There is a deliberate separation between contracts and our tooling so that we only copy the artifacts over before a new protocol upgrade. Up until that point, contract changes should not affect our tooling. Because of this, I suggest we simply make the copy helper script a separate command that does not run as part of the contract build step. Make sense?

packages/contract-artifacts/src/lint_properties.ts

packages/monorepo-scripts/src/copy_contract_artifacts.ts

xianny · 2019-06-04T05:08:22Z

@albrow @fabioberger, gotcha on updating contract-artifacts being a separate deliberate step.

Can we guarantee/assume that any contract development happens in a separate branch and is published immediately after merging? Or some other way to keep track of what is deployed? Otherwise, if there are incremental un-deployed contract changes, how do we generate artifacts to use in monorepo development?

For example right now Exchange.sol has string constant public VERSION = "3.0.0"; which causes a test to fail and alerts me that the artifact is out of sync with what is deployed. I could still regenerate v2 artifacts if there was a git tag for the state of the contract code at the moment of v2 deployment. But if there were incremental changes to the contracts that did not include updating the version number, it's really hard to find the state of the code when it was deployed.

fabioberger · 2019-06-04T10:15:10Z

Thanks @xianny.

Yeah, that's really annoying. We didn't do a good job of having the protocol team work on V3 in a separate branch from the get-go. Typically that should happen moving forward. Right @abandeali1?

That being said, updating the artifacts in our tooling should still remain a separate deliberate step.

coveralls · 2019-06-07T21:50:13Z

Coverage increased (+0.02%) to 83.375% when pulling f906cee on feature/prepublish-contract-artifacts into ecf939f on development.

xianny · 2019-06-07T22:36:40Z

@fabioberger @dekz ready for re-review. Much simpler changes now.

@hysz and I discussed on Slack and decided that the contract artifacts should remain as standard compiler output during development. Fields should only be removed/added prior to being published in @0x/contract-artifacts. Consequently we now only have the yarn lint shortcut to contract-artifacts and its dependent abi-gen-wrappers.

This means the abi-gen-templates will have to support both standard artifacts and modified artifacts if they diverge, since the templates are used for contract-artifacts/abi-gen-wrappers (modified artifact) and development in all the contracts/* packages (standard artifact). This isn't relevant yet but will become relevant when we need to modify the deployedBytecode field within artifacts as part of #1829.

This PR makes the following changes to contract-artifacts:

remove evm.bytecode.linkReferences from all artifacts
remove evm.deployedBytecode and sourceTreeHashHex from Coordinator artifact
prettify all artifacts (whitespace only changes)

fabioberger · 2019-06-10T23:08:06Z

packages/contract-artifacts/package.json

        "build": "yarn tsc -b",
        "build:ci": "yarn build",
        "test": "yarn run_mocha",
        "test:circleci": "yarn test",
        "run_mocha": "mocha --require source-map-support/register --require make-promises-safe lib/test/**/*_test.js --bail --exit",
        "clean": "shx rm -rf lib"
    },
+    "config": {
+        "contractsPackages": "@0x/contracts-asset-proxy @0x/contracts-erc20 @0x/contracts-erc721 @0x/contracts-erc1155 @0x/contracts-exchange @0x/contracts-exchange-forwarder @0x/contracts-exchange-libs @0x/contracts-extensions @0x/contracts-multisig @0x/contracts-test-utils @0x/contracts-utils @0x/contracts-coordinator"


In the script, let's actually read it from the root package.json instead of duplicating this here.

To get this to build, I had to change rootDir: ./ to rootDir: ../.. in the tsconfig.json. Waiting for CI to see if it causes any issues.

This wound up breaking relative links by introducing nesting to the build output. I'm not sure it's worth it to introduce this weirdness here.

Build output:

lib ├── package.json └── packages └── contract-artifacts ├── artifacts │ ├── AssetProxyOwner.json │ ├── Coordinator.json │ ├── CoordinatorRegistry.json │ ├── DummyERC20Token.json │ ├── DummyERC721Token.json │ ├── DutchAuction.json │ ├── ERC20Proxy.json │ ├── ERC20Token.json │ ├── ERC721Proxy.json │ ├── ERC721Token.json │ ├── Exchange.json │ ├── Forwarder.json │ ├── IAssetProxy.json │ ├── IValidator.json │ ├── IWallet.json │ ├── MultiAssetProxy.json │ ├── OrderValidator.json │ ├── WETH9.json │ └── ZRXToken.json ├── src │ ├── copy.d.ts │ ├── copy.d.ts.map │ ├── copy.js │ ├── copy.js.map │ ├── index.d.ts │ ├── index.d.ts.map │ ├── index.js │ ├── index.js.map │ ├── transform.d.ts │ ├── transform.d.ts.map │ ├── transform.js │ └── transform.js.map └── test ├── contract_artifacts_test.d.ts ├── contract_artifacts_test.d.ts.map ├── contract_artifacts_test.js └── contract_artifacts_test.js.map

Let's use fs.readFile

packages/contract-artifacts/src/properties.ts

packages/contract-artifacts/test/contract_artifacts_test.ts

packages/contract-artifacts/package.json

packages/monorepo-scripts/src/contract_artifacts_copy.ts

fabioberger · 2019-06-10T23:30:11Z

packages/monorepo-scripts/src/contract_artifacts_copy.ts

+    fs.copyFileSync(_path, targetPathPython);
+    utils.log(`Copied from ${_path} to ${targetPath}`);
+}
+utils.log(`Finished copying contract-artifacts. Remember to lint artifacts before using abi-gen.`);


Suggested change

utils.log(`Finished copying contract-artifacts. Remember to lint artifacts before using abi-gen.`);

utils.log(`Finished copying contract-artifacts.`);

The transform happens automatically now right?

Not necessarily. The yarn shortcut runs copy && transform, but this script only does copy, so I think we should keep this log line in case the script is used alone.

The resulting output looks like this:

[17:24:58]contract-artifacts (feature/prepublish-contract-artifacts)$ yarn artifacts_update yarn run v1.9.4 $ yarn artifacts_copy && yarn artifacts_transform $ node lib/src/copy.js ${npm_package_config_contractsPackages} Finished copying contract-artifacts. Remember to transform artifacts before publishing or using abi-gen $ node lib/src/transform.js ./artifacts && prettier --write ./artifacts/*.json && cp -r ./artifacts/ ../../python-packages/contract_artifacts/src/zero_ex/contract_artifacts/artifacts/ Deleting forbidden properties from artifacts in ./artifacts. Output to ./artifacts artifacts/AssetProxyOwner.json 117ms artifacts/Coordinator.json 37ms artifacts/CoordinatorRegistry.json 22ms artifacts/DummyERC20Token.json 27ms artifacts/DummyERC721Token.json 37ms artifacts/DutchAuction.json 22ms artifacts/ERC20Proxy.json 27ms artifacts/ERC20Token.json 25ms artifacts/ERC721Proxy.json 23ms artifacts/ERC721Token.json 19ms artifacts/Exchange.json 74ms artifacts/Forwarder.json 20ms artifacts/IAssetProxy.json 9ms artifacts/IValidator.json 8ms artifacts/IWallet.json 7ms artifacts/MultiAssetProxy.json 12ms artifacts/OrderValidator.json 21ms artifacts/WETH9.json 14ms artifacts/ZRXToken.json 13ms ✨ Done in 2.54s. [17:25:12]contract-artifacts (feature/prepublish-contract-artifacts)$

packages/monorepo-scripts/src/contract_artifacts_copy.ts

packages/monorepo-scripts/src/contract_artifacts_transform.ts

fabioberger · 2019-06-12T16:52:56Z

packages/contract-artifacts/src/transform.ts

+import { deleteNestedProperty, logUtils } from '@0x/utils';
+import * as fs from 'fs';
+
+export const REQUIRE_PROPERTIES: string[] = [


Suggested change

export const REQUIRE_PROPERTIES: string[] = [

export const REQUIRED_PROPERTIES: string[] = [

…xProject#1842) The contract artifacts should remain as standard compiler output during development. Fields should only be removed/added prior to being published in `@0x/contract-artifacts`. This PR adds the `yarn transform` script to `@0x/contract-artifacts` to facilitate this. Going forward, `abi-gen-templates` will have to support both standard artifacts and modified artifacts if they diverge, since the templates are used for `contract-artifacts`/`abi-gen-wrappers` (modified artifact) *and* development in all the `contracts/*` packages (standard artifact). This PR makes the following changes to `contract-artifacts`: - remove `evm.bytecode.linkReferences` from all artifacts - remove `evm.deployedBytecode` and `sourceTreeHashHex` from Coordinator artifact - prettify all artifacts (whitespace only changes)

xianny requested review from albrow and fabioberger as code owners May 31, 2019 21:08

probot-autolabeler bot added @0x/contract-artifacts @0x/monorepo-scripts @0x/sol-compiler labels May 31, 2019

xianny requested review from dekz and removed request for albrow May 31, 2019 21:08

dekz reviewed May 31, 2019

View reviewed changes

fabioberger suggested changes Jun 3, 2019

View reviewed changes

xianny mentioned this pull request Jun 4, 2019

Add parameter assertions to abi-gen-wrappers #1823

Merged

4 tasks

xianny requested review from abandeali1 and hysz as code owners June 6, 2019 19:34

probot-autolabeler bot added @0x/abi-gen-wrappers contracts labels Jun 6, 2019

xianny added 6 commits June 6, 2019 12:56

add workflow for copying compiled artifacts to 0x/contract-artifacts

654dbd3

update changelog

5c256a5

cleanup logging, error handling, readability

f17518d

modularise workflow and remove more unwanted properties

98cb2f1

cleanup

67c19fe

cleanup again

a0333dd

xianny force-pushed the feature/prepublish-contract-artifacts branch from 5ab73ed to a0333dd Compare June 6, 2019 19:56

xianny added 4 commits June 6, 2019 12:57

fix import

9114936

cleanup again

f609a3c

fix imports again

ccb154b

reset contract artifacts

145c781

xianny added 3 commits June 7, 2019 15:00

update contract artifacts scripts

a167b40

fix comment

314b765

prettier

60056f7

xianny requested review from fabioberger and dekz June 7, 2019 22:36

xianny added 2 commits June 10, 2019 14:46

run prettier on the artifacts to minimise diffs from future runs

dabb63f

add python artifacts

7b3bcb9

xianny requested a review from feuGeneA as a code owner June 10, 2019 22:04

probot-autolabeler bot added the python label Jun 10, 2019

rename lint -> transform

1e2f48e

fabioberger suggested changes Jun 10, 2019

View reviewed changes

changes after review

ede035a

probot-autolabeler bot added the @0x/utils label Jun 11, 2019

xianny added 9 commits June 10, 2019 17:42

try referencing contractPackages from root package.json

e18865f

revert e18865f

d74d60c

strip unneeded fields from artifacts

fcd7fe2

add doc comment

7443b58

Merge branch 'development' into feature/prepublish-contract-artifacts

a581713

update yarn.lock

86c94ee

export const instead of function

38ea4d5

update changelogs and package.json

4fe5a50

read contractsPackages from root package.json

4d6a0b8

fabioberger approved these changes Jun 12, 2019

View reviewed changes

xianny added 2 commits June 14, 2019 11:08

small cleanups

c813fc2

reproduce ci error

f906cee

xianny merged commit bd22803 into development Jun 17, 2019

xianny deleted the feature/prepublish-contract-artifacts branch June 17, 2019 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workflow for copying compiled artifacts to 0x/contract-artifacts #1842

Add workflow for copying compiled artifacts to 0x/contract-artifacts #1842

xianny commented May 31, 2019 •

edited

Loading

albrow commented May 31, 2019

albrow commented May 31, 2019 •

edited

Loading

fabioberger commented Jun 3, 2019

xianny commented Jun 4, 2019 •

edited

Loading

fabioberger commented Jun 4, 2019

coveralls commented Jun 7, 2019 •

edited

Loading

xianny commented Jun 7, 2019 •

edited

Loading

fabioberger Jun 10, 2019

xianny Jun 11, 2019

xianny Jun 11, 2019

fabioberger Jun 11, 2019

fabioberger Jun 10, 2019

xianny Jun 11, 2019

fabioberger Jun 11, 2019

fabioberger Jun 12, 2019

	utils.log(`Finished copying contract-artifacts. Remember to lint artifacts before using abi-gen.`);
	utils.log(`Finished copying contract-artifacts.`);

	export const REQUIRE_PROPERTIES: string[] = [
	export const REQUIRED_PROPERTIES: string[] = [

Add workflow for copying compiled artifacts to 0x/contract-artifacts #1842

Add workflow for copying compiled artifacts to 0x/contract-artifacts #1842

Conversation

xianny commented May 31, 2019 • edited Loading

Description

Part II

Testing instructions

Types of changes

Checklist:

albrow commented May 31, 2019

albrow commented May 31, 2019 • edited Loading

fabioberger commented Jun 3, 2019

xianny commented Jun 4, 2019 • edited Loading

fabioberger commented Jun 4, 2019

coveralls commented Jun 7, 2019 • edited Loading

xianny commented Jun 7, 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

xianny commented May 31, 2019 •

edited

Loading

albrow commented May 31, 2019 •

edited

Loading

xianny commented Jun 4, 2019 •

edited

Loading

coveralls commented Jun 7, 2019 •

edited

Loading

xianny commented Jun 7, 2019 •

edited

Loading