Relocate personal RPC endpoints to parity and geth class #1211

njgheorghita · 2019-01-15T14:48:19Z

What was wrong?

Updated web3.personal namespace to web3.geth.personal and web3.parity.personal which each reflect the personal jsonrpc endpoints supported by the respective client. Made the same update to PersonalModuleTest -> GoEthereumPersonalModuleTest and ParityPersonalModuleTest for integration testing.

Cute Animal Picture

njgheorghita · 2019-01-15T15:09:04Z

@pipermerriam @carver Here's a first stab at relocating the personal_* RPC endpoints to geth and parity modules respectively. I'm not sure if this is the best way to achieve the goal (many of the tests are broken, but i'll go ahead with fixing those after settling on a pattern).

I'm not sure if I like the pattern in this PR, but I had trouble finding something I liked better since there are going to be shared endpoints b/w geth&parity as well as endpoints unique to either geth&parity. Also, what about other clients? If someone wanted to use the trinity client, it might be confusing to use a web3.parity namespace to access a personal_* rpc endpoint (though I'm unsure if this is relevant).

https://docs.google.com/spreadsheets/d/1rNvszb2hbYrzUbZj0v4oPnSwyJezYYHhgcpiqdVf7Bk/edit?usp=sharing
Here's a link to a spreadsheet I made of all the RPC endpoints I could find, and whether/not they are supported by the different clients & web3.py, in case you find this useful.

pipermerriam · 2019-01-15T22:15:22Z

Combined with the new way module methods are being done in #1166 this might be a little cleaner since we'd be able to re-use the method definitions for any commonly shaped endpoints.

dylanjw · 2019-01-16T02:29:42Z

web3/geth.py

