-
Notifications
You must be signed in to change notification settings - Fork 550
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
Add basic plugin support #5503
Add basic plugin support #5503
Conversation
~metadata: | ||
[ ("path", `String path) | ||
; ("error", `String (Dynlink.error_message err)) ] ) ; | ||
coda_lib' := None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the plugins are loaded, regardless of error status, get_coda_lib
always raises?
Should the exception be named Done_initializing plugins
?
Should an error be fatal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error can also happen before init if a plugin gets linked into the main coda binary by mistake. Since linking order can affect this (and is hell to debug) I took the stance that it should always be an error if a plugin is linked into the coda binary itself.
I've updated this to make these errors fatal.
Should plugins be required to support an interface of some kind, perhaps indicating a name, author, description, that could be logged (or result in failure)? |
For now, I think we should treat this as an avenue for testing fees in tokens without writing a production-ready exchange rate mechanism. I think there should probably be a full RFC for how the API and system should work before we promote this as a feature or recommend that anyone uses it -- especially since it directly exposes the internals at the moment. It might be better not to expose it in released binaries yet. Do you think this should be behind a compile-time flag for now? |
That sounds like a good idea. |
@@ -46,6 +46,21 @@ let chain_id ~genesis_state_hash ~genesis_constants = | |||
[%%inject | |||
"compile_time_current_protocol_version", current_protocol_version] | |||
|
|||
[%%if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use ifdef plugins
, then you only have to define it in the profiles where it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an error from optcomp, where it asks for [%%undef plugins]
in the config :/
This PR adds basic plugin support, using the
Dynlink
module to dynamically load modules passed on the command line. Currently, the interface only exposes a reference toCoda_lib.t
, and this PR adds no hooks that a plugin could attach to.We want this to allow node operators to manage exchange rates, without our prescribing a specific system. This will allow us to expose simple hooks that operators can use to manage rates in any way that they see fit.
It may also make sense to move some other things to be plugins, or expose plugin interfaces for them. For example,
To test:
Lines similar to these should appear in the log:
Checklist: