Refactors the offchain worker api #3150
Conversation
dvc94ch
added
the
A3-in_progress
dvc94ch
force-pushed
the
dvc-offchain-api
branch
from
4226ecb to
e3b6d7b
Compare
dvc94ch
added
A0-please_review
|
|
||
| /// Returns currently configured authority key. | ||
| fn authority_key<TPair: crypto::Pair>(&self) -> Option<TPair>; | ||
| fn authority_key(&self, block_id: &BlockId<Block>) -> Option<Self::ConsensusPair>; |
There was a problem hiding this comment.
it's no worse than at present, so i'll let it pass, but in general i'd prefer to avoid any notion of "authority key" over this API. there should really just be the key store and if you want the Babe, Aura or Grandpa key, then you have to consult that module should implement/use a special off-chain API that allows you to find it.
There was a problem hiding this comment.
this is because we already have no single key that could reasonably be considered the "authority key". authorities in polkadot will have:
- network key
- babe key
- grandpa key
- session-change key
- controller account key
- parachain attestation key
they're all equally "authority" keys, yet this API presupposes that there exists only a single one.
| fn authority_key(&self, block_id: &BlockId<Block>) -> Option<Self::ConsensusPair>; | ||
|
|
||
| /// Returns currently configured finality gadget authority key. | ||
| fn fg_authority_key(&self, block_id: &BlockId<Block>) -> Option<Self::FinalityPair>; |
| kind: CryptoKind, | ||
| }, | ||
| /// Use the key the block authoring algorithm uses. | ||
| AuthorityKey, |
There was a problem hiding this comment.
this really isn't a thing; the block authoring algorithm won't necessarily use a (single) key. it could be a multi-key system, it could be a PoW system, it could be a hash-chain based system. so i'd prefer to move away from this. that said, as a refactoring it can wait, but please create an issue for it and mark for substrate 2.0 release.
| struct Factory { | ||
| Block = Block, | ||
| ConsensusPair = AuraPair, | ||
| FinalityPair = GrandpaPair, |
There was a problem hiding this comment.
again, not happy about this; i don't want to bake this stuff in; please mark for refactoring ASAP.
gavofyork
left a comment
gavofyork
left a comment
There was a problem hiding this comment.
Achieves needed goals, but needs further refactoring for a clean API.
gavofyork
added
A6-mustntgrumble
and removed
A0-please_review
|
@dvc94ch Thanks, the |
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
Looks sane, although I'm not 100% what we are trying to achieve. Seems that previously the offchain worker code was deciding on the expected crypto type.
Now we have an enum and the matching types are hardcoded in the factory, I think it would be better to give more freedom to the users.
Also I think it might be a good idea to change the underlying API of keystore to actually support the different kinds of keys more explicitly (i.e. finality gadget, session, etc), than simply matching on the expected type and probing all possible options.
If the PR is mostly about refactoring then it's ok, although I think the original plan was also to relax the verify API a bit and have it accept raw public key instead of CryptoKey.
|
|
||
| /// Encrypt a piece of data using given crypto key. | ||
| /// | ||
| /// If `key` is `None`, it will attempt to use current authority key of `CryptoKind`. |
There was a problem hiding this comment.
The docs in the entire PR could do with an update too.
| .ok_or(())?; | ||
| Ok(Key::AuthorityKey(key)) | ||
| } | ||
| CryptoKey::FgAuthorityKey => { |
There was a problem hiding this comment.
Fg is super cryptic, can't we just call it finality_gadget_key?
| } | ||
| } | ||
|
|
||
| enum LocalKey { |
There was a problem hiding this comment.
I'd merge the whole enum with Key for simplicity to be honest, but it's a really minor grumble
| /// Opaque type for created crypto keys. | ||
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| /// Key to use in the offchain worker crypto api. | ||
| #[derive(Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
We need Encode/Decode. Offchain workers users need a way to store that entity between runs.
|
Merged on the basis that it's urgently needed, is no worse than current, and that it'll be refactored away in the near future. |
6 participants
Required for #3034 Closes #3150
cc @gavofyork @tomusdrw @cmichi