-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
It looks like @ark930 hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, please reply to this thread with Many thanks, Parity Technologies CLA Bot |
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.
Hi @ark930! Thank you for your contribution, this is definitely a very good start, but I'd rather see things in different places, sorry for not specifying that directly in the issue.
client/rpc-api/src/chain/mod.rs
Outdated
@@ -54,6 +55,23 @@ pub trait ChainApi<Number, Hash, Header, SignedBlock> { | |||
#[rpc(name = "chain_getFinalizedHead", alias("chain_getFinalisedHead"))] | |||
fn finalized_head(&self) -> Result<Hash>; | |||
|
|||
/// Set offchain storage under given key and prefix. | |||
#[rpc(name = "chain_setOffchainStorage")] |
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.
#[rpc(name = "chain_setOffchainStorage")] | |
#[rpc(name = "offchain_localStorageSet")] |
I'd suggest to keep the naming from internal modules, i.e. Local Storage
and verb at the end.
Also IMHO it deserves a separate namespace in the RPC (i.e. not chain_
, but rather offchain_
) and a separate file for this.
client/rpc-api/src/chain/mod.rs
Outdated
#[rpc(name = "chain_setOffchainStorage")] | ||
fn set_offchain_storage( | ||
&self, | ||
prefix: StorageKey, |
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.
Could we rather keep the enum instead of allowing any kind of prefix directly? I think this will get sorted as well if you switch to use OffchainWorkers
structure directly instead of going through Client
to low-level OffchainStorage
db.
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.
You mean prefix must be StorageKind type, right?
client/src/client.rs
Outdated
@@ -1265,6 +1266,25 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where | |||
self.backend.blockchain().justification(*id) | |||
} | |||
|
|||
/// Set offchain storage under given key and prefix. | |||
pub fn set_offchain_storage(&self, prefix: &[u8], key: &[u8], value: &[u8]) -> sp_blockchain::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.
I'd prefer to avoid going through the Client
and use OffchainWorkers
or OffchainStorage
directly in the RPC.
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.
Ok, I'll try to use OffchainWorkers.
fcfc958
to
03ffa5a
Compare
@ark930 sad to see it being closed 😞 Is it just a mistake? Is there any way I could help you finish it up? |
I just revert commit and re-implement Offchain RPC Api by high level |
|
It looks like @ark930 signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
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 was thinking about using the entire OffchainWorkers
struct, as it's already in Arc
. Cloning offchain::LocalStorage
works fine, so I'm fine keeping it as-is. Few small things, but looks good
primitives/core/src/offchain/mod.rs
Outdated
@@ -50,6 +52,7 @@ pub trait OffchainStorage: Clone + Send + Sync { | |||
|
|||
/// A type of supported crypto. | |||
#[derive(Clone, Copy, PartialEq, Eq, Encode, Decode, RuntimeDebug, PassByEnum)] | |||
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] |
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.
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] | |
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] |
You can do this to avoid adding extra, feature-gated import line.
client/offchain/src/lib.rs
Outdated
@@ -51,7 +51,7 @@ pub use sp_offchain::{OffchainWorkerApi, STORAGE_PREFIX}; | |||
/// An offchain workers manager. | |||
pub struct OffchainWorkers<Client, Storage, Block: traits::Block> { | |||
client: Arc<Client>, | |||
db: Storage, | |||
pub db: Storage, |
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.
Would be best to avoid making it public.
client/rpc/src/offchain/mod.rs
Outdated
/// Create new instance of Offchain API. | ||
pub fn new(storage: T) -> Self { | ||
Offchain { | ||
storage: Arc::new(Mutex::new(storage)), |
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.
A RwLock
should suffice here.
client/rpc-api/src/offchain/mod.rs
Outdated
pub trait OffchainApi { | ||
/// Set offchain local storage under given key and prefix. | ||
#[rpc(name = "offchain_localStorageSet")] | ||
fn set_local_storage(&self, kind: StorageKind, key: Vec<u8>, value: 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 all should be Bytes
instead of Vec<u8>
otherwise we will get an array of numbers instead of hex-string.
I try to use |
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.
@ark930 It's fine. Probably OffchainWorkers
would need to have a different API for this and it's not really worth it. I like how the PR looks already, so let's merge it in current form.
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.
Looks good, just some minor nitpicks.
client/rpc-api/src/offchain/error.rs
Outdated
@@ -0,0 +1,51 @@ | |||
// Copyright 2017-2020 Parity Technologies (UK) Ltd. |
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.
// Copyright 2017-2020 Parity Technologies (UK) Ltd. | |
// Copyright 2020 Parity Technologies (UK) Ltd. |
client/rpc-api/src/offchain/mod.rs
Outdated
@@ -0,0 +1,37 @@ | |||
// Copyright 2017-2020 Parity Technologies (UK) Ltd. |
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.
// Copyright 2017-2020 Parity Technologies (UK) Ltd. | |
// Copyright 2020 Parity Technologies (UK) Ltd. |
client/rpc/src/offchain/mod.rs
Outdated
@@ -0,0 +1,67 @@ | |||
// Copyright 2017-2020 Parity Technologies (UK) Ltd. |
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.
// Copyright 2017-2020 Parity Technologies (UK) Ltd. | |
// Copyright 2020 Parity Technologies (UK) Ltd. |
fn set_local_storage(&self, kind: StorageKind, key: Bytes, value: Bytes) -> Result<()> { | ||
let prefix = match kind { | ||
StorageKind::PERSISTENT => sp_offchain::STORAGE_PREFIX, | ||
StorageKind::LOCAL => return Err(Error::UnavailableStorageKind), |
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 be supported in the future? If not, I don't understand why we take kind
here.
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.
Yes, it mirrors the API available for offchain workers. Implementation could be done on top of #3263
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.
What is the difference between local and persistent?
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.
Fork awareness. LOCAL
entries should be removed in case there is an reorg, while PERSISTENT
should stay even if reorg happened (i.e. we see results produced by offchain workers that run on a different fork). The second one is useful if you want to make sure to avoid duplication of external "side effects".
client/rpc/src/offchain/tests.rs
Outdated
@@ -0,0 +1,36 @@ | |||
// Copyright 2017-2020 Parity Technologies (UK) Ltd. |
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.
// Copyright 2017-2020 Parity Technologies (UK) Ltd. | |
// Copyright 2020 Parity Technologies (UK) Ltd. |
system::SystemApi::to_delegate(system), | ||
rpc_extensions.clone(), | ||
)) | ||
match offchain_storage.clone() { |
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.
match offchain_storage.clone() { | |
match offchain_storage { |
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.
@ark930 please apply this as well.
match offchain_storage.clone() { | ||
Some(storage) => { | ||
let offchain = sc_rpc::offchain::Offchain::new(storage); | ||
sc_rpc_server::rpc_handler(( |
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.
@tomusdrw it would be nice if rpc_handlers
would support Option<>
, so we don't have this repetition here.
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.
Done: paritytech/jsonrpc#535
Some one need to document this as an unsafe RPC https://github.com/paritytech/substrate/wiki/Public-RPC |
@tomusdrw
This RP is for RPC api to set/get offchain local storage, See aslo #4420.