RPC api for offchain storage #4694

ark930 · 2020-01-21T09:40:51Z

@tomusdrw
This RP is for RPC api to set/get offchain local storage, See aslo #4420.

parity-cla-bot · 2020-01-21T09:40:55Z

It looks like @ark930 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

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 [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

tomusdrw

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.

tomusdrw · 2020-01-21T09:46:41Z

client/rpc-api/src/chain/mod.rs

@@ -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")]


Suggested change

#[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.

tomusdrw · 2020-01-21T09:47:34Z

client/rpc-api/src/chain/mod.rs

+	#[rpc(name = "chain_setOffchainStorage")]
+	fn set_offchain_storage(
+		&self,
+		prefix: StorageKey,


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.

You mean prefix must be StorageKind type, right?

tomusdrw · 2020-01-21T09:49:35Z

client/src/client.rs

@@ -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<()> {


I'd prefer to avoid going through the Client and use OffchainWorkers or OffchainStorage directly in the RPC.

Ok, I'll try to use OffchainWorkers.

tomusdrw · 2020-01-27T11:09:11Z

@ark930 sad to see it being closed 😞 Is it just a mistake? Is there any way I could help you finish it up?

ark930 · 2020-01-27T15:35:08Z

@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 OffchainStorage.

ark930 · 2020-01-27T15:50:47Z

[clabot:check]

parity-cla-bot · 2020-01-27T15:50:49Z

It looks like @ark930 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

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

tomusdrw · 2020-01-27T15:45:16Z

primitives/core/src/offchain/mod.rs

@@ -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))]


Suggested change

#[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.

tomusdrw · 2020-01-27T15:50:35Z

client/offchain/src/lib.rs

@@ -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,


Would be best to avoid making it public.

tomusdrw · 2020-01-27T15:52:44Z

client/rpc/src/offchain/mod.rs

