Update shh module & relocate to parity and geth namespaces #1308

njgheorghita · 2019-03-29T16:51:57Z

What was wrong?

Updated Shh module to V2
Exposed supported Whisper endpoints in GethShh and ParityShh
Hooked up integration tests for supported endpoints for geth and parity

Cute Animal Picture

njgheorghita · 2019-03-29T18:25:09Z

web3/shh.py

-        return self.web3.manager.request_blocking("shh_hasKeyPair", [id])
+def post_munger(module, message):
+    if message and ("payload" in message):
+        return (message,)


@dylanjw I'm not sure if this is a misunderstanding on my end or a bug with the munger, but this was the only way I could successfully use this munger. From what I understand this should return something like return module, [message] - but that wouldn't work.

mungers only need to return the arguments, and not the module object. This was changed, and it looks like I didnt update the docstring in the Method class.

So what you have here is correct.

docs/web3.shh.rst

dylanjw · 2019-03-30T19:09:24Z

web3/parity.py

+    """
+    https://wiki.parity.io/JSONRPC-shh-module
+    """
+    info = info()


It might be clearer if these methods were just assigned to variable names, rather than the output of these functions. info = Method(...) and then class attribute assignment could lose the parens. info = info.

dylanjw · 2019-03-30T19:12:16Z

web3/shh.py

