Enable `ink_metadata` to be used for metadata generation outside of `ink!` #1357

xermicus · 2022-08-17T10:55:27Z

In order to fix this issue together with this change, we want to enable metadata construction at runtime. In Solang, we'd like to rely on the official ink! crates instead of maintaining our own solution. Additionally, it will be useful for anyone attempting to produce metadata for substrate contracts outside cargo-contract (e.g. when other languages want to add a Substrate target).

So far, this PR:

Adds new constructors where needed
Opens up some builder to be generic over Form
Makes String the default String type

Ideally this should not break any existing API.

crates/metadata/src/specs.rs

crates/metadata/src/tests.rs

…ilders

ascjones · 2022-10-27T15:17:47Z

crates/metadata/src/specs.rs

@@ -397,19 +428,19 @@ pub struct MessageSpec<F: Form = MetaForm> {
    ///
    /// In case of trait provided messages and constructors the prefix
    /// by convention in ink! is the label of the trait.
-    label: F::String,
+    pub label: F::String,


Don't forget to remove all these pub modifiers to require use of builders

almost forgot 🐒 thanks!

#[must_use] is set for the new<T>(ty: T) function, however there is no other means to construct it anyways? There is no builder for ReturnTypeSpec (probably because there is only one fuild anyways). So I just moved the must_use to the ReturnTypeSpec itself

codecov-commenter · 2022-10-27T19:41:04Z

Codecov Report

Merging #1357 (f7556d6) into master (4825d0b) will increase coverage by 19.38%.
The diff coverage is 52.27%.

❗ Current head f7556d6 differs from pull request most recent head c658c46. Consider uploading reports for the commit c658c46 to get more accurate results

@@             Coverage Diff             @@
##           master    #1357       +/-   ##
===========================================
+ Coverage   44.77%   64.16%   +19.38%     
===========================================
  Files         200      200               
  Lines        6101     6111       +10     
===========================================
+ Hits         2732     3921     +1189     
+ Misses       3369     2190     -1179

Impacted Files	Coverage Δ
crates/metadata/src/lib.rs	`0.00% <0.00%> (ø)`
crates/metadata/src/utils.rs	`50.00% <37.50%> (-20.00%)`	⬇️
crates/metadata/src/specs.rs	`69.30% <56.52%> (-4.66%)`	⬇️
crates/metadata/src/layout/mod.rs	`75.63% <100.00%> (ø)`
crates/allocator/src/bump.rs	`86.55% <0.00%> (-0.85%)`	⬇️
crates/e2e/src/lib.rs	`0.00% <0.00%> (ø)`
crates/e2e/src/xts.rs	`0.00% <0.00%> (ø)`
crates/env/src/engine/off_chain/test_api.rs	`64.70% <0.00%> (+5.88%)`	⬆️
crates/ink/ir/src/ir/selector.rs	`80.00% <0.00%> (+8.00%)`	⬆️
... and 33 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

SkymanOne

LGTM

SkymanOne · 2022-10-31T15:07:39Z

crates/metadata/src/utils.rs

-    } else {
-        item.trim_end()
+pub trait DocString {
+    fn trim_extra_whitespace(item: Self) -> Self;


Nit: Add docs with examples

Got rid of it completely 😁

ascjones

Constructors need to be tidied up a bit.

Since this will part of ink 4.0, we are not constrained here by having to make the changes non breaking as we were with scale-info so a bit more flexibility.

ascjones · 2022-10-31T15:54:58Z

crates/metadata/src/specs.rs

@@ -730,7 +800,7 @@ where

 /// The 4 byte selector to identify constructors and messages
 #[derive(Debug, Default, PartialEq, Eq, derive_more::From)]
-pub struct Selector([u8; 4]);
+pub struct Selector(pub [u8; 4]);


Is the pub still needed now with the new constructor?

ascjones · 2022-10-31T15:57:35Z

crates/metadata/src/layout/mod.rs

@@ -511,7 +519,7 @@ impl FieldLayout {
    /// Creates a new field layout.
    pub fn new<N, L>(name: N, layout: L) -> Self
    where
-        N: Into<&'static str>,
+        N: Into<<MetaForm as Form>::String>,


I think we should just use this constructor and make it generic over form, and remove new_custom below

ascjones · 2022-10-31T16:08:57Z

crates/metadata/src/layout/mod.rs

@@ -241,6 +247,10 @@ where
    pub fn ty(&self) -> &F::Type {
        &self.ty
    }
+
+    pub fn new_from_ty(key: LayoutKey, ty: <F as Form>::Type) -> Self {


I think this constructor should become new because it allows setting all fields. The existing new<T> can be renamed to from_key<T> or similar to be consistent.

ascjones · 2022-10-31T16:09:54Z

crates/metadata/src/layout/mod.rs

@@ -525,6 +533,17 @@ impl<F> FieldLayout<F>
 where
    F: Form,
 {
+    /// Creates a new custom field layout.
+    pub fn new_custom<N, L>(name: N, layout: L) -> Self


Remove this in favour of generic new constructor

ascjones · 2022-10-31T16:35:15Z

crates/metadata/src/specs.rs

@@ -942,6 +1024,21 @@ impl<F> EventParamSpec<F>
 where
    F: Form,
 {
+    /// Creates a new event paramater specification builder from a custom type.
+    pub fn new_custom(label: F::String, ty: TypeSpec<F>) -> EventParamSpecBuilder<F> {


EventParamSpecBuilder already has of_type to mutate the type, so you don't need this.

The reason for it was that Type::new() is currently only implemented for MetaForm, but this should be doable

Yes we should open up any idiomatic new constructors to be generic

ascjones · 2022-10-31T16:38:31Z

crates/metadata/src/specs.rs

@@ -1102,6 +1208,15 @@ impl<F> MessageParamSpec<F>
 where
    F: Form,
 {
+    /// Constructs a new message parameter specification via builder.
+    pub fn new_custom(label: F::String, ty: TypeSpec<F>) -> Self {


You can then remove this and use MessageParamSpecBuilder::of_type

ascjones · 2022-10-31T16:44:20Z

crates/metadata/src/specs.rs

@@ -366,21 +393,25 @@ impl<S, P> ConstructorSpecBuilder<S, P> {
    /// Sets the documentation of the message specification.
    pub fn docs<D>(self, docs: D) -> Self
    where
-        D: IntoIterator<Item = &'static str>,
+        D: IntoIterator<Item = <F as Form>::String>,
+        F::String: DocString,


Suggested change

F::String: DocString,

F::String: AsRef<[u8]>,

This might work to avoid requiring the DocString trait and the new impls.

ascjones · 2022-10-31T16:45:18Z

crates/metadata/src/specs.rs

    {
        let mut this = self;
        debug_assert!(this.spec.docs.is_empty());
        this.spec.docs = docs
            .into_iter()
-            .map(trim_extra_whitespace)
+            .map(DocString::trim_extra_whitespace)


Suggested change

.map(DocString::trim_extra_whitespace)

.map(|s| trim_extra_whitespace(s.as_ref()))

Then you should be able to do something like this.

ascjones · 2022-10-31T16:48:04Z

crates/metadata/src/specs.rs

@@ -888,6 +966,10 @@ where
    pub fn display_name(&self) -> &DisplayName<F> {
        &self.display_name
    }
+
+    pub fn new_from_ty(ty: <F as Form>::Type, display_name: DisplayName<F>) -> Self {


Again I would make this the new constructor, and change the existing new<T> to e.g. of_type<T> or maybe with_type<T> as other constructors are called on this.

ascjones · 2022-10-31T16:49:41Z

crates/metadata/src/utils.rs

@@ -58,11 +58,26 @@ where
    deserializer.deserialize_str(Visitor)
 }

-/// Strips a single whitespace at the start and removes trailing spaces
-pub fn trim_extra_whitespace(item: &str) -> &str {


This might be able to be restored and remove the duplicate impls, see usage site above with AsRef<[u8]> bound

The ÀsRef<[u8]>` part is not even needed, was able to nail it using just str refs 👌

xermicus · 2022-11-02T11:33:41Z

@ascjones was able to tidy this up quite a bit and got rid of all the new_customs and new_with_tys etc. I think I'm done implementing all your code review suggestions, thanks! ❤️ Only downside is I had to break the API (just slightly) so I'm gonna wait for the CI, but I think you can glance over the code again already if you feel like it

ascjones · 2022-11-02T11:47:13Z

Only downside is I had to break the API (just slightly)

Breaking the API here is totally fine since this will be part of a major release 4.0 anyway.

ascjones

LGTM just needs CI fixing

ascjones · 2022-11-02T11:53:54Z

LGTM just needs CI fixing

Oh it's a subxt thing, I will look at that now

xermicus · 2022-11-02T11:56:30Z

Yeah right the CI failures seem unrelated to the change. Just got to watch as TypeSpec::new is now TypeSpec::of_type

xermicus · 2022-11-02T12:00:41Z

LGTM just needs CI fixing

Oh it's a subxt thing, I will look at that now

Do you want me to wait with merging?

ascjones reviewed Aug 17, 2022

View reviewed changes

crates/metadata/src/specs.rs Outdated Show resolved Hide resolved

xermicus commented Aug 17, 2022

View reviewed changes

crates/metadata/src/tests.rs Show resolved Hide resolved

xgreenx mentioned this pull request Aug 26, 2022

Storage refactoring new hope for review #1331

Merged

xermicus changed the title ~~switch API to support String~~ Enable ink_metadata to be used for metadata generation outside of ink! Aug 29, 2022

xermicus marked this pull request as ready for review August 29, 2022 16:46

xermicus requested review from cmichi and HCastano as code owners August 29, 2022 16:46

switch API to support string, expose more generic constructors and bu…

98fc658

…ilders

xermicus force-pushed the cl/runtime-metadata branch from cfa5ce9 to 98fc658 Compare September 8, 2022 12:13

this is not needed

29f1740

xermicus mentioned this pull request Sep 13, 2022

Milestone 1: Ensure Solang works with latest Substrate hyperledger-solang/solang#969

Closed

26 tasks

xermicus added 14 commits September 19, 2022 12:39

Merge branch 'master' into cl/runtime-metadata

defcbc1

Merge branch 'master' into cl/runtime-metadata

85ce4f7

patch scale-info dep

c980a89

clippy happy

e44b706

Merge branch 'master' into cl/runtime-metadata

98b43e1

patch scale-info dep to current dev branch

5427347

WIP adapting for changes in scale-info

fec263b

clippy

494d9a3

point to scale-info master

61b82f3

Merge branch 'master' into cl/runtime-metadata

98d093b

more generic root layout constructor

9553e49

Merge branch 'master' into cl/runtime-metadata

5a5f44c

test future version of scale-info

d936040

nigthly fmt

df2baee

ascjones reviewed Oct 27, 2022

View reviewed changes

xermicus added 3 commits October 27, 2022 18:35

make spec fields private again

a4fe8f5

Merge branch 'master' into cl/runtime-metadata

b480e4d

new_custom for ReturnTypeSpec

aae6fa4

xermicus requested review from agryaznov, athei, DoubleOTheven and lean-apple October 31, 2022 13:35

SkymanOne approved these changes Oct 31, 2022

View reviewed changes

ascjones requested changes Oct 31, 2022

View reviewed changes

xermicus added 2 commits November 1, 2022 11:10

tidy up some constructors

dd2f2d7

getting rid of DocString trait

d385e16

xermicus removed request for cmichi, DoubleOTheven, athei, agryaznov, Robbepop, HCastano and lean-apple November 2, 2022 10:45

generic type specs constructors

8ca76e9

ascjones approved these changes Nov 2, 2022

View reviewed changes

xermicus added 2 commits November 2, 2022 13:03

TypeSpec::new renamed to TypeSpec::of_type

bd2f379

Merge branch 'master' into cl/runtime-metadata

c658c46

xermicus merged commit 408a299 into master Nov 2, 2022

xermicus deleted the cl/runtime-metadata branch November 2, 2022 13:52

ascjones mentioned this pull request Nov 3, 2022

patch scale-info dep #1452

Closed

xermicus mentioned this pull request Nov 3, 2022

Add tests for metadata construction at runtime #1464

Merged

ascjones mentioned this pull request Nov 4, 2022

Update scale-info requirement to 2.3 #1467

Merged

ascjones mentioned this pull request Dec 1, 2022

Bump minimum requirement of scale-info in template to 2.3 use-ink/cargo-contract#847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `ink_metadata` to be used for metadata generation outside of `ink!` #1357

Enable `ink_metadata` to be used for metadata generation outside of `ink!` #1357

xermicus commented Aug 17, 2022 •

edited

Loading

ascjones Oct 27, 2022

xermicus Oct 27, 2022

xermicus Oct 27, 2022 •

edited

Loading

codecov-commenter commented Oct 27, 2022 •

edited

Loading

SkymanOne left a comment

SkymanOne Oct 31, 2022

xermicus Nov 2, 2022

ascjones left a comment

ascjones Oct 31, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

xermicus Nov 1, 2022

ascjones Nov 1, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

xermicus Nov 1, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

ascjones Oct 31, 2022

xermicus Nov 2, 2022 •

edited

Loading

xermicus commented Nov 2, 2022 •

edited

Loading

ascjones commented Nov 2, 2022

ascjones left a comment

ascjones commented Nov 2, 2022

xermicus commented Nov 2, 2022 •

edited

Loading

xermicus commented Nov 2, 2022

	.map(DocString::trim_extra_whitespace)
	.map(\|s\| trim_extra_whitespace(s.as_ref()))

Enable ink_metadata to be used for metadata generation outside of ink! #1357

Enable ink_metadata to be used for metadata generation outside of ink! #1357

Conversation

xermicus commented Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 27, 2022 • edited Loading

Codecov Report

SkymanOne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones 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

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

xermicus Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

xermicus commented Nov 2, 2022 • edited Loading

ascjones commented Nov 2, 2022

ascjones left a comment

Choose a reason for hiding this comment

ascjones commented Nov 2, 2022

xermicus commented Nov 2, 2022 • edited Loading

xermicus commented Nov 2, 2022

Enable `ink_metadata` to be used for metadata generation outside of `ink!` #1357

Enable `ink_metadata` to be used for metadata generation outside of `ink!` #1357

xermicus commented Aug 17, 2022 •

edited

Loading

xermicus Oct 27, 2022 •

edited

Loading

codecov-commenter commented Oct 27, 2022 •

edited

Loading

xermicus Nov 2, 2022 •

edited

Loading

xermicus commented Nov 2, 2022 •

edited

Loading

xermicus commented Nov 2, 2022 •

edited

Loading