Add proper test Custom values #1147

tadeohepperle · 2023-08-29T13:56:50Z

I added a test that uses some generated metadata to get custom values via an OfflineClient:

// static query:
let foo_address = node::custom().foo();
let foo = api.custom_values().at(&foo_address)?;
assert_eq!(foo, expected_foo);

// dynamic query:
let foo_value = api.custom_values().at("Foo")?;
let foo: Foo = foo_value.as_type()?;
assert_eq!(foo, expected_foo);

lexnv · 2023-08-29T15:38:19Z

testing/generate-custom-metadata/src/lib.rs

@@ -1,15 +1,14 @@
-// Copyright 2019-2023 Parity Technologies (UK) Ltd.


nit: I think we could keep the copyright here (everywhere else)

codegen/src/api/custom_values.rs

jsdw · 2023-08-30T09:07:29Z

testing/generate-custom-metadata/src/main.rs

+fn main() {
+    let metadata_prefixed = generate_custom_metadata::metadata_custom_values_foo();
+    let bytes = metadata_prefixed.encode();
+    std::fs::write("./artifacts/metadata_with_custom_values.scale", bytes)


Can we add a call to this to /scripts/artifacts.sh (ie cargo run --bin generate-custom-metadata > artifacts/metadata_with_custom_values.scale), so that running that script updates all of our artifacts :)

(also, let's either allow the caller to decide where to write the file to, or just write the file to stdout, so that the file location can be specified in artifacts.sh so it's clear from that script what is being generated)

Oh yes, that is a great idea!

jsdw · 2023-08-30T09:28:28Z

testing/generate-custom-metadata/src/lib.rs

@@ -48,7 +54,7 @@ pub fn metadata_custom_values_foo() -> RuntimeMetadataPrefixed {
    let unit_ty = registry.register_type(&meta_type::<()>());

    // Metadata needs to contain this DispatchError, since codegen looks for it.
-    registry.register_type(&meta_type::<crate::utils::dispatch_error::ArrayDispatchError>());
+    registry.register_type(&meta_type::<dispatch_error::ArrayDispatchError>());

    let metadata = RuntimeMetadataV15 {


Below, we add some CustomMetadata with a valid Foo value inside it (sorry; I can't comment on the exact line).

Could we also add a custom value with a random type ID that isn't in use (eg u32::MAX); people might opt to do this too and we want to be sure that we can still access the bytes and that nothing will panic (just Err) if we try decoding them.

In that case we just test if the bytes are returned from the dynamic interface right? E.g. we do not generate any static custom value address. Or should we generate a static address that has a return type of Vec<u8>?

Since we've decided that you can't call .at() on a value with an invalid type ID, perhaps we can test that such an attempt fails with a compile error (via trybuild), but that .at_bytes() works, sicne that's what we've decided to allow.

jsdw · 2023-08-30T09:32:27Z

Looks good; just a couple of small comments! I'm not super keen on the dispatch error types being exported from the new generate-custom-values crate (it's a bit of a random place for them), but I'm not too bothered for now (I think ultimately a test-utils crate to dump various helper code like this is probably the way to go ultimately, so feel free to do that at some point if you run out of things :))

…rface

jsdw · 2023-09-11T14:18:57Z

codegen/src/types/mod.rs

+    pub fn types(&self) -> &PortableRegistry {
+        self.type_registry
+    }


Nit: Instead of exposing this in this TypeGenerator, let's just pass the PortableRegistry into the function instead (it's accessible as something like self.metadata.types() in the parent function). Not a big deal though!

jsdw · 2023-09-11T14:19:14Z

metadata/src/utils/validation.rs

-        &hash(custom_value.bytes()),
-    )
+    let name_hash = hash(custom_value.name.as_bytes());
+    let bytes_hash =hash(custom_value.bytes());


Suggested change

let bytes_hash =hash(custom_value.bytes());

let bytes_hash = hash(custom_value.bytes());

jsdw · 2023-09-11T14:24:11Z

metadata/src/utils/validation.rs

-        &hash(custom_value.bytes()),
-    )
+    let name_hash = hash(custom_value.name.as_bytes());
+    let bytes_hash =hash(custom_value.bytes());


The goal of this validation stuff is to make sure that the code generated from some metadata is still compatible with some node (and the node metadata) so that it can still be used without issue.

With that in mind, we don't actually need to check that custom_value.bytes() is identical; it doesn't matter if the bytes are different so long as they still decode OK into the generated type that the codegen is expecting. So let's remove the hashing of bytes. Otherwise looks good offhand (though this stuff always needs some thought and I obv missed this last time :))