+    https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal
+    """
+    def __init__(self, w3):
+        self.w3 = w3


This will override the inherited Module __init__, which could cause a bug in the future.

dylanjw · 2019-01-16T02:46:12Z

web3/geth.py

+class Geth(Module):
+    @property
+    def personal(self):
+        return GethPersonal(self.web3)


Im not sure if it is better, but submodules could be configured in Web3 along with the modules.

Something to the effect of this in web3.Web3:

def get_default_modules(): return [ {"name": "eth", "module": Eth}, {"name": "net", "module": Net}, {"name": "geth", "module": Geth, "submodules": {"personal": Personal}} {"name": "personal", "module": Personal}, {"name": "version", "module": Version}, {"name": "txpool", "module": TxPool}, {"name": "miner", "module": Miner}, {"name": "admin", "module": Admin}, {"name": "parity", "module": Parity}, {"name": "testing", "module": Testing}, ] class Web3: [...] def __init__(self, provider=None, middlewares=None, modules=None, ens=empty): self.manager = self.RequestManager(self, provider, middlewares) if modules is None: modules = get_default_modules() for module in modules: module['module'].attach(self, module.get('name')) if 'submodules' in module: for subname, submodule in module['submodules'].items(): submodule.attach(getattr(self, module['name']), submodule) self.ens = ens

The Module.attach method appears to work with modules 2 levels deep. Im leaning toward having 1 way modules are attached.

dylanjw · 2019-01-16T02:54:31Z

With #1166 way of defining methods, the personal module methods could all be written in one place and the partiy/geth personal modules could compose the methods they support. Then the not implemented methods could just be absent.

njgheorghita · 2019-02-28T23:09:07Z

web3/_utils/module_testing/personal_module.py

+            unlockable_account_dual_type,
+            'bad-password',
+            None
+        )


I cannot figure this out for the life of me why parity insists on returning True if there is a failure to the personal_unlockAccount call.

https://wiki.parity.io/JSONRPC-personal-module#personal_unlockaccount

You could a comment to the code explaining that this is the actual expected output at the moment, since its pretty weird. Maybe open an issue with Parity?

njgheorghita · 2019-02-28T23:10:51Z

also pinging @dylanjw if you don't mind taking another look over this

dylanjw

Looks great! Just a couple small changes.

dylanjw · 2019-03-01T12:09:30Z

web3/_utils/module_testing/personal_module.py

+            unlockable_account_dual_type,
+            'bad-password',
+            None
+        )


You could a comment to the code explaining that this is the actual expected output at the moment, since its pretty weird. Maybe open an issue with Parity?

dylanjw · 2019-03-01T12:41:11Z

web3/personal.py

+    return Method(
+        "personal_importRawKey",
+        mungers=[default_root_munger],
+        formatter_lookup_fn=None,


Nice!

Seeing the new Method class used, I think I need to make Method's arguments more intuitive. I prefer omitting formatter_lookup_fn here rather than setting it to None. Setting it to None looks like it would not have formatters applied, but it actually will have the default formatters applied. I will make changes to Method to either remove formatter_lookup_fn from the arguments, or have it override the default formatters when set to None or a custom lookup function.

Yeah, I would expect:

"formatter_lookup_fn kwarg missing": default formatting

"formatter_lookup_fn=None": no formatting

"formatter_lookup_fn=my_formatter": only my_formatter runs

carver · 2019-03-01T21:49:28Z

tests/integration/test_ethereum_tester.py

        ValueError,
    )
+
+    # Test overridden here since eth-tester returns False rather than None for failed unlock


I'm in favor of changing eth-tester's behavior to match geth

carver · 2019-03-01T21:52:23Z

docs/web3.personal.rst

@@ -78,9 +142,9 @@ The following methods are available on the ``web3.personal`` namespace.

    .. code-block:: python

-        >>> web3.personal.unlockAccount('0xd3cda913deb6f67967b99d67acdfa1712c293601', 'wrong-passphrase')
+        >>> web3.parity.personal.unlockAccount('0xd3cda913deb6f67967b99d67acdfa1712c293601', 'wrong-passphrase')


Looks like this example result needs to switch to True and almost certainly an explanation that that is not a typo in the docs.

carver · 2019-03-01T22:02:33Z

web3/main.py

+        {"name": "txpool", "module": TxPool},
+        {"name": "miner", "module": Miner},
+        {"name": "admin", "module": Admin},
+        {"name": "parity", "module": Parity, 'submodules': {'personal': ParityPersonal}},


You may like this format better:

return { ... "admin": Admin, "parity": (Parity, {"personal": ParityPersonal}), ... }

One nice thing about this is that the submodule format is the same as the supermodule format, which makes for easier extension to multiple layers if desired. It also means less special casing when stitching the modules togother. Plus I think it's a little nicer to read.

Here's an example with 3 layers:

return { ... "admin": Admin, "parity": (Parity, { "personal": ParityPersonal, "test": (TestModule, { "eth": ParityEthTest, "bzz": ParitySwarmTest, ... }), }), ... }

carver · 2019-03-01T22:04:28Z

web3/personal.py

+    return Method(
+        "personal_importRawKey",
+        mungers=[default_root_munger],
+        formatter_lookup_fn=None,


Yeah, I would expect:

"formatter_lookup_fn kwarg missing": default formatting

"formatter_lookup_fn=None": no formatting

"formatter_lookup_fn=my_formatter": only my_formatter runs

njgheorghita · 2019-03-11T15:43:28Z

web3/main.py

+            module_class.attach(self, module)
+            if submodules:
+                for subname, subclasses in submodules[0].items():
+                    subclasses[0].attach(getattr(self, module), subname)



@carver Thoughts on this setup? Maybe there's a cleaner way to achieve the attaching of subclasses?

carver · 2019-03-11T19:28:22Z

web3/main.py

+            module_class, *submodules = modules[module]
+            module_class.attach(self, module)
+            if submodules:
+                for subname, subclasses in submodules[0].items():


Yeah, this submodules[0] looks surprising, I think. Also, if we're going to go with a recursive definition, might as well implement with a recursive solution, so it can go arbitrarily (well stack-limit) deep.

Something like a new utility method:

def attach_modules(parent_module, module_definitions) for module_name, module_info in module_definitions.items(): module_class = module_info[0] module_class.attach(parent_module, module_name) if len(module_info) == 2: submodule_definitions = module_info[1] module = getattr(parent_module, module_name) attach_modules(module, submodule_definitions) elif len(module_info) != 1: raise ValidationError("Module definition can only have 1 or 2 elements")

Then the web3 init just uses it, like:

attach_modules(self, modules)

njgheorghita force-pushed the relocate-personal-module branch 2 times, most recently from 7053617 to 5c3fc71 Compare January 15, 2019 14:52

dylanjw reviewed Jan 16, 2019

View reviewed changes

njgheorghita mentioned this pull request Jan 17, 2019

Planned v5 changes #722

Closed

22 tasks

njgheorghita force-pushed the relocate-personal-module branch 23 times, most recently from b5f468e to 02977cc Compare February 28, 2019 21:18

njgheorghita force-pushed the relocate-personal-module branch 10 times, most recently from 92534c6 to 282ba94 Compare February 28, 2019 23:07

njgheorghita commented Feb 28, 2019

View reviewed changes

njgheorghita changed the title ~~WIP - Relocate personal RPC endpoints to parity and geth class~~ Relocate personal RPC endpoints to parity and geth class Feb 28, 2019

njgheorghita requested review from pipermerriam and kclowes February 28, 2019 23:10

njgheorghita force-pushed the relocate-personal-module branch 2 times, most recently from dbec384 to 7ca37c2 Compare February 28, 2019 23:41

njgheorghita requested a review from carver March 1, 2019 05:25

dylanjw approved these changes Mar 1, 2019

View reviewed changes

carver approved these changes Mar 1, 2019

View reviewed changes

njgheorghita force-pushed the relocate-personal-module branch from 3f614e7 to a770663 Compare March 11, 2019 09:54

njgheorghita added 2 commits March 11, 2019 10:58

Relocate personal RPC endpoints to parity and geth class

89bb51a

Update geth.personal to compose personal endpoints

8253f5e

njgheorghita force-pushed the relocate-personal-module branch from a770663 to e912bec Compare March 11, 2019 10:09

njgheorghita commented Mar 11, 2019

View reviewed changes

carver reviewed Mar 11, 2019

View reviewed changes

Update get_default_modules structure to cleaner format

d78e187

njgheorghita force-pushed the relocate-personal-module branch from e912bec to d78e187 Compare March 12, 2019 12:45

njgheorghita merged commit 0cb2dc0 into ethereum:master Mar 12, 2019

njgheorghita deleted the relocate-personal-module branch March 12, 2019 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relocate personal RPC endpoints to parity and geth class #1211

Relocate personal RPC endpoints to parity and geth class #1211

njgheorghita commented Jan 15, 2019 •

edited

Loading

njgheorghita commented Jan 15, 2019

pipermerriam commented Jan 15, 2019

dylanjw Jan 16, 2019

dylanjw Jan 16, 2019

dylanjw commented Jan 16, 2019

njgheorghita Feb 28, 2019 •

edited

Loading

dylanjw Mar 1, 2019

njgheorghita commented Feb 28, 2019

dylanjw left a comment

dylanjw Mar 1, 2019

dylanjw Mar 1, 2019

carver Mar 1, 2019

carver Mar 1, 2019

carver Mar 1, 2019

carver Mar 1, 2019

carver Mar 1, 2019

njgheorghita Mar 11, 2019

carver Mar 11, 2019 •

edited

Loading

Relocate personal RPC endpoints to parity and geth class #1211

Relocate personal RPC endpoints to parity and geth class #1211

Conversation

njgheorghita commented Jan 15, 2019 • edited Loading

What was wrong?

Cute Animal Picture

njgheorghita commented Jan 15, 2019

pipermerriam commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjw commented Jan 16, 2019

njgheorghita Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita commented Feb 28, 2019

dylanjw 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

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

carver Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

njgheorghita commented Jan 15, 2019 •

edited

Loading

njgheorghita Feb 28, 2019 •

edited

Loading

carver Mar 11, 2019 •

edited

Loading