+	/// Create new instance of Offchain API.
+	pub fn new(storage: T) -> Self {
+		Offchain {
+			storage: Arc::new(Mutex::new(storage)),


A RwLock should suffice here.

tomusdrw · 2020-01-27T15:55:07Z

client/rpc-api/src/offchain/mod.rs

+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<()>;


This all should be Bytes instead of Vec<u8> otherwise we will get an array of numbers instead of hex-string.

ark930 · 2020-01-28T05:44:35Z

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

I try to use OffchainWorkers, but the complier reminder me OffchainWorkers's db is not mutable when I call set method of OffchainStorage. How could I resolve this ploblem?

tomusdrw

@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.

bkchr

Looks good, just some minor nitpicks.

bkchr · 2020-01-28T15:32:13Z

client/rpc-api/src/offchain/error.rs

@@ -0,0 +1,51 @@
+// Copyright 2017-2020 Parity Technologies (UK) Ltd.


Suggested change

// Copyright 2017-2020 Parity Technologies (UK) Ltd.

// Copyright 2020 Parity Technologies (UK) Ltd.

bkchr · 2020-01-28T15:32:39Z

client/rpc-api/src/offchain/mod.rs

@@ -0,0 +1,37 @@
+// Copyright 2017-2020 Parity Technologies (UK) Ltd.


Suggested change

// Copyright 2017-2020 Parity Technologies (UK) Ltd.

// Copyright 2020 Parity Technologies (UK) Ltd.

bkchr · 2020-01-28T15:33:20Z

client/rpc/src/offchain/mod.rs

@@ -0,0 +1,67 @@
+// Copyright 2017-2020 Parity Technologies (UK) Ltd.


Suggested change

// Copyright 2017-2020 Parity Technologies (UK) Ltd.

// Copyright 2020 Parity Technologies (UK) Ltd.

bkchr · 2020-01-28T15:34:46Z

client/rpc/src/offchain/mod.rs

+	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),


Should this be supported in the future? If not, I don't understand why we take kind here.

Yes, it mirrors the API available for offchain workers. Implementation could be done on top of #3263

What is the difference between local and persistent?

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/mod.rs

bkchr · 2020-01-28T15:35:28Z

client/rpc/src/offchain/tests.rs

@@ -0,0 +1,36 @@
+// Copyright 2017-2020 Parity Technologies (UK) Ltd.


Suggested change

// Copyright 2017-2020 Parity Technologies (UK) Ltd.

// Copyright 2020 Parity Technologies (UK) Ltd.

bkchr · 2020-01-28T15:36:01Z

client/service/src/builder.rs

-				system::SystemApi::to_delegate(system),
-				rpc_extensions.clone(),
-			))
+			match offchain_storage.clone() {


Suggested change

match offchain_storage.clone() {

match offchain_storage {

@ark930 please apply this as well.

bkchr · 2020-01-28T15:37:12Z

client/service/src/builder.rs

+			match offchain_storage.clone() {
+				Some(storage) => {
+					let offchain = sc_rpc::offchain::Offchain::new(storage);
+					sc_rpc_server::rpc_handler((


@tomusdrw it would be nice if rpc_handlers would support Option<>, so we don't have this repetition here.

Done: paritytech/jsonrpc#535

xlc · 2020-02-04T21:53:23Z

Some one need to document this as an unsafe RPC https://github.com/paritytech/substrate/wiki/Public-RPC

ark930 requested a review from tomusdrw as a code owner January 21, 2020 09:40

tomusdrw suggested changes Jan 21, 2020

View reviewed changes

ark930 closed this Jan 27, 2020

ark930 force-pushed the ark-rpc-offchain-storage branch from fcfc958 to 03ffa5a Compare January 27, 2020 10:59

Rpc api for offchain storage

317b556

ark930 reopened this Jan 27, 2020

tomusdrw reviewed Jan 27, 2020

View reviewed changes

Replace Vec<u8> to Bytes, replace Mutex to RwLock

8ee23d3

Remove pub

758c6e2

tomusdrw approved these changes Jan 28, 2020

View reviewed changes

tomusdrw added A8-looksgood labels Jan 28, 2020

bkchr reviewed Jan 28, 2020

View reviewed changes

Modify copyright year

6694101

ark930 requested review from bkchr January 28, 2020 17:35

bkchr merged commit acb1b9b into paritytech:master Jan 28, 2020

jacogr mentioned this pull request Jan 29, 2020

Add offline_* RPCs polkadot-js/api#1767

Closed

tomusdrw mentioned this pull request Feb 18, 2020

Allow OffchainWorker Local Storage (DB) set/get over RPC. #4420

Closed

xlc mentioned this pull request Feb 22, 2020

Pass arguments to off-chain worker #3912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC api for offchain storage #4694

RPC api for offchain storage #4694

ark930 commented Jan 21, 2020

parity-cla-bot commented Jan 21, 2020

tomusdrw left a comment

tomusdrw Jan 21, 2020

tomusdrw Jan 21, 2020

ark930 Jan 21, 2020

tomusdrw Jan 21, 2020

ark930 Jan 21, 2020

tomusdrw commented Jan 27, 2020

ark930 commented Jan 27, 2020

ark930 commented Jan 27, 2020

parity-cla-bot commented Jan 27, 2020

tomusdrw left a comment

tomusdrw Jan 27, 2020

tomusdrw Jan 27, 2020

tomusdrw Jan 27, 2020

tomusdrw Jan 27, 2020

ark930 commented Jan 28, 2020

tomusdrw left a comment

bkchr left a comment

bkchr Jan 28, 2020

bkchr Jan 28, 2020

bkchr Jan 28, 2020

bkchr Jan 28, 2020

tomusdrw Jan 28, 2020

bkchr Jan 28, 2020

tomusdrw Jan 28, 2020 •

edited

Loading

bkchr Jan 28, 2020

bkchr Jan 28, 2020

bkchr Jan 28, 2020

bkchr Jan 28, 2020

tomusdrw Jan 28, 2020

xlc commented Feb 4, 2020

	#[rpc(name = "chain_setOffchainStorage")]
	#[rpc(name = "offchain_localStorageSet")]

	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
	#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]

		@@ -0,0 +1,51 @@
		// Copyright 2017-2020 Parity Technologies (UK) Ltd.

	// Copyright 2017-2020 Parity Technologies (UK) Ltd.
	// Copyright 2020 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,37 @@
		// Copyright 2017-2020 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,67 @@
		// Copyright 2017-2020 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,36 @@
		// Copyright 2017-2020 Parity Technologies (UK) Ltd.

RPC api for offchain storage #4694

RPC api for offchain storage #4694

Conversation

ark930 commented Jan 21, 2020

parity-cla-bot commented Jan 21, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Jan 27, 2020

ark930 commented Jan 27, 2020

ark930 commented Jan 27, 2020

parity-cla-bot commented Jan 27, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ark930 commented Jan 28, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlc commented Feb 4, 2020

tomusdrw Jan 28, 2020 •

edited

Loading