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

Auto-gen Python Exchange wrapper #1919

Merged
merged 44 commits into from
Jul 23, 2019

Conversation

feuGeneA
Copy link
Contributor

@feuGeneA feuGeneA commented Jul 3, 2019

Fixes #1888 .

Description

Include Exchange in the list of ABI's that packages/python-contract-wrapper is currently generating wrappers for, and change python-packages/contract_wrappers to pull in that code (overwriting the existing, manually-written wrapper) before linting and testing itself. Existing tests/examples should all pass with minimal changes (ideally none).

Testing instructions

Since the end result is generated code, reviewing that code (and the docs generated from it) is the first step in testing.

I found some CircleCI configuration to persist "Build Artifacts," which accurately describes our generated code. With this, you can go to a build job's "Artifacts" tab (here's a link to the "Artifacts" tab on a recent run of test-python), and drill down through the repo tree to find persisted artifacts.

I'm using this to persist generated Python wrappers, generated test output from abi-gen's command-line tests, and documentation generated from the Python code. Here are the links to the artifacts from the latest CircleCI run:


If you want to actually run the generation process yourself:

0x-monorepo/python-packages$ ./pre_install
0x-monorepo/python-packages$ ./install_editable
0x-monorepo/python-packages/contract_wrappers$ PKG=@0x/python-contract-wrappers yarn build && ./setup.py pre_install && ./setup.py lint && ./setup.py test
[edit code]
0x-monorepo/python-packages/contract_wrappers$ PKG=@0x/python-contract-wrappers yarn build && ./setup.py pre_install && ./setup.py lint && ./setup.py test
[repeat]

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Rename contract methods parameters in existing exchange contract to match contract method parameter names
  • Rename existing wraper from exchange_wrapper.py to just exchange.py.
  • Fix faulty encoding of Order struct tuple in generated wrapper methods. See CI failure.
  • Generate documentation from the generated wrapper and make sure it looks okay.
  • Document bytes parameter methods as requiring a UTF-8 encoding.
  • Stop generating TypedDicts for structs that are only used as the return value for non-const methods, since there's no generated reference to such a TypedDict, because those return values aren't accessible since a non-const method can only return a transaction hash.
  • Document flags used in abi-gen/diff.sh. Also, move that diff.sh into test-cli, since that's the only place it's ever used, and remove the parameters, since it's always called with the same arguments, and also rename it to run_diff_test.sh.
  • In abi-gen/test-cli/fixtures/contracts/AbiGenDummy.sol, rename Event to AnEvent.
  • .gitignore JSON schema files in python package area, and also generated wrappers in python area.
  • Review old, manually-written wrapper to see if there are other things that need to be ported over.
  • Add JSON schema validation.
  • Rebase on development in order to fix test-publish failure.
  • Add signature validation. Decided not to do this; see comment below.
  • Add new entries to the relevant CHANGELOG.jsons.
  • When generated methods are called with view_only=True (to do an eth_call instead of an eth_sendTransaction) the method should return the value returned by the method, not return a transaction hash.
  • Fix bug with wrong return value for some generated methods, shown in this comment below.
  • Raise follow-on Issue: Add refined types to output values of get_*_event() methods. Currently they're all just Tuple[AttributeDict].
  • Raise follow-on Issue: Add support for accepting signatures as strings (not just bytes).

@feuGeneA feuGeneA self-assigned this Jul 3, 2019
@feuGeneA feuGeneA added @0x/python-contract-wrappers Python wrappers for 0x contracts, generated by Handlebars.js, to be consumed by Python packages. python Multi-language Support labels Jul 3, 2019
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch 2 times, most recently from 8943621 to 9dbabb2 Compare July 11, 2019 17:12
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch 5 times, most recently from ba02cf4 to ca52224 Compare July 12, 2019 00:43
@coveralls
Copy link

coveralls commented Jul 12, 2019

Coverage Status

Coverage decreased (-0.1%) to 83.121% when pulling 0f6cd5d on feature/python/generate-exchange-wrapper into c59d886 on development.

@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch from ca52224 to 10e9523 Compare July 12, 2019 01:29
@feuGeneA
Copy link
Contributor Author

This branch is currently in a good checkpoint state. The Exchange contract is being generated and run through the tests and doctest examples with every build, and are clean in CircleCI (except for the test-publish failure, quoted below).

However, generated wrappers are currently not yet doing any JSON schema validation, nor any signature validation. There may also be other specially crafted logic in there too, which would need to be ported over, but at this time all of the examples/tests are using this generated wrapper and passing. These omissions are reflected in the checklist in the PR description.

The generated Exchange wrapper, as of this point in time, can be seen here: https://gist.github.com/feuGeneA/de3754c87d3732e8a2e2e69e53da78de

The pre-existing, manually-written Exchange wrapper, can be seen in the commit that deleted it.

For the reviewer's convenience, here is a diff of the two: https://www.diffchecker.com/7tEnPG7v . Sadly, there is a lot of change in the diff.

test-publish is failing with

ERROR in package @0x/contracts-exchange:
Command failed: npm install --save @0x/[email protected] --registry=http://localhost:4873/
npm WARN deprecated [email protected]: This module is no longer maintained, try this instead:
npm WARN deprecated   npm i nyc
npm WARN deprecated Visit https://istanbul.js.org/integrations for other alternatives.

🤷‍♂️ a re-run didn't fix it...

packages/abi-gen/diff.sh Show resolved Hide resolved
packages/abi-gen/src/utils.ts Outdated Show resolved Hide resolved
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch 7 times, most recently from e14fab8 to 5071f26 Compare July 16, 2019 23:38
feuGeneA added 6 commits July 17, 2019 23:27
This leaves space for user-defined additions to the same module, such as
for custom types, as shown herein.
For abi-gen command-line test output, for generated Python contract
wrappers as output by abi-gen, for generated Python contract wrappers as
reformatted and included in the Python package area, and for the "build"
output folder in each Python package, which includes the generated
documentation.
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch from 5e524cf to 6833c91 Compare July 18, 2019 03:29
@feuGeneA
Copy link
Contributor Author

@fabioberger @xianny @PirosB3 this PR is pretty big, but not as big as it looks.

It says I added 2,300 new lines of code, but a lot of that is generated: expected outputs for those new abi-gen CLI tests, pre-compiled contract artifacts for abi-gen test fixtures, etc.

And there are 30 commits, but many of them are very small. There are maybe 2 or 3 commits that are pretty big and hairy, and maybe 3 or 4 medium sized ones, and the rest are small to tiny.

@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch from 33af363 to 7a0d5a8 Compare July 18, 2019 04:36
.circleci/config.yml Show resolved Hide resolved
.gitignore Show resolved Hide resolved
packages/abi-gen-templates/CHANGELOG.json Outdated Show resolved Hide resolved
packages/abi-gen-templates/CHANGELOG.json Outdated Show resolved Hide resolved
packages/abi-gen/CHANGELOG.json Outdated Show resolved Hide resolved
packages/abi-gen/src/index.ts Outdated Show resolved Hide resolved
packages/abi-gen/src/index.ts Show resolved Hide resolved
python-packages/order_utils/CHANGELOG.md Outdated Show resolved Hide resolved
@feuGeneA feuGeneA requested a review from fabioberger July 18, 2019 14:36
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch from 4634f4a to c01f76f Compare July 18, 2019 20:29
@fabioberger
Copy link
Contributor

Looking great @feuGeneA! 🎉

feuGeneA added 3 commits July 19, 2019 23:10
Specifically, wrapper methods wrapping contract methods that modify
contract state and return no return value.  There was no test case for
this.  Now there is.
@feuGeneA feuGeneA force-pushed the feature/python/generate-exchange-wrapper branch from a5ec3f9 to e2d932f Compare July 20, 2019 03:28
@feuGeneA feuGeneA merged commit ead8099 into development Jul 23, 2019
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.

Auto-generate Python wrapper for the Exchange contract
3 participants