Simplify feature-flag and use common types #62

lexnv · 2023-06-14T11:45:44Z

This PR makes the following changes to the repo, in preparation for the V15 stabilization:

Feature Flags

The version feature flags are now removed and replaced by:

legacy this feature flag enables only the versions prior to V14
current this feature flag enables the latest stabilized metadata versions
unstable this feature flag enables the unstable metadata that is currently in work until stabilization

Common Types

The V15 and V14 shared a few common types. With the introduction of the new feature flags, the V15 metadata directly uses the common types exposed by V14.
This saves some extra lines of code from maintenance and allows easier conversions between V14 and V15 (from subxt and substrate)

// @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw

I'm a bit wary of having a common module like this, because it's only really for types common between v14 and v15 (and over time I could see it becoming a bit of a mess of types that are in common between certain versions (and the feature flags would become more meaningless as more "common" types are always included, which might not matter).

My original thought was that the v15 module could just import and pub use the types it wanted from v14 (and then eg v16 could do the same with v15, v17 with v16 and so on).

The feature flags complicate this though, and as we add more versions and have more common types shared between further-apart versions it'll get more annoying if we want to hdie and show the relevant types for a given set of feature flags, when they are shared.

I'm not quite sure what the utility of the feature flags really is either (aside from the "unstable-v15" one which is good becasue it hides an unstable part of the interface). @ascjones do you have more insight into what the feature flags are here for?

If we can ditch the feature flags (aside from any "unstable" ones while needed) it'd make life easy and we can just import from v14 any types we want to also expose as v15 types, and do the same when v16 lands etc.

If not, I'd probably tweak the common module so that each type is feature flagged to make clear which versions it's common for (and generally honour the feature flags hiding unnecessary stuff). At least that way if it becomes messier it'll always be clear which types are used where.

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv · 2023-06-15T12:00:03Z

Have moved the feature flags on struct declaration and impls from the common file to unblock this.

Looking forwards, it would be much better from a maintenance perspective to remove the version feature flags (v14, v15 etc). The new versions V14 and V15 bring with them the scale-info dependency, which would be a downside for users that want to decode just older versions.

Considering that this is an edge-case that could be later mitigated by having one single legacy feature-flag for older metadata only, I suggest we remove the version feature flags. I may not have all the details here, @ascjones what do you think? 🙏

ascjones

I'm happy for the version feature flags to go, I can't remember the reasoning...I think it is historical artefact from when I hijacked this repository after it was forked from the in tree frame-metadata crate.

jsdw · 2023-06-16T09:57:42Z

I'm happy for the version feature flags to go, I can't remember the reasoning...I think it is historical artefact from when I hijacked this repository after it was forked from the in tree frame-metadata crate.

Thanks for the feedback!

Id propose in that case that we could have the following flags:

legacy: all of the pre-v14 stuff
current: a default flag. pulls in scale-info and enables all recent versions (currently just v14 but soon v14 and v15). (open to name ideas here!)
unstable: to cover whatever currently unstable metadata version we're working on.
serde_full, std and decode as they are.

And then we can ditch the current feature flags and ditch this common.rs file, instead just having v15.rs pull what it wants from v14.rs (I feel like that'll be a clearer approach going forwards).

I guess all of this will be a breaking change anyway when we stabilise v15 in a couple of weeks so now is as good a time as any to break the features?!

This reverts commit 5988d8a. Revert "Guard common types by feature flags" This reverts commit f12cdc6. Revert "Use common types" This reverts commit a012d1c. Revert "Add common types between v14 and v15" This reverts commit 718c9d3.

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2023-06-20T11:18:26Z

The changes look great, and yeah feels simpler now too with the new feature flags.

I guess I'm ok with us marking V15 as stable in this PR, which is the only contentious change here; it does mean we won't want to release this until we are happy to call V15 stable and done, but hopefully we're about there now!

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2023-06-21T10:04:23Z

frame-metadata/src/lib.rs

 pub mod v14;

 /// Metadata v15
-#[cfg(feature = "v15-unstable")]
+#[cfg(all(feature = "unstable", feature = "current"))]


Should it be the other way round? ie if you enable the "unstable" feature you get v14 and v15 (since some types are shared). And if you enable "current" you just get v14 atm?

Currently this means you need to enable "current" and "unstable" to get unstable metadata.

Oh yea! It makes more sense to have the unstable feature enable the current than to need to specify them both, thanks! Have modified it

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2023-06-21T11:51:16Z

Looks good to me :)

lexnv added 2 commits June 14, 2023 14:21

Add common types between v14 and v15

718c9d3

Signed-off-by: Alexandru Vasile <[email protected]>

Use common types

a012d1c

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv self-assigned this Jun 14, 2023

jsdw reviewed Jun 14, 2023

View reviewed changes

lexnv added 2 commits June 15, 2023 14:48

Guard common types by feature flags

f12cdc6

Signed-off-by: Alexandru Vasile <[email protected]>

Use feature flags on the imports

5988d8a

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw approved these changes Jun 15, 2023

View reviewed changes

ascjones approved these changes Jun 16, 2023

View reviewed changes

lexnv added 4 commits June 20, 2023 13:16

Revert "Use feature flags on the imports"

2f2e18d

This reverts commit 5988d8a. Revert "Guard common types by feature flags" This reverts commit f12cdc6. Revert "Use common types" This reverts commit a012d1c. Revert "Add common types between v14 and v15" This reverts commit 718c9d3.

Adjust feature flags

0332ef5

Signed-off-by: Alexandru Vasile <[email protected]>

Use v14 types for v15

01841ce

Signed-off-by: Alexandru Vasile <[email protected]>

Rename module_error_ty to be consisten with event/call

da74cb0

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv changed the title ~~Add common types between v14 and v15~~ Simplify feature-flag and use common types Jun 20, 2023

lexnv mentioned this pull request Jun 20, 2023

v15: Remove the module_error_ty #63

Closed

Mark V15 as unstable

8c7591d

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 approved these changes Jun 20, 2023

View reviewed changes

jsdw reviewed Jun 21, 2023

View reviewed changes

Adjust feature flags

3b39f1f

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv merged commit fc36bec into main Jun 27, 2023

lexnv deleted the lexnv/common_types branch June 27, 2023 13:11

OverOrion mentioned this pull request Jul 6, 2023

Dependencies: frame-metadata features change integritee-network/worker#1388

Open

haerdib mentioned this pull request Jul 6, 2023

Update dependencies and frame-metadata feature flags scs/substrate-api-client#608

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify feature-flag and use common types #62

Simplify feature-flag and use common types #62

lexnv commented Jun 14, 2023 •

edited

Loading

jsdw left a comment

lexnv commented Jun 15, 2023

ascjones left a comment

jsdw commented Jun 16, 2023 •

edited

Loading

jsdw commented Jun 20, 2023

jsdw Jun 21, 2023 •

edited

Loading

lexnv Jun 21, 2023

jsdw commented Jun 21, 2023

Simplify feature-flag and use common types #62

Simplify feature-flag and use common types #62

Conversation

lexnv commented Jun 14, 2023 • edited Loading

Feature Flags

Common Types

jsdw left a comment

Choose a reason for hiding this comment

lexnv commented Jun 15, 2023

ascjones left a comment

Choose a reason for hiding this comment

jsdw commented Jun 16, 2023 • edited Loading

jsdw commented Jun 20, 2023

jsdw Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

lexnv Jun 21, 2023

Choose a reason for hiding this comment

jsdw commented Jun 21, 2023

lexnv commented Jun 14, 2023 •

edited

Loading

jsdw commented Jun 16, 2023 •

edited

Loading

jsdw Jun 21, 2023 •

edited

Loading