[Api] add `update_runtime` function and return metadata from cache #337

haerdib · 2022-11-22T14:34:47Z

💥 Breaking Changes:

Api variables are no longer public. They should be accessed via the getter methods
get_metadata, get_spec_version, get_genesis_hash are now private. The user should access these values via the cached values in the Api struct.
❗ Careful: In case one relied on get_metadata returning the most recent metadata from the node, that's not the case any more. If that's necessary, run update_runtime() before calling the cache via metadata().
Setter methods (such as set_signer) now take a mutable reference and do not return a Self anymore.

💚 Feature Changes:

Added update_runtime function to update the Api in case of a runtime upgrade
removes extra _get_request function: RpcClient now directly returns an Option without mapping it to an empty string and back to an Option.
Added some descriptive comments and split up the impl Api for a more sensible layout.

node-api/src/metadata.rs

echevrier

Only one question.

echevrier · 2022-11-22T18:13:18Z

src/api/api_client.rs


 		match version {
 			Some(v) => serde_json::from_str(&v).map_err(|e| e.into()),
 			None => Err(ApiClientError::RuntimeVersion),
 		}
 	}

-	fn _get_metadata(client: &Client) -> ApiResult<RuntimeMetadataPrefixed> {
+	pub fn get_metadata_from_node(client: &Client) -> ApiResult<RuntimeMetadataPrefixed> {
 		let jsonreq = json_req::state_get_metadata();


The names get_metadata, get_metadata_from_node (and similar ones) are a bit confusing, as if the metadata returned by get_metadata is not the node's metadata. The difference is not the source of the metadata, but its currentness.
Wouldn't it be better to have get_cached_metadata for get_metadata and let get_metadata for get_metadata_from_node ?

I see your point, but I think the get_metadata should stay as is, because that should be the usual way to retrieve the metadata. Getting the metadata from the node should be the non-normal way, therefore warranting the longer name, I believe.

How about query_metadata_from_node?

Perfect. That's clear. Thanks

I think we are mixing concepts here, get_* sometimes performs a network request now, and sometimes it does not, this inconsistency is confusing.

I suggest get_* always talks to the node, then we don't need to distinguish between query_metadata_from_node to emphasize that it actually does the same as most of the get_* calls.

We don't have to re-invent the wheel here, we can just do:

api.metadata() is a simple getter to the field
api.get_metdata() is the network request, however, do we even want to keep it?

Same for the runtime version.

api.get_metdata() is the network request, however, do we even want to keep it?

Not sure. We need a private node query method, because the api needs it to be able to update the cached metdata, but it does not need to be public.. because one could just run update_... and then query the cached metadata.

I don't have a strong opinion here, but well.. why not make it private and force the user to update cached metadata if up-to-date metadata is needed.

I don't have a strong opinion here, but well.. why not make it private and force the user to update cached metadata if up-to-date metadata is needed.

This seems good to me! However, the private one should still be get_metadata IMO. :)

Renamed accordingly 👍

Also restructured a little, because it was veery chaotic. Updated the PR description accordingly to what has been changed.

clangenb

May I ask what was the rationale behind adding metadata as a field?

Spontaneously, I think I would prefer that the Api is stateless to prevent errors of inconsistency.

haerdib · 2022-11-24T10:44:31Z

If the metadata is not cached, that means for every extrinsic sent (online), the metadata, runtime version and such need to be queried first. Not sure if that's smart in regard to optimization.

haerdib · 2022-11-24T10:55:41Z

Just checked: The metadata field has been there since 2019, so I guess I can't answer the original rationale, but I suppose the speed is a concern.

clangenb

I also have a remark about the naming, but otherwise it looks good. I totally agree about the metadata caching now. :)

clangenb · 2022-11-24T11:16:56Z

src/api/api_client.rs

+	pub fn update_runtime(mut self) -> ApiResult<Self> {
+		let metadata = Self::get_metadata_from_node(&self.client).map(Metadata::try_from)??;
+		debug!("Metadata: {:?}", metadata);
+
+		let runtime_version = Self::get_runtime_version_from_node(&self.client)?;
+		info!("Runtime Version: {:?}", runtime_version);
+
+		self.metadata = metadata;
+		self.runtime_version = runtime_version;
+		Ok(self)
+	}


I think this should be pub fn update_runtime(&mut self) -> ApiResult<Metadata>. The mut self is rather used for the builder pattern and not the object itself (btw. I think the setSigner method should also be changed).

I don't know if we want to return anything at all, it could also just be (), I don't like the naming 100%, but I can't come up with a better one now.

Agreed. Tried to be consistent, with what's there already, but passing a mut reference is more common.

clangenb · 2022-11-24T11:23:48Z

src/api/api_client.rs


 		match version {
 			Some(v) => serde_json::from_str(&v).map_err(|e| e.into()),
 			None => Err(ApiClientError::RuntimeVersion),
 		}
 	}

