[WIP] pm-api: Write Registry class #1098

njgheorghita · 2018-10-10T17:21:29Z

What was wrong?

web3.pm needs a way to interact with an on-chain registry (i.e. release, fetch, transfer_owner)

Cute Animal Picture

njgheorghita · 2018-10-10T17:25:43Z

@pipermerriam still fairly dirty/wip, but i'm looking for some thoughts on the general direction of this api. missing features, etc..

also the Registry class is coupled very tightly to the registry implementation I wrote in vyper. It's close, but not 100% inline with EIP 1319 due to some of the limitations of vyper. I'm not sure the best way to move forward? It seems unfair to require users to use the vyper contract I wrote, but maybe move ahead with this for now, and then focus on updating the standard to support both solidity/vyper abis? Then update Registry accordingly

pipermerriam · 2018-10-10T21:39:45Z

web3/pm.py

    def get_package_from_manifest(self, manifest):
        pkg = Package(manifest, self.web3)
        return pkg

    def get_package_from_uri(self, uri):
        pkg = Package.from_uri(uri, self.web3)
        return pkg
+
+    def release_package(self, name, version, uri):


might be more intuitive if we used manifest_uri in these methods.

pipermerriam · 2018-10-10T21:43:08Z

web3/pm.py

+
+    def set_registry(self, address=None):
+        if not address:
+            self.registry = Registry.deploy_new_instance(self.web3)


This is not intuitive behavior imho. Maybe this should be two different APIs, one that sets it, set_registry and one that creates a new one deploy_and_set_registry. Good user guides/examples and documentation are probably the most important part for this since we will have to teach people about the workflows for managing their registry.

pipermerriam · 2018-10-10T21:43:52Z

web3/pm.py

+class Registry(Contract):
+    def __init__(self, address, w3):
+        # only works with v.py registry in ethpm/assets
+        # abi verification for compatibility?


How about checking the runtime_bytecode. Should be a known value right?

Yup, but wouldn't that require anybody who wants to use this class to use the registry.v.py implementation I wrote (which is still only 90% erc1319 compliant)? Not sure if that's ideal. Ideally, I'd like to couple this class to the ERC1319 standard rather than a particular implementation in a particular language.

pipermerriam · 2018-10-10T21:45:19Z

web3/pm.py

+
+    def release(self, name, version, uri):
+        tx_receipt = self.registry.functions.release(name, version, uri).transact()
+        self.w3.eth.waitForTransactionReceipt(tx_receipt)


probably good to return either the receipt or the transaction hash.

pipermerriam · 2018-10-10T21:47:55Z

web3/pm.py

+        for index in range(package_count):
+            package_id = self.registry.functions.get_package_id(index).call()
+            package_data = self.registry.functions.get_package_data_by_id(package_id).call()
+            yield (package_data[0].rstrip(b'\x00'), package_data[1])


I think it might be nice for future maintainers if you decomposed package_data into it's two parts. I am pretty sure package_data[0] is the package name. It isn't clear to me what package_data[1] is.

Also, commenting on the reason for the rstrip call would be good.

pipermerriam · 2018-10-10T21:48:31Z

web3/pm.py

+
+
+    @to_tuple
+    def get_all_package_versions(self, name): # -> (version, uri):


Instead of the commented out type strings, consider adding a docstring which explains what this returns.

pipermerriam · 2018-10-10T21:49:13Z

web3/pm.py

+
+    def transfer_owner(self, new_address):
+        tx_receipt = self.registry.functions.transfer_owner(new_address).transact()
+        self.w3.eth.waitForTransactionReceipt(tx_receipt)


Should probably return the txn hash or receipt.

pipermerriam · 2018-10-10T21:50:02Z

web3/pm.py

+        for index in range(release_count):
+            release_id = self.registry.functions.get_release_id_by_package_and_count(name, (index + 1)).call()
+            release_data = self.registry.functions.get_release_data_by_id(release_id).call()
+            yield (release_data[1].rstrip(b'\x00'), release_data[2].rstrip(b'\x00'))


Same here with respect to readability/future-maintenance. I'd suggest decomposing release_data into its parts and leaving some comments which signal why some of the normalization via rstrip is being performed.

njgheorghita · 2018-10-14T00:33:50Z

@pipermerriam could use another 👀 on this - added web3.pm.tie_registry_to_ens
Also shoutout @carver - the ens fixture in the ens test suite was a huge help. If you don't mind 👀 the ens stuff in this pr, just in case i'm missing something/doing something wrong, I'd appreciate it. Thanks!

