Stabilize PM module #1125

njgheorghita · 2018-10-25T14:51:18Z

What was wrong?

Stabillize pm module.

How was it fixed?

web3.pm.PM - Main class for interacting with on-chain registries.
web3.pm.ERCRegistry - Base class for subclasses to inherit from that target specific implementations of an ERC-compliant registry.
web3.pm.VyperReferenceRegistry - Class to interact with deployments of the Vyper Reference Registry implementation
web3.pm.SolidityReferenceRegistry - Class to interact with deployments of the Solidity Reference Registry implementation.

Cute animal picture

tests/core/pm-module/test_pm_init.py

tests/core/pm-module/test_registry_integration.py

njgheorghita · 2018-10-27T20:04:15Z

@carver @pipermerriam ping for final review - sorry about the massive pr, though the vast majority of this pr has already been approved in prior pr chunks.

@carver, i'm not sure the best way to resolve core-eth_abi2 failing builds failing in circleci.
it seems as though force reinstalling eth-abi-v2 in that tox env is causing problems with the pytest-ethereum plugin (which depends on web3 with eth-abi>=1.2,<2 installed) any thoughts on how to resolve this? the error resolves when I remove all web3 dependencies from pytest-ethereum but of course then, pytest-ethereum is fairly useless and breaks.

carver · 2018-10-28T05:51:46Z

tox env is causing problems with the pytest-ethereum plugin (which depends on web3 with eth-abi>=1.2,<2 installed) any thoughts on how to resolve this?

pytest-ethereum should probably add support for eth-abi v2-alpha.

How much is pytest-ethereum being used? If it's only a couple places, maybe it's fastest to just drop it and locally replace any missing utilities until it has support for eth-abi v2-alpha.

docs/web3.pm.rst

tests/core/pm-module/conftest.py

tests/core/pm-module/test_ens_integration.py

njgheorghita · 2018-11-28T11:18:49Z

@carver It seems that I'm blocked on updating pytest-ethereum and ethpm to work with eth-abiV2 until a web3 version is released which is dependent on eth-abiV2 (currently web3's eth-abi dependency is "eth-abi>=1.2.0,<2.0.0", - Any idea of when I can expect a web3 release with an eth-abiV2 dependency?

carver · 2018-11-28T22:14:00Z

Any idea of when I can expect a web3 release with an eth-abiV2 dependency?

It first glance, I think that change is backwards compatible (a project that depends on web3 v4 might also depend on eth-abi v1). How about a v5-alpha.0 release?

njgheorghita · 2018-11-29T17:14:51Z

How about a v5-alpha.0 release?

@carver That'd be great! It's no huge rush though, whenever it's ready is fine

njgheorghita · 2019-01-17T15:15:38Z

@carver re: comment above, I'm torn between the two

adding PM as a default module on web3 - making it easier for the user to use, but being restricted to no backwards incompatible changes until web3 v6
leaving PM off web3 by default - forcing the user to attach PM to their web3 instance, but having more flexibility with the api while still in web3 v5

But, I'm leaning towards the latter option for now as it seems possible the API might change in the near future (especially as collaboration with the other pm teams is picking up). And the attach line is more just annoying than an actual impediment to use.

pipermerriam · 2019-01-17T17:37:48Z

As discussed in this morning's stand-up, I'd advocate for requiring explicitly enabling this API for now before we commit to stability.

w3 = Web3(...)
w3.enable_unstable_package_management_api()  # handles the attaching

kclowes · 2019-01-18T16:30:14Z

web3/main.py

+                "`w3.enable_unstable_package_management_api()` and try again."
+            )
+
+    def enable_unstable_package_management_api(self):


Question from the peanut gallery... do you think it would be useful to have a disable function too? Or I guess you can just ignore features and/or detach if someone decides they actually don't want to use the features?

I'm not inclined to implement a disable API as they could just re-instantiate their Web3 instance.

pipermerriam

The code in tests/core/pm-module/conftest.py looks potentially useful and might be a nice API to have available via web3.tools.pm as a public API (eventually) to allow people to easily setup a registry on a test chain.

pipermerriam · 2019-01-18T17:28:16Z

docs/web3.pm.rst

