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

Execution order of ZeroNet plugins #2128

Closed
filips123 opened this issue Aug 7, 2019 · 17 comments
Closed

Execution order of ZeroNet plugins #2128

filips123 opened this issue Aug 7, 2019 · 17 comments

Comments

@filips123
Copy link
Contributor

In which order do ZeroNet plugins execute when registering to core classes?

I tested this and it seems to be in reverse alphabetical order. Is that right?


Because of that, I have a problem with developing the ZeroNet plugin for Ethereum Name Service. It will be called ENS so it will be executed somewhere in the middle. This is a problem because plugins that are executed before it won't have resolved domain so they will probably crash. This isn't a problem for NameCoin plugin because it is called Zeroname so it is executed first.

Is it possible to fix this? A quick solution would be to rename my plugin to ZeroENS but there should be better handling of plugin order.


There needs to be support to specify priority and execution order of plugins.

There need to be a few priority classes reserved for a specific type of plugin:

  • High - For DNS plugins and other plugins that need to be executed first
  • Medium - Normal plugins
  • Low - Network plugins that need to be executed last

So I would recommend documenting those three priority classes (maybe more if needed) and represent them with numbers:

  • High - 100
  • Medion - 50
  • Low - 0

Here, the highest number would execute first. Plugins with the same number should be executed in reverse alphabetical order (as currently).

This would allow plugins to categorize into groups, but still allow exceptional plugins to be executed before or after majority or other plugins. Normal plugins should be using one of those three numbers but they would also be able to use other numbers if needed.

@rllola
Copy link
Contributor

rllola commented Aug 7, 2019

I think it is right because it is going to check all the folders in the plugins folder which doesn't start with disabled-.

As far as I know this behavior is going to be modified as we are going to explicitly ask to activate plugins inside the config file. You might be able to specify the order then.

@filips123
Copy link
Contributor Author

filips123 commented Aug 7, 2019

As far as I know this behavior is going to be modified as we are going to explicitly ask to activate plugins inside the config file. You might be able to specify the order then.

Yes, but it would be better to also allow plugins themselves to define when they should be executed. So something like @PluginManager.registerTo('SiteManager', priority=50) where priority should be an optional argument (and set to 50 by default) to maintain backwards compatibility. And if multiple plugins have the same priority, they should be executed depending on alphabetical order or order in the config file.

@HelloZeroNet
Copy link
Owner

The plugins are loaded alphabetically, but if you subclass the same class multiple times, then the last will be called as it's overrides the previous function (then you can call the previous function using super())

There could be some cases where the order matters, but I think for domain resolver it should note be an issue.

@rllola
Copy link
Contributor

rllola commented Aug 7, 2019

Do you have the source code of your plugin somewhere ? Maybe we can brainstorm to help you fix your problem.

It might be an issue with the zeroname plugin now that I think about it. As it should only do something if it catch a domain name ending with .bit and if I am not wrong ENS if for Ethereum and therefore end with .eth.

@filips123
Copy link
Contributor Author

@HelloZeroNet @rllola There is an issue for my plugin. Plugin TranslateSite will be called before my ENS. It depends on path argument of actionSiteMedia to get site, but because my plugin is still not executed, it will contain .eth domain instead of the address. It will fail and site content won't load. Although this is a problem with TranslateSite and ENS it could probably happen with any other plugins.

As it should only do something if it catch a domain name ending with .bit and if I am not wrong ENS if for Ethereum and therefore end with .eth.

I fixed this with also loading plugin on SiteManager that also checks for .eth domains. And all this should work perfectly except above problem.

@rllola
Copy link
Contributor

rllola commented Aug 7, 2019

There is no much that we can do to help without your code. And like it was mention for domain name resolver it should not be a problem. Your problem is compatibility with the other zeroname plugin, right?

@filips123
Copy link
Contributor Author

filips123 commented Aug 8, 2019

Here is my plugin: ENS

Because there are currently no ZeroNet ENS domains, I modified it for testing to return ZeroHello site address at domain example.eth. You should test that domain.

Also, to use it you need to install my version of Web3.py which contains changes that are currently waiting to be merged into Web3.py. Also, you can install some missing Python wheels (if needed) from here.

@rllola
Copy link
Contributor

rllola commented Aug 10, 2019

Hi,

Sorry for the delay. Yeah I get your problem. It is indeed calling the actionSiteMedia function from first ZeroName (probably because it is the last one loaded as plugins are registered in alphabetical order?) then TranslateSite and then ENS but is it is too late because TranslateSite throw an error.

Could you confirm I am one right track ?

I am wondering could it be possible instead of creating a new plugin to integrate it to the ZeroName plugin ? We could activate differents name provider using the config file ?

@rllola
Copy link
Contributor

rllola commented Aug 10, 2019

An alternative solution would be to verify that path in TranslateSite is indeed an address and if it is a domain please try to resolve.

@rllola
Copy link
Contributor

rllola commented Aug 10, 2019

Actually no need to do anything with TranslateSite. It is ZeroName plugin which is missing the super() call and stop the propagation.

https://github.com/HelloZeroNet/ZeroNet/blob/py3/plugins/Zeroname/SiteManagerPlugin.py#L38-L53

I will prepare a PR. And @filips123 you might want to add it too in your SiteManagerPlugin class.

@filips123
Copy link
Contributor Author

filips123 commented Aug 11, 2019

@rllola Thank you for that. Unfortunately, I will only be able to test this next Sunday, but you can also test this with code I provided.

you might want to add it too in your SiteManagerPlugin class.

What do you mean with that? Can you provide now details?

Update: Do you mean that I should add the same change as your in my own plugin?

@purplesyringa
Copy link
Contributor

Update: Do you mean that I should add the same change as your in my own plugin?

I bet that's what they meant.

@rllola
Copy link
Contributor

rllola commented Aug 11, 2019

Yeah. You can see the modification in the PR. It is so people won't have the same problem if we have another domain plugin.

No problem. Take your time to test and let us know if it does fix your problem.

Example of what you might need to add based on your code :

@PluginManager.registerTo('SiteManager')
class SiteManagerPlugin:
    _ens_resolver = None

...

    def resolveDomain(self, domain):
        return self.ens_resolver.resolveDomain(domain) or super(SiteManagerPlugin, self).resolveDomain(domain)

@HelloZeroNet
Copy link
Owner

I have added a modification that should make it easier to create domain resolver plugins: 2bdd073

Example Ethname plugin:

import re

from Plugin import PluginManager


@PluginManager.registerTo("SiteManager")
class SiteManagerPlugin(object):
    # Return: True if the address is .bit domain
    def isEthDomain(self, address):
        return re.match(r"(.*?)([A-Za-z0-9_-]+\.eth)$", address)

    # Resolve domain
    # Return: The address or None
    def resolveEthDomain(self, domain):
        domains = {
            "zerohello.eth": "1HeLLo4uzjaLetFx6NH3PMwFP3qbRbTf3D",
            "zeroblog.eth": "1BLogC9LN4oPDcruNz3qo1ysa133E9AGg8"
        }

        return domains.get(domain)

    # Turn domain into address
    def resolveDomain(self, domain):
        return self.resolveEthDomain(domain) or super(SiteManagerPlugin, self).resolveDomain(domain)

    # Return: True if the address is domain
    def isDomain(self, address):
        return self.isEthDomain(address) or super(SiteManagerPlugin, self).isDomain(address) 

@filips123
Copy link
Contributor Author

@HelloZeroNet My plugin now works with that fixes so I will close this issue. I will soon create PR for the plugin.

@rllola
Copy link
Contributor

rllola commented Aug 21, 2019

@filips123 One question. Can you verify if it works without ZeroName plugin and only yours ?

@filips123
Copy link
Contributor Author

@rllola No, it doesn't work. It has the same problem as before with disabled Zeroname. Log output is here.

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

No branches or pull requests

4 participants