-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Offchain execution extensions #4145
Conversation
| { | ||
| let proving_backend = proving_backend::ProvingBackend::new(trie_backend); | ||
| let mut extensions = Extensions::new(); | ||
| if let Some(keystore) = keystore { |
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.
Do we really need keystore here? If that can be invoked from the network context (i.e. light client requesting to prove some execution) we can end up signing something with a local key and giving it back as a response.
| { | ||
| let mut extensions = Extensions::new(); | ||
| if let Some(keystore) = keystore { | ||
| extensions.register(keystore); |
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.
Same here.
primitives/sr-io/with_std.rs
Outdated
| } | ||
|
|
||
| impl OffchainApi for () { | ||
| fn submit_transaction(data: Vec<u8>) -> Result<(), ()> { |
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 file is not used anymore. It was probably readded in your big refactoring pr.
andresilva
left a comment
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.
lgtm. so for my use case I should just provide a client to the grandpa code that has already pre-registered all the necessary extensions, so as to avoid introducing those types into the grandpa crate (e.g. transaction pool).
your comments regarding unnecessary registration of keystore extensions are also pertinent.
| side_effects_handler: Option<OffchainExt>, | ||
| recorder: &Option<ProofRecorder<Block>>, | ||
| enable_keystore: bool, | ||
| extensions: Option<Extensions>, |
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.
indented with spaces.
| pub mod notifications; | ||
|
|
||
| pub use error::*; | ||
| // TODO: avoid re-exports |
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.
Should this have been handled?
This PR is an attempt to clean up how we deal with externalities extensions for offchain calls (i.e. the additional APIs available for some contexts), namely:
OffchainWorkerextensions,TransactionPoolandKeystore.Instead of passing things separately this PR introduces a
ExecutionExtensionsidea, where allExternalitiesExtensionscan be registered (avoiding cycles though). Later on in the call stack we passExtensionsinstead of arbitraryOption<KeystorePtr>orOption<OffchainExt>stuff.This cleans up some APIs and also avoids uneccesary direct dependencies.
CC @andresilva - finally :)