+.. autoclass:: web3.pm.PM
+   :members:
+
+.. note:: If you want to implement your own registry and use it with ``web3.pm``, you must create a subclass that inherits from ``ERCRegistry``, implements all the methods defined in ``ERCRegistry``, and manually set it as the ```registry`` attribute on ``web3.pm``.


This seems like something that would be worth putting in it's own section and 1) explaining why someone would want to do this and 2) how they would do it with a code example.

If the code example isn't something you want to maintain or you don't see it as very useful then maybe just state that replacing the registry is beyond the scope of this guide.

pipermerriam · 2019-01-18T17:29:43Z

tests/core/pm-module/conftest.py

+    VyperReferenceRegistry,
+)
+
+VY_PACKAGE_ID_1 = b'\xd0Y\xe8\xa6\xeaZ\x8b\xbf\x8d\xd0\x97\xa7\xb8\x92#\x16\xdc\xb7\xf0$\xe8"\x0bV\xd3\xc9\xe7\x18\x8ajv@'  # noqa: E501


It might be nice to represent these as their hex values like:

VY_PACKAGE_ID_1 = to_bytes(hexstr='0xd0...') # noqa: E501 ...

As those values are easier to visually parse.

pipermerriam · 2019-01-18T17:30:40Z

tests/core/pm-module/conftest.py

+SOL_RELEASE_ID_4 = b"p\x82\xe9T\xe4\xfdj\xdf\x8a%\xc6\xce\xfe!\x8f2\xecf\xd8\xa1\x97\x19\x7f\x1d\x05\xaag\xa6\\\xafQ\x11"  # noqa: E501
+
+
+logging.basicConfig(stream=sys.stdout, level=logging.INFO, format="%(message)s")


Instead of this you can just use pytest tests/test_things.py --log-cli-level=INFO to get logging messages to show up in your terminal. This approach will unconditionally dump them to stdout which probably isn't globally desired.

pipermerriam

Not required for this PR but this new package management API almost definitely deserves multiple guides in the documentation.

carver

Reminder to remove WIP from the title to signal that a merge is pending.

carver · 2019-01-18T20:38:30Z

setup.py

        "hexbytes>=0.1.0,<1.0.0",
        "lru-dict>=1.1.6,<2.0.0",
-        "eth-hash[pycryptodome]>=0.2.0,<1.0.0",
+        "pytest-ethereum>=0.1.3a6,<1",


Looks like this should be moved to the tester extra, instead of as a core dependency.

Hmmm, maybe I'm missing something, but we use pytest-ethereum's Deployer in the web3.pm module. But, I would say that this use case of using the Deployer is not that critical at all, since it's a relatively straightforward deployment. So, I can definitely pull it out, in which case pytest-ethereum would only be needed in the tests and help minimize the core dependencies.

Ah, I just did a Ctrl-f in the github diff, and the diff on pm was hidden.

At least semantically, I find it pretty surprising to use anything called pytest-* in the main deployed package. If Deployer is to be used in production code like this, then maybe it could be moved into a web3 _utils module.

carver · 2019-01-18T20:42:51Z

tests/core/pm-module/conftest.py

+    LinkableContract,
+)
+from pytest_ethereum import (
+    linker as l,


One-letter variable? Maybe don't rename the import at all.

Works for me, it was convention leftover from py-ethpm's builder tool to avoid from ethpm.builder import * but cut down on wordiness, but in this case isn't as useful.

carver · 2019-01-18T20:56:12Z

web3/pm.py

+    The ERCRegistry class is a base class for all registry implementations to inherit from. It
+    defines the methods specified in `ERC 1319 <https://github.com/ethereum/EIPs/issues/1319>`__.
+    All of these methods are prefixed with an underscore, since they are not intended to be
+    accessed directly, but rather through the methods on ``web3.pm``.


One important reason that we usually have for methods that are "not intended to be accessed directly" is that the API might change without a major version bump. Would be nice to add a note about whether these method signatures are subject to change. That's not super relevant during alpha, but we might as well document for stable now.

Disable pm api by default

pipermerriam reviewed Oct 25, 2018

View reviewed changes

tests/core/pm-module/test_pm_init.py Outdated Show resolved Hide resolved

pipermerriam reviewed Oct 25, 2018

View reviewed changes

tests/core/pm-module/test_registry_integration.py Outdated Show resolved Hide resolved

njgheorghita force-pushed the pm-api branch 6 times, most recently from 859dadd to 038c1a0 Compare October 26, 2018 09:58

njgheorghita mentioned this pull request Oct 26, 2018

Minor fixes to vyper registry ethpm/py-ethpm#121

Merged

njgheorghita force-pushed the pm-api branch 6 times, most recently from f6257eb to c8071fb Compare October 26, 2018 19:46

njgheorghita mentioned this pull request Oct 27, 2018

Update registry to allow 64 byte uris ethpm/py-ethpm#123

Merged

njgheorghita force-pushed the pm-api branch 5 times, most recently from 60973c7 to 9de5e00 Compare October 27, 2018 16:09

njgheorghita mentioned this pull request Nov 27, 2018

PM-API: Refactor pm-api #1141

Merged

pipermerriam approved these changes Nov 27, 2018

View reviewed changes

njgheorghita force-pushed the pm-api branch from f8f9d2e to a9de289 Compare November 28, 2018 11:07

njgheorghita force-pushed the pm-api branch from a9de289 to 71a904c Compare November 28, 2018 20:24

njgheorghita added 7 commits January 17, 2019 15:30

Write pm.tie_registry_to_ens

3d11990

pm-api branch cleanup

26020b9

pm-api branch cleanup

8986d30

Fix get_all_package_versions bug

bfec5b1

Update solidity registry integration test

f7e82f9

Refactor web3.pm module and add docs

a42b542

Minor adjustments from pr comments

921c70e

njgheorghita force-pushed the pm-api branch 2 times, most recently from 15098ec to 1be055d Compare January 17, 2019 14:48

Minor adjustments from updated dependencies

02da228

njgheorghita force-pushed the pm-api branch from 1be055d to 02da228 Compare January 17, 2019 14:54

Disable pm api by default

8416c8a

kclowes reviewed Jan 18, 2019

View reviewed changes

pipermerriam approved these changes Jan 18, 2019

View reviewed changes

pipermerriam reviewed Jan 18, 2019

View reviewed changes

njgheorghita force-pushed the pm-api branch 4 times, most recently from f22668a to c184699 Compare January 18, 2019 19:59

carver approved these changes Jan 18, 2019

View reviewed changes

njgheorghita changed the title ~~WIP - Stabilize PM module~~ Stabilize PM module Jan 18, 2019

njgheorghita force-pushed the pm-api branch 3 times, most recently from 72d154a to 4f5c790 Compare January 22, 2019 09:27

Merge pull request #1216 from njgheorghita/pm-api-unstable-flag

5fa1ffc

Disable pm api by default

njgheorghita force-pushed the pm-api branch from 4f5c790 to 5fa1ffc Compare January 22, 2019 09:31

njgheorghita merged commit 90b2527 into master Jan 22, 2019

njgheorghita deleted the pm-api branch January 22, 2019 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize PM module #1125

Stabilize PM module #1125

njgheorghita commented Oct 25, 2018 •

edited

Loading

njgheorghita commented Oct 27, 2018

carver commented Oct 28, 2018

njgheorghita commented Nov 28, 2018

carver commented Nov 28, 2018

njgheorghita commented Nov 29, 2018

njgheorghita commented Jan 17, 2019 •

edited

Loading

pipermerriam commented Jan 17, 2019

kclowes Jan 18, 2019

pipermerriam Jan 18, 2019

pipermerriam left a comment

pipermerriam Jan 18, 2019

pipermerriam Jan 18, 2019

pipermerriam Jan 18, 2019

pipermerriam left a comment

carver left a comment

carver Jan 18, 2019

njgheorghita Jan 21, 2019 •

edited

Loading

carver Jan 21, 2019

carver Jan 18, 2019

njgheorghita Jan 21, 2019

carver Jan 18, 2019 •

edited

Loading

		SOL_RELEASE_ID_4 = b"p\x82\xe9T\xe4\xfdj\xdf\x8a%\xc6\xce\xfe!\x8f2\xecf\xd8\xa1\x97\x19\x7f\x1d\x05\xaag\xa6\\\xafQ\x11" # noqa: E501


		logging.basicConfig(stream=sys.stdout, level=logging.INFO, format="%(message)s")

Stabilize PM module #1125

Stabilize PM module #1125

Conversation

njgheorghita commented Oct 25, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute animal picture

njgheorghita commented Oct 27, 2018

carver commented Oct 28, 2018

njgheorghita commented Nov 28, 2018

carver commented Nov 28, 2018

njgheorghita commented Nov 29, 2018

njgheorghita commented Jan 17, 2019 • edited Loading

pipermerriam commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam left a 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

pipermerriam left a comment

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita Jan 21, 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

carver Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

njgheorghita commented Oct 25, 2018 •

edited

Loading

njgheorghita commented Jan 17, 2019 •

edited

Loading

njgheorghita Jan 21, 2019 •

edited

Loading

carver Jan 18, 2019 •

edited

Loading