pipermerriam · 2018-10-17T20:58:22Z

slipped through cracks... looking at this now

pipermerriam · 2018-10-17T21:00:51Z

tests/core/pm-module/test_ens_integration.py

+
+@pytest.fixture
+def ens_setup(solc_deployer):
+    ENS_MANIFEST = ASSETS_DIR / 'ens' / '1.0.1.json'


I started to pester you to move this up to module level but realized why it's here (conditional import failures). Worth a comment anytime we do something abnormal like this.

pipermerriam · 2018-10-17T21:04:24Z

tests/core/pm-module/test_ens_integration.py

+        ens_contract.address,
+        transaction={"from": ens_key}
+    )
+    public_resolver = public_resolver_package.deployments.get_instance("PublicResolver")


These seem like it would be beneficial to split them up into more granular fixtures, but I don't see that as required (just that it might make this easier to read/understand if it splits up well into smaller pieces.

pipermerriam · 2018-10-17T21:09:54Z

tests/core/pm-module/test_registry_integration.py

+@pytest.mark.skipif(ethpm_installed is False, reason="ethpm is not installed")
+def test_pm_deploy_and_set_registry(w3):
+    w3.pm.deploy_and_set_registry()
+    assert isinstance(w3.pm.registry, Registry)


should probably have a pre-check before the deployment to sanity check it wasn't somehow already present.

pipermerriam · 2018-10-17T21:10:04Z

tests/core/pm-module/test_registry_integration.py

+@pytest.mark.skipif(ethpm_installed is False, reason="ethpm is not installed")
+def test_pm_set_registry(vy_registry, w3):
+    w3.pm.set_registry(address=to_canonical_address(vy_registry.address))
+    assert isinstance(w3.pm.registry, Registry)


should probably have a pre-check before the deployment to sanity check it wasn't somehow already present.

pipermerriam · 2018-10-17T21:12:25Z

web3/pm.py

+        """
+        if is_canonical_address(address):
+            self.registry = Registry(address, self.web3)
+        elif isinstance(address, str):


Why isinstance(v, str)? Seems like we should require either a bytes address or a checksum'd address here to be consistent with the rest of the library.

You could use web3._utils.ens.is_ens_name() here.

pipermerriam · 2018-10-17T21:14:53Z

web3/pm.py

+        Returns (package_name, release_count) for every package on registry.
+        """
+        package_count = self.registry.functions.packageCount().call()
+        for index in range(0, package_count, 4):


using the term offset here instead of index will be more inline with the vyper implementation.

pipermerriam · 2018-10-17T21:17:54Z

web3/pm.py

+                    pkg_name = self.registry.functions.getPackageName(pkg_id).call()
+                    _, _, release_count = self.registry.functions.getPackageData(pkg_name).call()
+                    # rstrip used to trim trailing bytes returned in package_name: bytes32
+                    yield (pkg_name.rstrip(b'\x00'), release_count)


I was surprised that this returns a tuple containing the release count instead of just the names (or objects).

Maybe get_all_package_data?

Alternatively, get_all_package_names is probably fine and you could have a get_release_count function to lookup the release counts separately. Not sure it's good to bundle this functionality.

carver · 2018-10-17T21:53:00Z

web3/pm.py

+        """
+        if is_canonical_address(address):
+            self.registry = Registry(address, self.web3)
+        elif isinstance(address, str):


You could use web3._utils.ens.is_ens_name() here.

carver · 2018-10-17T21:55:06Z

web3/pm.py

+        if is_canonical_address(address):
+            self.registry = Registry(address, self.web3)
+        elif isinstance(address, str):
+            ns = ENS.fromWeb3(self.web3, ens_address)


Instead of making a new ens instance every time, you could use use w3.ens. Careful that it might be None in which case there is no ENS to attempt a lookup.

Take a look at how the abi_ens_resolver does it.

carver · 2018-10-17T21:55:54Z

web3/pm.py

+            self.registry = Registry(address, self.web3)
+        elif isinstance(address, str):
+            ns = ENS.fromWeb3(self.web3, ens_address)
+            addr_lookup = ns.address(address)


Let's use guess_tld=False which should probably be the default at the next major release. #722

carver · 2018-10-17T21:57:30Z

web3/pm.py

+            if addr_lookup:
+                self.registry = Registry(addr_lookup, self.web3)
+            else:
+                raise PMError("No address found after ENS lookup for name: {0}.".format(address))


More like a NameNotFound I think. eg~

web3.py/web3/_utils/ens.py

Lines 28 to 33 in aa86e01

def validate_name_has_address(ens, name):

addr = ens.address(name, guess_tld=False)

if addr:

return addr

else:

raise NameNotFound("Could not find address for name %r" % name)

Although don't use that "return or raise" style, which is from old code that wouldn't pass review today. X)

carver · 2018-10-17T22:02:10Z

web3/pm.py

+        ENS registry on whatever chain being used.
+        """
+        if is_canonical_address(address):
+            self.registry = Registry(address, self.web3)


