Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Test for OpenRPC subscription methods on trace/debug modules / whitelist-style pubsub skips #364

Conversation

meowsbits
Copy link
Contributor

@meowsbits meowsbits commented Apr 23, 2021

I know it's annoying that I've polluted the diff with the regenerated docs... 🤷

The issue this is trying to solve is that #356's https://github.com/etclabscore/core-geth/pull/356/files#diff-98749d04d0efbddfeb407114a7c48c0309646642e7ff67b807f39ecdc5809924R924 adds debug_(un|)subscribe and trace_(un|)subscribe which AFAIK don't exist; those modules don't use the same weird pattern that eth_subscribe does.

Please feel free to cherry-pick, ignore, modify, or whatever here. Just a sketch of concept.

Date: 2021-04-23 06:43:41-05:00
Signed-off-by: meows <b5c6@protonmail.com>
These modules don't have '_subscribe' methods.
Removing these special registrations is not for
sure what I want to do (maybe I actually just
want to change the mock methods provided) but this
is a lego piece.

I'm removing it now experimentally.

Date: 2021-04-23 06:48:42-05:00
Signed-off-by: meows <b5c6@protonmail.com>
I want to allow debug and trace API modules
through.

Date: 2021-04-23 06:49:26-05:00
Signed-off-by: meows <b5c6@protonmail.com>
…methods

This adds a whitelist-style condition to the
PubSub handling for unconventional methods
and receivers.

See the added and commented methods in ethclient TestRPCDiscover
to see the outcome.

Date: 2021-04-23 07:43:42-05:00
Signed-off-by: meows <b5c6@protonmail.com>
These are now not required anymore
since we allow the reflector to handle
them directly (b/c the whitelist-style
logic the previous commit(s) add.

Date: 2021-04-23 07:45:24-05:00
Signed-off-by: meows <b5c6@protonmail.com>
@meowsbits meowsbits requested a review from ziogaschr April 23, 2021 12:48
"eth_getFilterChanges",
"eth_getFilterLogs",
// "eth_getFilterChanges",
// "eth_getFilterLogs",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left the removed methods commented for 👀

// This catches MOST of them (except SubscribeSyncStatus)
return false
pkgStr := method.Type.In(0).String()
if (isPubSub(method.Type) && pkgStr == "*eth.PublicEthereumAPI") || pkgStr == "*filters.PublicFilterAPI" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the important part.

I'm using the stringer condition because using reflect.TypeOf(&eth.PublicEthereumAPI) will cause an import cycle error.

I think its correct that all the methods from *filters.PublicFilterAPI can be skipped since, AFAIU, these are all "unconventional" methods in service of either eth or debug or trace methods which use subs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziogaschr
Copy link
Member

Moving our discussion here as well. (debug|trace)_(un)subscribe exists and this is the way to use the needed methods.
This approach looks cleaner and on top of this one we have to create a new template generator for the subscription methods.

@meowsbits
Copy link
Contributor Author

meowsbits commented Apr 23, 2021

Oh man, well if since (trace|debug)_(un|)subscribe exists, then....
a.) I'm very wrong
b.) your approach might be way better?

@ziogaschr
Copy link
Member

Let me try something later tonight or tomorrow on this PR as I am working on trace stuff now. I think you opened my eyes one more time and the solution is in the intersection of both PRs. Thanks

@meowsbits
Copy link
Contributor Author

Closing since @ziogaschr has got an improved version already going on at #356 , which is this PR's base branch.

@meowsbits meowsbits closed this Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants