Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vendor ethPM #1379

Merged
merged 10 commits into from Jul 11, 2019
Merged

Vendor ethPM #1379

merged 10 commits into from Jul 11, 2019

Conversation

@njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Jul 9, 2019

What was wrong?

Things that happened here (aka why is this pr so large)

  • py-ethpm codebase was relocated to web3/ethpm
  • critical components moved from pytest-ethereum to web3.tools.pytest_ethereum
  • pytest-ethereum plugins exposed via entrypoints in setup.py
  • removed web3's core dependencies on py-ethpm and pytest-ethereum
  • added a couple core dependencies necessary for ethpm
  • a local ipfs node now runs in the background of circle builds
  • ethpm documentation has been moved to web3 docs

Things to note

  • No code has been changed / needs review in the migrated files, so the general structure of the migrated files is what primarily needs review in this pr.

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the migrate-ethpm branch 2 times, most recently from eb4b660 to 712605d Jul 9, 2019
@voith
Copy link
Contributor

@voith voith commented Jul 10, 2019

@njgheorghita Out of curiosity, what is the reason behind relocating ethpm to web3.py?

@njgheorghita
Copy link
Contributor Author

@njgheorghita njgheorghita commented Jul 10, 2019

@voith Totally fair, it basically comes down to a circular dependency between web3 and ethpm that's causing some issues. web3 depends on ethpm concepts in web3.pm and ethpm uses web3 concepts liberally. My impression is the best solution to remove this dependency is to vendor ethpm and all relevant pytest-ethereum code in web3. It adds a fair amount of code overhead to the web3 codebase, but the goal is to keep it well separated from the web3 core code so it doesn't get messy.

@njgheorghita njgheorghita force-pushed the migrate-ethpm branch 3 times, most recently from 96ba040 to 4dfec57 Jul 10, 2019
@njgheorghita njgheorghita changed the title [WIP] Vendor ethPM Vendor ethPM Jul 10, 2019
Copy link
Collaborator

@kclowes kclowes left a comment

This looks good to me, @njgheorghita! I just had one question while looking over this: what is the reasoning behind removing the pytest-ethereum dependency? Is it another circular dependency problem?

@njgheorghita
Copy link
Contributor Author

@njgheorghita njgheorghita commented Jul 10, 2019

@kclowes Yup! Exactly, pytest-ethereum has the same circular dependency problem with web3 that ethpm had. However, since pytest-ethereum doesn't rely on web3 as heavily as ethpm, only the few core components were migrated into the web3 codebase rather than the entire library. So, pytest-ethereum will continue to exist as a wrapper around and extensions beyond these new web3 components.

carver
carver approved these changes Jul 10, 2019
Copy link
Collaborator

@carver carver left a comment

I won't be able to review the ethpm package, it's way too big. Just gave a quick scan of everything else.

@@ -9,6 +9,17 @@ common: &common
- restore_cache:
keys:
- cache-{{ .Environment.CIRCLE_JOB }}-{{ checksum "setup.py" }}-{{ checksum "tox.ini" }}
- run:
name: install ipfs
Copy link
Collaborator

@carver carver Jul 10, 2019

Is this necessary for every test now, or just the new ethpm ones?

docs/ethpm.rst Outdated
>>> from ethpm import Package, ASSETS_DIR
>>> from web3 import Web3
>>> owned_manifest_path = ASSETS_DIR / 'owned' / '1.0.0.json'
Copy link
Collaborator

@carver carver Jul 10, 2019

Could move this down to the next batch of lines, since it's not used yet.

>>> safe_send_tx_receipt = w3.eth.waitForTransactionReceipt(safe_send_tx_hash)
>>> # Link Escrow factory to deployed SafeSendLib instance
>>> LinkedEscrowFactory = EscrowFactory.link_bytecode({"SafeSendLib": to_canonical_address(safe_send_tx_receipt.contractAddress)})
Copy link
Collaborator

@carver carver Jul 10, 2019

Having to convert to a canonical address feels out of place now that ethpm is embedded with web3.py. Seems like it should follow the same standard of the rest of web3.py: either a checksummed address or a canonical address is fine.

Copy link
Contributor Author

@njgheorghita njgheorghita Jul 10, 2019

Yup, that makes sense - will take care of this in a following pr to keep review of this pr relatively simple

docs/ethpm.rst Outdated

.. py:attribute:: LinkableContract.linked_references
A list of link reference data for the runtime bytecode, if present in the manifest data used to generate a ``LinkableContract`` factory. Runtime bytecode link reference data must be present in a manifest in order to use ``pytest-ethereum``'s ``Deployer`` for a contract which requires bytecode linking.
Copy link
Collaborator

@carver carver Jul 10, 2019

Is this second sentence equivalent to:

If you want to use pytest-ethereum's Deployer for a contract, then runtime bytecode link reference data must be present in a manifest.

I still don't know exactly what it means, because I've never used Deployer. But in this phrasing, I know I don't have to worry about it, because I'm not using Deployer.

@@ -9,7 +9,6 @@
'tester': [
"eth-tester[py-evm]==0.1.0-beta.39",
"py-geth>=2.0.1,<3.0.0",
"pytest-ethereum>=0.1.3a6,<1.0.0",
Copy link
Collaborator

@carver carver Jul 10, 2019

Sweet

@njgheorghita njgheorghita merged commit 598c852 into ethereum:master Jul 11, 2019
28 checks passed
@njgheorghita njgheorghita deleted the migrate-ethpm branch Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants