Vendor ethPM #1379

njgheorghita · 2019-07-09T15:07:21Z

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

voith · 2019-07-10T12:24:22Z

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

njgheorghita · 2019-07-10T15:21:00Z

@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.

kclowes

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 · 2019-07-10T20:57:23Z

@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

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

carver · 2019-07-10T20:00:24Z

.circleci/config.yml

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


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

carver · 2019-07-10T20:30:46Z

docs/ethpm.rst

+   >>> from ethpm import Package, ASSETS_DIR
+   >>> from web3 import Web3
+
+   >>> owned_manifest_path = ASSETS_DIR / 'owned' / '1.0.0.json'


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

carver · 2019-07-10T20:36:52Z

docs/ethpm.rst

+   >>> 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)})


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.

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

carver · 2019-07-10T20:41:07Z

docs/ethpm.rst

+
+.. 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.


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.

carver · 2019-07-10T21:07:19Z

setup.py

@@ -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",


Initial migration of ethpm codebase

41ab477

njgheorghita force-pushed the migrate-ethpm branch 2 times, most recently from eb4b660 to 712605d Compare July 9, 2019 15:12

Add ethpm environment to tox

309186c

njgheorghita force-pushed the migrate-ethpm branch 2 times, most recently from b8c3190 to b9a9079 Compare July 9, 2019 17:25

njgheorghita mentioned this pull request Jul 9, 2019

Tweaks to reflect breaking changes in ethpm refactor #1374

Closed

Move pytest-ethereum code to web3.tools

8cff826

njgheorghita force-pushed the migrate-ethpm branch from b9a9079 to 8cff826 Compare July 9, 2019 17:29

njgheorghita added 3 commits July 9, 2019 11:43

Fix linting errors

b15062c

Isort all new files

c6cbb8f

Fix import from pytest-ethereum error

faaabd6

njgheorghita force-pushed the migrate-ethpm branch from 4bd5bed to faaabd6 Compare July 9, 2019 18:14

Run local ipfs in background in circle

5d18a70

njgheorghita force-pushed the migrate-ethpm branch from 75317ce to 5d18a70 Compare July 9, 2019 19:38

njgheorghita force-pushed the migrate-ethpm branch 3 times, most recently from 96ba040 to 4dfec57 Compare July 10, 2019 16:04

njgheorghita changed the title ~~[WIP] Vendor ethPM~~ Vendor ethPM Jul 10, 2019

Move ethpm documentation over to web3 docs

968cebd

njgheorghita force-pushed the migrate-ethpm branch from 4dfec57 to 968cebd Compare July 10, 2019 17:00

njgheorghita requested review from pipermerriam, carver and kclowes July 10, 2019 17:11

kclowes approved these changes Jul 10, 2019

View reviewed changes

carver approved these changes Jul 10, 2019

View reviewed changes

Remove unused pytest exception

a7b9846

Move background ipfs node to ethpm only environments in circle

ea824f1

njgheorghita merged commit 598c852 into ethereum:master Jul 11, 2019

njgheorghita deleted the migrate-ethpm branch July 11, 2019 18:28

njgheorghita mentioned this pull request Jul 11, 2019

Relax ethpm canonical address requirement #1380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor ethPM #1379

Vendor ethPM #1379

njgheorghita commented Jul 9, 2019 •

edited

Loading

voith commented Jul 10, 2019

njgheorghita commented Jul 10, 2019 •

edited

Loading

kclowes left a comment

njgheorghita commented Jul 10, 2019

carver left a comment

carver Jul 10, 2019

carver Jul 10, 2019

carver Jul 10, 2019

njgheorghita Jul 10, 2019

carver Jul 10, 2019

carver Jul 10, 2019


		.. 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.

Vendor ethPM #1379

Vendor ethPM #1379

Conversation

njgheorghita commented Jul 9, 2019 • edited Loading

What was wrong?

Cute Animal Picture

voith commented Jul 10, 2019

njgheorghita commented Jul 10, 2019 • edited Loading

kclowes left a comment

Choose a reason for hiding this comment

njgheorghita commented Jul 10, 2019

carver left a comment

Choose a reason for hiding this comment

carver Jul 10, 2019

Choose a reason for hiding this comment

carver Jul 10, 2019

Choose a reason for hiding this comment

carver Jul 10, 2019

Choose a reason for hiding this comment

njgheorghita Jul 10, 2019

Choose a reason for hiding this comment

carver Jul 10, 2019

Choose a reason for hiding this comment

carver Jul 10, 2019

Choose a reason for hiding this comment

njgheorghita commented Jul 9, 2019 •

edited

Loading

njgheorghita commented Jul 10, 2019 •

edited

Loading