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

Python contract demo, with lots of refactoring #1485

Merged
merged 18 commits into from
Jan 9, 2019

Conversation

feuGeneA
Copy link
Contributor

@feuGeneA feuGeneA commented Jan 4, 2019

Description

Implemented a demonstration call to Exchange.getOrderInfo(), in python-packages/contract_demo/test/test_exchange.py. (Asana task)

And, did quite a bit of refactoring, including:

  • Changing the data types of some fields in the Order class so that it will be compatible with web3, allowing you to pass an instance of the class as the argument to getOrderInfo().
  • Some utilities to convert between order representations. One representation is for compatibility with the JSON schema validation, and the other is for compatibility with web3. (Asana task)
  • Getting all of the contract addresses into python (only had Exchange before), and into their own package (0x-contract-addresses). (Asana task)
  • Putting a 0x-contract-artifacts package in place. (Asana task)
  • Adding some top-level scripts to be able to execute commands (install, test, lint, etc) against all packages simultaneously.

Also note that I had to duplicate both the JSON schemas and the contract artifacts that are stored in the TypeScript packages. I tried to use symlinks, and tried a few other hacky things, but I couldn't get around this duplication. To make myself feel better about it, I added an additional "linter," to both contract-addresses and contract-artifacts, that's just a diff between what's in the python package and what's in the TypeScript package, so that builds will fail if they end up diverging for some reason.

Rendered docs:

Testing instructions

It's all in CI!

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.

Changed some of the fields in the Order class so that it can be passed
to our contracts via Web3.

Added conversion utilities so that an Order can be easily converted to
and from a JSON-compatible dict (specifically by encoding/decoding the
`bytes` fields), to facilitate validation against the JSON schema.

Also modified JSON order schema to accept integers in addition to
stringified integers.
Has-types indicator file was not being included in package.

Schemas were not being properly gathered into package installation.
@LogvinovLeon
Copy link
Contributor

I think I was requested to review that because I'm a CODEOWNER of .circle.yml. That file changes look good. Didn't look too closely into other changes. It's a shame we can't re-use schemas. I will maybe give it one more try. I like the solution with a linter command, but it will be brilliant if we can figure it out. Otherwise we'll have that problem in any language in the future. Maybe we can download the npm package in the python folder and unpack it and include files from there. Maybe we can put schemas outside of TS packages and reference it upwards.

@feuGeneA
Copy link
Contributor Author

feuGeneA commented Jan 8, 2019

@LogvinovLeon

Otherwise we'll have that problem in any language in the future.

I think the problem is specific to Python, in particular this bug with its packaging tools not following symlinks.

Maybe we can download the npm package in the python folder and unpack it and include files from there.

That would require using a custom docker image in Circle CI, since their standard Python image doesn't include npm. Wouldn't be too much work, but I think there's an advantage to keeping things simple enough to use their regularly-maintained image.

Maybe we can put schemas outside of TS packages and reference it upwards.

Don't think that would work for this case, as it would still be just a relative path from the python package source, which would still need a symlink. Maybe I'm missing something though...

@coveralls
Copy link

coveralls commented Jan 8, 2019

Coverage Status

Coverage decreased (-0.008%) to 85.461% when pulling ced598c on feature/python-get-order-info into 4689f20 on development.

@feuGeneA
Copy link
Contributor Author

feuGeneA commented Jan 8, 2019

@fabioberger I addressed all your requests.

@feuGeneA feuGeneA merged commit aa5af04 into development Jan 9, 2019
@feuGeneA feuGeneA deleted the feature/python-get-order-info branch March 13, 2019 23:52
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.

4 participants