jsdw · 2023-09-11T14:25:11Z

subxt/src/custom_values/custom_value_address.rs

@@ -10,6 +10,9 @@ use crate::metadata::DecodeWithMetadata;
 pub trait CustomValueAddress {
    /// The type of the custom value.
    type Target: DecodeWithMetadata;
+    /// Should be set to `Yes` for Dynamic values and static values that have a valid type.
+    /// Should be `()` for custom values, that have an invalid type id.
+    type Decodable;


Nice! For consistency with storage, how about IsDecodable?

jsdw · 2023-09-11T14:26:56Z

subxt/src/custom_values/custom_values_client.rs

@@ -26,7 +26,7 @@ impl<T, Client> CustomValuesClient<T, Client> {
 impl<T: Config, Client: OfflineClientT<T>> CustomValuesClient<T, Client> {
    /// Access a custom value by the address it is registered under. This can be just a [str] to get back a dynamic value,
    /// or a static address from the generated static interface to get a value of a static type returned.
-    pub fn at<Address: CustomValueAddress + ?Sized>(
+    pub fn at<Address: CustomValueAddress<Decodable =Yes> + ?Sized>(


Suggested change

pub fn at<Address: CustomValueAddress<Decodable =Yes> + ?Sized>(

pub fn at<Address: CustomValueAddress<Decodable = Yes> + ?Sized>(

subxt/src/custom_values/custom_values_client.rs

jsdw · 2023-09-11T14:29:12Z

subxt/src/custom_values/custom_values_client.rs

+            .ok_or_else(|| MetadataError::CustomValueNameNotFound(address.name().to_string()))?;
+
+        Ok(custom_value.bytes().to_vec())
+    }


Looks good! Shame about needing to to_vec(), but I can see it's because the metadata is cloned and not referenced; maybe something we can address elsewhere at some point :)

jsdw · 2023-09-11T14:32:40Z

testing/ui-tests/Cargo.toml

 scale-info = { workspace = true, features = ["bit-vec"] }
 frame-metadata ={ workspace = true }
 codec = { package = "parity-scale-codec", workspace = true, features = ["derive", "bit-vec"] }
-subxt = { workspace = true, features = ["native", "jsonrpsee"] }
+subxt = { workspace = true, features = ["native", "jsonrpsee", "substrate-compat"] }


Where is substrate-compat needed? Offhand I can't see any use for it but I'm likely to be missing something :)

jsdw · 2023-09-11T14:37:08Z

Looking good, and I like the new bytes_at() approach for valiues without valid type IDs!

I left a few comments which are mostly just little things, but with these small bits looked at I'd be happy to see this merge:

Decodable -> IsDecodable
Don't hash the bytes in the validation check (name and type should suffice)
Confirm whether substrate-compat feature is needed in the subxt dev-dep
Check on the test failure; I'm not quite sure what that's about so would be good to understand it :)

tadeohepperle · 2023-09-12T07:39:56Z

Confirm whether substrate-compat feature is needed in the subxt dev-dep

You are right, it is not necessary, I changed an import to config::substrate::H256 and now it works without it. I found the cause of the test failure (older custom value metadata) and now it should be okay.

Co-authored-by: James Wilson <james@jsdw.me>

jsdw

The changes look great to me; nice one!

lexnv · 2023-09-12T11:59:20Z

scripts/artifacts.sh

 cargo run --bin subxt metadata --file artifacts/polkadot_metadata_full.scale --pallets "" > artifacts/polkadot_metadata_tiny.scale
+# generate a metadata file that only contains some custom metadata
+cargo run --bin generate-custom-metadata > artifacts/metadata_with_custom_values.scale 


nit: Could use an extra line at the end of the script

lexnv · 2023-09-12T12:02:40Z

testing/ui-tests/Cargo.toml

 scale-info = { workspace = true, features = ["bit-vec"] }
 frame-metadata ={ workspace = true }
 codec = { package = "parity-scale-codec", workspace = true, features = ["derive", "bit-vec"] }
 subxt = { workspace = true, features = ["native", "jsonrpsee"] }
 subxt-metadata = { workspace = true }
+generate-custom-metadata = { path = "../generate-custom-metadata" }


nit: extra line

lexnv

LGTM! Nice one!

lexnv · 2023-09-12T12:31:04Z

scripts/artifacts.sh

 cargo run --bin subxt metadata --file artifacts/polkadot_metadata_full.scale --pallets "" > artifacts/polkadot_metadata_tiny.scale
+# generate a metadata file that only contains some custom metadata
+cargo run --bin generate-custom-metadata > artifacts/metadata_with_custom_values.scale 


Not related to this PR, just to not forget about it, would be a good practice to exit on failures:
set -e such we don't move forward if some prior step fails

tadeohepperle added 3 commits August 29, 2023 15:50

add proper tests for custom values

cae28c3

Merge branch 'master' into tadeo-hepperle-custom-values-add-proper-test

03ed1a3

remove try operators

e7971c2

tadeohepperle requested a review from a team as a code owner August 29, 2023 13:56

lexnv reviewed Aug 29, 2023

View reviewed changes

use sustrate compat for import of hash

7e56e61

lexnv reviewed Aug 29, 2023

View reviewed changes

codegen/src/api/custom_values.rs Show resolved Hide resolved

add license and hex

d87a969

jsdw reviewed Aug 30, 2023

View reviewed changes

add script to artifacts.sh

ccc125d

jsdw reviewed Aug 30, 2023

View reviewed changes

tadeohepperle added 4 commits September 5, 2023 17:23

custom values with ids not in metadata can be accessed in static inte…

30b47b8

…rface

fmt and clippy

10ff27f

access bytes of custom values directly, even if type id wrong

3578bd7

Merge branch 'master' into tadeo-hepperle-custom-values-add-proper-test

08864a4

jsdw reviewed Sep 11, 2023

View reviewed changes

subxt/src/custom_values/custom_values_client.rs Outdated Show resolved Hide resolved

jsdw reviewed Sep 11, 2023

View reviewed changes

tadeohepperle added 3 commits September 11, 2023 18:25

final fixes

da5d91b

Merge branch 'master' into tadeo-hepperle-custom-values-add-proper-test

7830ba8

removing substrate-compat flag from ui tests

d4728ad

Update subxt/src/custom_values/custom_values_client.rs

c2330f1

Co-authored-by: James Wilson <james@jsdw.me>

remove types access in type generator

f558dd5

jsdw approved these changes Sep 12, 2023

View reviewed changes

lexnv reviewed Sep 12, 2023

View reviewed changes

lexnv approved these changes Sep 12, 2023

View reviewed changes

2 extra lines

d13af85

lexnv reviewed Sep 12, 2023

View reviewed changes

tadeohepperle merged commit c8462de into master Sep 12, 2023

tadeohepperle deleted the tadeo-hepperle-custom-values-add-proper-test branch September 12, 2023 13:46

jsdw mentioned this pull request Sep 27, 2023

Prep to release 0.32.0 #1178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proper test Custom values #1147

Add proper test Custom values #1147

tadeohepperle commented Aug 29, 2023

lexnv Aug 29, 2023

jsdw Aug 30, 2023 •

edited

Loading

tadeohepperle Aug 30, 2023

jsdw Aug 30, 2023 •

edited

Loading

tadeohepperle Aug 30, 2023 •

edited

Loading

jsdw Sep 11, 2023

jsdw commented Aug 30, 2023 •

edited

Loading

jsdw Sep 11, 2023

jsdw Sep 11, 2023

jsdw Sep 11, 2023

jsdw Sep 11, 2023 •

edited

Loading

jsdw Sep 11, 2023

jsdw Sep 11, 2023

jsdw Sep 11, 2023

jsdw commented Sep 11, 2023

tadeohepperle commented Sep 12, 2023

jsdw left a comment

lexnv Sep 12, 2023

lexnv Sep 12, 2023

lexnv left a comment

lexnv Sep 12, 2023

		@@ -1,15 +1,14 @@
		// Copyright 2019-2023 Parity Technologies (UK) Ltd.

	let bytes_hash =hash(custom_value.bytes());
	let bytes_hash = hash(custom_value.bytes());

	pub fn at<Address: CustomValueAddress<Decodable =Yes> + ?Sized>(
	pub fn at<Address: CustomValueAddress<Decodable = Yes> + ?Sized>(

Add proper test Custom values #1147

Add proper test Custom values #1147

Conversation

tadeohepperle commented Aug 29, 2023

Choose a reason for hiding this comment

jsdw Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

tadeohepperle Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Sep 11, 2023 • 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

jsdw commented Sep 11, 2023

tadeohepperle commented Sep 12, 2023

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Aug 30, 2023 •

edited

Loading

jsdw Aug 30, 2023 •

edited

Loading

tadeohepperle Aug 30, 2023 •

edited

Loading

jsdw commented Aug 30, 2023 •

edited

Loading

jsdw Sep 11, 2023 •

edited

Loading