-)
-from web3.module import (
-    Module,
+from web3.method import (


It might be good to move this whole file to utils now, since nothing in here works without being assigned to a module. People might think that web3.ssh.* is part of the api.

If we're going to do it that way, the mungers property on the Method class needs to be converted to a tuple as well as auditing it for any other mutable properties to ensure that modifications to a Method object don't have unintended side effects.

as well as auditing it for any other mutable properties to ensure that modifications to a Method object don't have unintended side effects.

Can you explain this a little further - what are some potential side effects? what are the other mutable properties we should check?

dylanjw · 2019-03-30T19:34:23Z

web3/shh.py

-        return self.web3.manager.request_blocking("shh_hasKeyPair", [id])
+def post_munger(module, message):
+    if message and ("payload" in message):
+        return (message,)


mungers only need to return the arguments, and not the module object. This was changed, and it looks like I didnt update the docstring in the Method class.

So what you have here is correct.

tests/integration/parity/common.py

pipermerriam · 2019-04-01T15:33:49Z

web3/_utils/filters.py

@@ -239,7 +239,8 @@ def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

    def get_new_entries(self):
-        log_entries = self._filter_valid_entries(self.web3.shh.getMessages(self.filter_id))
+        all_messages = self.web3.parity.shh.getFilterMessages(self.filter_id)


This seems wrong that this is accessing the primary w3.parity module.

web3/_utils/module_testing/shh_module.py

pipermerriam · 2019-04-01T16:33:51Z

web3/_utils/module_testing/shh_module.py

+        topic = '0x13370000'
+        payloads = [web3.toHex(text="test message :)"), web3.toHex(text="2nd test message")]
+
+        shh_filter_id = web3.parity.shh.newMessageFilter({


There seems to be a lot of similarity between these two implementations (geth and parity) of the tests. Thoughts on trying to collapse these into something a little more DRY?

I'd like to, but imo they aren't as similar as they may appear. Besides the different shh modules being tested against, the way parity implements post, newMessageFilter, info and version all differ in the params accepted by the jsonrpc endpoint, and the type of data returned. The only tests that are the same are test_shh_asymmetric_key_pair and test_shh_symmetric_key_pair. Beyond that it seems to me like sharing tests might require a fair amount of config overhead that might end up more confusing

web3/geth.py

pipermerriam · 2019-04-01T16:37:16Z

web3/shh.py

-)
-from web3.module import (
-    Module,
+from web3.method import (


If we're going to do it that way, the mungers property on the Method class needs to be converted to a tuple as well as auditing it for any other mutable properties to ensure that modifications to a Method object don't have unintended side effects.

njgheorghita · 2019-04-04T12:58:35Z

@pipermerriam @kclowes ping for final review.

Also, geth v1.7.2 integration tests just broke all of a sudden

ci.go:180: You have Go version go1.12.1
ci.go:181: go-ethereum requires at least Go version 1.7 and cannot
ci.go:182: be compiled with an earlier version. Please upgrade your Go installation.

Makes no sense to me why this happened, especially since geth v1.7.2 tests have been compiling happily with go1.12 ever since I updated the go version in #1301

Also raises the question, why are we running integration tests against geth 1.7.2 and 1.8.22? Is it fine to just drop the 1.7.2 tests as they're becoming more and more outdated?

kclowes · 2019-04-04T18:43:37Z

I am not sure why this just broke, but it's due to this line in 1.7. Apparently go1.12 is less than go1.7. I can't find how long they will support geth 1.7 (or if they even still are supporting 1.7?), but the last 1.7 release was Nov. 2017. So I guess I see 3 options:

Downgrade the go version to 1.9 (which is unsupported) here
Backport the fix that's in geth 1.8 to 1.7 (here)
We drop integration tests for 1.7.
Like I said, I don't have a good feel for how long Geth 1.7 will be supported or how many people are still using geth 1.7, so I'm not sure what the right answer is, but I think I would naively lean toward option 1 and/or 2. I could also be persuaded otherwise.

kclowes

🚀 Looks good to me @njgheorghita! Just left one typo comment.

kclowes · 2019-04-04T18:44:30Z

docs/web3.geth.rst

+
+.. py:method:: Shh.deleteKeyPair(self, id)
+
+    * Deletes the specifies key if it exists.


njgheorghita · 2019-04-05T14:22:02Z

@kclowes

Apparently go1.12 is less than go1.7.

where did you see that? I'm wholly unfamiliar with go haha - but that seems weird to me, also when it seems 1.12 is the latest release

pipermerriam · 2019-04-05T14:26:11Z

What is the oldest version of go-ethereum that supports the mainnet?

njgheorghita · 2019-04-05T14:35:38Z

@pipermerriam Not sure if I understand your question - but from what I can tell it's v1.0.0

This is the first major and first live-net version of the go-ethereum client geth (1.0.0) and marks a major milestone in the history of Ethereum and the Go Ethereum codebase.

kclowes · 2019-04-05T15:44:41Z

@njgheorghita I learned yesterday that you can compare strings in go. So in geth v1.7 they were comparing runtime.Version() < 'go1.7', where in our case runtime.Version() == go1.12. But because it's not comparing numbers, I think what's happening is that it compares the first 5 characters and returns true because go1.1 is less than go1.7. It looks like the geth team changed that line of code in version 1.8 though. So 1.12 is definitely the latest go release, there is just a bug in how geth1.7 checks the go version.

pipermerriam · 2019-04-05T16:03:35Z

@kclowes you can do this type of string comparison (and make the same mistake) in python as well (I've done it)

@njgheorghita I was intending to mean the current mainnet. Basically, I'm wondering how useful it is for us to support any of the 1.7x line of go-ethereum

pipermerriam · 2019-04-05T16:04:05Z

@njgheorghita ignore me, I now see this is the version of go, not goethereum

kclowes · 2019-04-05T16:13:16Z

@pipermerriam Interesting! I can see where it would be a useful feature.

I think our problem has just fixed itself. So ignore all of the above. Now I really don't know what was going on 😆

njgheorghita · 2019-04-05T17:54:13Z

Pffft that's frustrating, but who cares it works!!

njgheorghita force-pushed the relocate-shh-module branch 12 times, most recently from 74fb1b5 to 7f1d7d0 Compare March 29, 2019 18:23

njgheorghita commented Mar 29, 2019

View reviewed changes

docs/web3.shh.rst Outdated Show resolved Hide resolved

njgheorghita requested review from pipermerriam and kclowes March 29, 2019 18:27

dylanjw reviewed Mar 30, 2019

View reviewed changes

pipermerriam reviewed Apr 1, 2019

View reviewed changes

tests/integration/parity/common.py Show resolved Hide resolved

pipermerriam reviewed Apr 1, 2019

View reviewed changes

njgheorghita force-pushed the relocate-shh-module branch 4 times, most recently from d8e5710 to 750c90a Compare April 2, 2019 16:18

njgheorghita added 4 commits April 3, 2019 12:32

Update geth 1.8.1 to 1.8.22

353338f

Update shh integration tests

3949c75

Update method v2 configuration

1d3d3de

Move web3 method v2 files to _utils folder

6c54808

njgheorghita force-pushed the relocate-shh-module branch 3 times, most recently from f0a9be1 to 174d816 Compare April 4, 2019 11:59

njgheorghita force-pushed the relocate-shh-module branch from 174d816 to 1094a63 Compare April 4, 2019 12:51

njgheorghita force-pushed the relocate-shh-module branch from 1094a63 to 62ea7bd Compare April 4, 2019 16:30

kclowes approved these changes Apr 4, 2019

View reviewed changes

njgheorghita force-pushed the relocate-shh-module branch 2 times, most recently from fad9241 to a489f61 Compare April 5, 2019 17:56

Update whisper docs for geth / parity

27948f7

njgheorghita force-pushed the relocate-shh-module branch from a489f61 to 27948f7 Compare April 5, 2019 17:59

njgheorghita merged commit cba9538 into ethereum:master Apr 5, 2019

njgheorghita deleted the relocate-shh-module branch April 5, 2019 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update shh module & relocate to parity and geth namespaces #1308

Update shh module & relocate to parity and geth namespaces #1308

njgheorghita commented Mar 29, 2019 •

edited

Loading

njgheorghita Mar 29, 2019

dylanjw Mar 30, 2019

dylanjw Mar 30, 2019

dylanjw Mar 30, 2019

pipermerriam Apr 1, 2019

njgheorghita Apr 2, 2019

dylanjw Mar 30, 2019

pipermerriam Apr 1, 2019

pipermerriam Apr 1, 2019

njgheorghita Apr 2, 2019

pipermerriam Apr 1, 2019

njgheorghita commented Apr 4, 2019

kclowes commented Apr 4, 2019

kclowes left a comment

kclowes Apr 4, 2019

njgheorghita commented Apr 5, 2019 •

edited

Loading

pipermerriam commented Apr 5, 2019

njgheorghita commented Apr 5, 2019

kclowes commented Apr 5, 2019 •

edited

Loading

pipermerriam commented Apr 5, 2019

pipermerriam commented Apr 5, 2019

kclowes commented Apr 5, 2019

njgheorghita commented Apr 5, 2019


		.. py:method:: Shh.deleteKeyPair(self, id)

		* Deletes the specifies key if it exists.

Update shh module & relocate to parity and geth namespaces #1308

Update shh module & relocate to parity and geth namespaces #1308

Conversation

njgheorghita commented Mar 29, 2019 • edited Loading

What was wrong?

Cute Animal Picture

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 Apr 4, 2019

kclowes commented Apr 4, 2019

kclowes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita commented Apr 5, 2019 • edited Loading

pipermerriam commented Apr 5, 2019

njgheorghita commented Apr 5, 2019

kclowes commented Apr 5, 2019 • edited Loading

pipermerriam commented Apr 5, 2019

pipermerriam commented Apr 5, 2019

kclowes commented Apr 5, 2019

njgheorghita commented Apr 5, 2019

njgheorghita commented Mar 29, 2019 •

edited

Loading

njgheorghita commented Apr 5, 2019 •

edited

Loading

kclowes commented Apr 5, 2019 •

edited

Loading