srml/authority-discovery: Abstract session key type #3698
Conversation
Previously `srml/authority-discovery` dependet on the `srml/im-online` session key type directly. With this patch `srml/authority-discovery` is generic over the session key type it is going to use, as long as it implements the RuntimeAppPublic trait. With this patch one can use the `srml/authority-discovery` module without the `srml/im-online` module. Next to the above, this patch configures `node/runtime` to use the babe session keys for the authority discovery module.
srml/authority-discovery/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| app-crypto = { package = "substrate-application-crypto", path = "../../core/application-crypto", default-features = false } | ||
| babe-primitives = { package = "substrate-consensus-babe-primitives", path = "../../core/consensus/babe/primitives", default-features = false } |
There was a problem hiding this comment.
This needs to go into dev-dependencies. You don't need it for the normal build.
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
One change required, otherwise looks good.
srml/authority-discovery/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| app-crypto = { package = "substrate-application-crypto", path = "../../core/application-crypto", default-features = false } | ||
| babe-primitives = { package = "substrate-consensus-babe-primitives", path = "../../core/consensus/babe/primitives", default-features = false } |
|
@bkchr thanks for the review! Can you take another look? |
| type AuthoritySignatureFor<T> = | ||
| <<T as im_online::Trait>::AuthorityId as RuntimeAppPublic>::Signature; | ||
| pub trait Trait: system::Trait + session::Trait { | ||
| type AuthorityId: RuntimeAppPublic + Default + Decode + Encode + PartialEq; |
node/runtime/src/lib.rs
Outdated
| // and set impl_version to equal spec_version. If only runtime | ||
| // implementation changes and behavior does not, then leave spec_version as | ||
| // is and increment impl_version. | ||
| spec_version: 165, |
There was a problem hiding this comment.
Shouldn't the spec version be increased (and not impl version)?
There was a problem hiding this comment.
@andresilva do I understand correctly that we increase the spec_version whenever we have breaking changes visible only outside of the runtime boundary?
This patch set does not touch the core/authority-discovery/primitives::AuthorityDiscoveryApi, thus as far as I can tell it is not visible outside of the runtime boundary. Am I missing something?
There was a problem hiding this comment.
Given that we switch to Babe as authority id, @andresilva is right. You change the behavior of runtime and this new runtime would not be compatible with the old runtime when it comes to authority discovery.
There was a problem hiding this comment.
My vague definition for when to increment spec_version is a bit stricter than that, since internal behavior of the module can change without any changes being introduced at the boundaries. Keeping the same spec_version means that a previous compiled blob of the runtime without these changes can be executed as is.
|
can merge when passing |
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
Merge master to fix CI issues and also bump spec version.
…very-no-im-online
5 participants
Previously
srml/authority-discoverydepended on thesrml/im-onlinesession key type directly. With this patch
srml/authority-discoveryisgeneric over the session key type it is going to use, as long as it
implements the RuntimeAppPublic trait.
With this patch one can use the
srml/authority-discoverymodulewithout the
srml/im-onlinemodule.Next to the above, this patch configures
node/runtimeto use the babesession keys for the authority discovery module.