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

Expose plugin system, move walletdb to a plugin #156

Closed
wants to merge 54 commits into from
Closed

Conversation

chjj
Copy link
Member

@chjj chjj commented Mar 10, 2017

Finally exposing a new plugin system similar to the one described in #98.

A few things needed to happen before this was possible:

  • Expose a config object on the Node object so plugins can access configuration options.
  • Add "mounted" servers to the HTTP base server, allowing plugins to mount their behavior on the current server.
  • Allow adding of custom RPC calls to the main RPC object.

These three things allowed the wallet HTTP routes and RPC calls to be separated into their own modules:

As far as the Node object goes, a plugins option has been added. e.g.

var node = new FullNode({
  plugins: ['walletdb', '../stratum/lib/stratum'],
  loader: require
});

require must be passed in from the outside. This avoids bcoin having to directly make conditional requires.

A simple plugin might look like:

function MyPlugin(options) {
  this.chain = options.chain;
  this.myOption = options.myOption;
}
MyPlugin.prototype.open = function() { return Promise.resolve(); };
MyPlugin.prototype.close = function() { return Promise.resolve(); };
MyPlugin.id = 'myplugin'; // give the plugin a name
MyPlugin.init = function(node) { // called by the plugin loader
  return new MyPlugin({
    chain: node.chain,
    myOption: node.config.get('my-option')
  });
};
module.exports = MyPlugin;

I'm now wondering if we should have the wallet disabled by default (requiring bcoin to be run with bcoin --plugins walletdb if a user wants a wallet). The eventual goal is to move the wallet code out of this repository completely. The next task would be to have the wallet run as its own server (very close to happening now).

Thoughts, comments, criticism are all appreciated.

@bucko13
Copy link
Contributor

bucko13 commented Mar 10, 2017

This looks awesome! The idea of plugins is great. I saw in #98 you were considering the name modules, any reason for sticking with plugins? I personally kind of prefer 'modules'. walletdb at least feels like more of a module than a plugin. But that's a semantic thing.

For enabling the plugin with node.require does it have to be "require" or can you make it anything via the loader option? Require was a little confusing to me when I first read it because of the nodejs module system that uses the require.js module system. Something like node.load('walletdb') seems like it would read well too.

@chjj
Copy link
Member Author

chjj commented Mar 14, 2017

This looks awesome! The idea of plugins is great. I saw in #98 you were considering the name modules, any reason for sticking with plugins? I personally kind of prefer 'modules'. walletdb at least feels like more of a module than a plugin. But that's a semantic thing.

Yeah, I don't really like the plugin name either, but module may cause confusion with node.js modules. Maybe we can switch it to modules. That term does seem to fit better.

For enabling the plugin with node.require does it have to be "require" or can you make it anything via the loader option? Require was a little confusing to me when I first read it because of the nodejs module system that uses the require.js module system. Something like node.load('walletdb') seems like it would read well too.

I like require better since you're not necessarily loading anything (you're requiring something as a dependency).

@chjj
Copy link
Member Author

chjj commented Mar 14, 2017

@mcelrath, do you think this would impact usage of the SPVNode?

@chjj
Copy link
Member Author

chjj commented Mar 14, 2017

Merged in, but I'm going to keep walletdb as a default plugin for now.

@chjj chjj closed this Mar 14, 2017
@mcelrath
Copy link

There are a lot of changes here...I'll evaluate it.

We're using the walletdb to hold SPV transactions, but not a wallet (bcoin has no privkeys and no coins). So if that interface has changed it will certainly affect us.

@chjj
Copy link
Member Author

chjj commented Mar 14, 2017

@mcelrath, yeah, lots of changes, but it had to be done. The wallet doesn't belong on the full node. That being said, I may keep walletdb on the SPVNode object just like before (since the spvnode is basically useless without the wallet).

@chjj chjj deleted the plugin-refactor branch July 17, 2017 21:45
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.

3 participants