-	fn _get_metadata(client: &Client) -> ApiResult<RuntimeMetadataPrefixed> {
+	pub fn get_metadata_from_node(client: &Client) -> ApiResult<RuntimeMetadataPrefixed> {
 		let jsonreq = json_req::state_get_metadata();


I think we are mixing concepts here, get_* sometimes performs a network request now, and sometimes it does not, this inconsistency is confusing.

I suggest get_* always talks to the node, then we don't need to distinguish between query_metadata_from_node to emphasize that it actually does the same as most of the get_* calls.

We don't have to re-invent the wheel here, we can just do:

api.metadata() is a simple getter to the field
api.get_metdata() is the network request, however, do we even want to keep it?

Same for the runtime version.

echevrier

Nice work. Thanks

clangenb

I have a question, but it looks very clean to me otherwise!

src/api/api_client.rs

haerdib self-assigned this Nov 22, 2022

haerdib added F6-optimization This should optimize the performance E2-breaksapi labels Nov 22, 2022

haerdib marked this pull request as ready for review November 22, 2022 14:38

haerdib requested review from clangenb and echevrier November 22, 2022 14:38

haerdib commented Nov 22, 2022

View reviewed changes

node-api/src/metadata.rs Outdated Show resolved Hide resolved

haerdib changed the title ~~Api: Add update_runtime function and small clean up~~ [Api] add update_runtime function and small clean up Nov 22, 2022

haerdib marked this pull request as draft November 22, 2022 15:08

haerdib marked this pull request as ready for review November 22, 2022 15:25

haerdib changed the title ~~[Api] add update_runtime function and small clean up~~ [Api] add update_runtime function and return metadata from memory Nov 22, 2022

echevrier reviewed Nov 22, 2022

View reviewed changes

clangenb reviewed Nov 24, 2022

View reviewed changes

haerdib added the F7-enhancement Enhances an already existing functionality label Nov 28, 2022

haerdib requested review from echevrier and clangenb November 28, 2022 13:29

echevrier approved these changes Nov 28, 2022

View reviewed changes

clangenb approved these changes Nov 29, 2022

View reviewed changes

src/api/api_client.rs Outdated Show resolved Hide resolved

haerdib added 9 commits November 29, 2022 13:38

remove extra _get_request fn

f1a4e0c

add update_runtime function

4440263

readd get_genesis_hash

5b6340f

fix exmaple and metadata

0e64db6

fix doc

a82d6d8

fix no std build

58db985

revert metadata changes

4748c16

add update runtime to example

895d74d

add pub to methods.. why not?

1647160

haerdib added 6 commits November 29, 2022 13:38

clean up and make node calls private

89daebd

fix clippy

1f085dc

update api set fns to use &mut

d2f8269

fix clippy

770b931

fix staking examples

6c4d1ee

add as_ref()

71f88c1

haerdib force-pushed the bh/clean-up-api branch from f6a2852 to 71f88c1 Compare November 29, 2022 12:47

cargo fmt

fb2c3a5

haerdib changed the title ~~[Api] add update_runtime function and return metadata from memory~~ [Api] add update_runtime function and return metadata from cache Nov 29, 2022

haerdib merged commit 566cea3 into master Nov 29, 2022

haerdib deleted the bh/clean-up-api branch November 29, 2022 13:08

haerdib mentioned this pull request Dec 1, 2022

Introduce easy runtime updates (loop?) #348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Api] add `update_runtime` function and return metadata from cache #337

[Api] add `update_runtime` function and return metadata from cache #337

haerdib commented Nov 22, 2022 •

edited

Loading

echevrier left a comment

echevrier Nov 22, 2022

haerdib Nov 23, 2022

echevrier Nov 23, 2022

clangenb Nov 24, 2022

haerdib Nov 24, 2022

clangenb Nov 25, 2022

haerdib Nov 28, 2022

haerdib Nov 28, 2022

clangenb left a comment

haerdib commented Nov 24, 2022 •

edited

Loading

haerdib commented Nov 24, 2022

clangenb left a comment

clangenb Nov 24, 2022 •

edited

Loading

haerdib Nov 24, 2022 •

edited

Loading

haerdib Nov 28, 2022

clangenb Nov 24, 2022

echevrier left a comment

clangenb left a comment

[Api] add update_runtime function and return metadata from cache #337

[Api] add update_runtime function and return metadata from cache #337

Conversation

haerdib commented Nov 22, 2022 • edited Loading

echevrier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

haerdib commented Nov 24, 2022 • edited Loading

haerdib commented Nov 24, 2022

clangenb left a comment

Choose a reason for hiding this comment

clangenb Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

haerdib Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier left a comment

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

[Api] add `update_runtime` function and return metadata from cache #337

[Api] add `update_runtime` function and return metadata from cache #337

haerdib commented Nov 22, 2022 •

edited

Loading

haerdib commented Nov 24, 2022 •

edited

Loading

clangenb Nov 24, 2022 •

edited

Loading

haerdib Nov 24, 2022 •

edited

Loading