I'd suggest either:

if is_canonical_address(address) or is_checksum_address(address): self.registry = Registry(address, self.web3) # and update Registry to work with checksum addresses

if is_canonical_address(address): self.registry = Registry(address, self.web3) # and update Registry to work with checksum addresses elif is_checksum_address(address): self.registry = Registry(to_bytes(hexstr=address), self.web3)

It's reasonable for the Registry library to be public-facing (used directly by devs) so I'd recommend option 1 for their convenience.

carver · 2018-10-17T22:02:41Z

web3/pm.py

+        owner of the domain name for this method to work.
+        """
+        self._validate_set_registry()
+        ns = ENS.fromWeb3(self.web3, ens_address)


Use w3.ens again

carver · 2018-10-17T22:07:08Z

web3/pm.py

+        onchain registry.
+
+        :address: Address of the on-chain registry - Accepts ENS name
+        :ens_address: If using an ens, but not on the mainnet, this is the address of the central


I think we should just have people set up their w3.ens to be the one they want to use. Then it doesn't require these ens configuration parameters on every function.

carver · 2018-10-17T22:08:11Z

web3/pm.py

+        """
+        self._validate_set_registry()
+        ns = ENS.fromWeb3(self.web3, ens_address)
+        ns.setup_address(ens_name, self.registry.address)


This seems like a simple enough operation that the whole method doesn't need to exist. It's a one-liner: w3.ens.setup_address(ens_name, w3.pm.registry.address)

njgheorghita · 2018-10-18T18:05:02Z

Thanks for the review @carver & @pipermerriam - gearing up to stabilize py-ethpm soon & pr to merge pm-api into master sometime (hopefully early) next week.

Write Registry class

29c4866

pipermerriam approved these changes Oct 10, 2018

View reviewed changes

Update Registry class to use new erc1319 contract

23ef954

njgheorghita force-pushed the pm-api-registry branch 12 times, most recently from c6717c1 to cdfa433 Compare October 14, 2018 00:23

njgheorghita force-pushed the pm-api-registry branch 5 times, most recently from 5e81210 to a68d9b8 Compare October 16, 2018 00:07

pipermerriam approved these changes Oct 17, 2018

View reviewed changes

carver reviewed Oct 17, 2018

View reviewed changes

njgheorghita force-pushed the pm-api-registry branch from a68d9b8 to 61870a2 Compare October 18, 2018 17:26

Write pm.tie_registry_to_ens

55131a5

njgheorghita force-pushed the pm-api-registry branch from 61870a2 to 55131a5 Compare October 18, 2018 17:31

njgheorghita merged commit 28741bf into ethereum:pm-api Oct 18, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] pm-api: Write Registry class #1098

[WIP] pm-api: Write Registry class #1098

njgheorghita commented Oct 10, 2018 •

edited

Loading

njgheorghita commented Oct 10, 2018 •

edited

Loading

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

njgheorghita Oct 14, 2018 •

edited

Loading

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

njgheorghita commented Oct 14, 2018

pipermerriam commented Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

carver Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

carver Oct 17, 2018

njgheorghita commented Oct 18, 2018



		@to_tuple
		def get_all_package_versions(self, name): # -> (version, uri):

	def validate_name_has_address(ens, name):
	addr = ens.address(name, guess_tld=False)
	if addr:
	return addr
	else:
	raise NameNotFound("Could not find address for name %r" % name)

[WIP] pm-api: Write Registry class #1098

[WIP] pm-api: Write Registry class #1098

Conversation

njgheorghita commented Oct 10, 2018 • edited Loading

What was wrong?

Cute Animal Picture

njgheorghita commented Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita Oct 14, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita commented Oct 14, 2018

pipermerriam commented Oct 17, 2018

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

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

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

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

njgheorghita commented Oct 18, 2018

njgheorghita commented Oct 10, 2018 •

edited

Loading

njgheorghita commented Oct 10, 2018 •

edited

Loading

njgheorghita Oct 14, 2018 •

edited

Loading