Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Runtime API versioning #11779

Merged
merged 37 commits into from
Aug 13, 2022
Merged

Runtime API versioning #11779

merged 37 commits into from
Aug 13, 2022

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Jul 5, 2022

Related to issue #11577

Add support for multiple versions of a Runtime API. The purpose is to have one main version of the API, which is considered stable and multiple extensions (also referred as versioned methods) to it. Effectively from substrate point of view these are just different versions of the same API. It's up to the implementor to add semantics.

How it works

Some methods of the API trait can be tagged with #[api_version(N)] attribute where N is version number bigger than the main one. Let's call them versioned methods for brevity. Then the implementor of the API decides which version to implement.

Example (from #11577 (comment)):

decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[api_version(11)]
         fn new_cool_function() -> u32;
    }
}
impl_runtime_apis! {
    #[api_version(11)]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }

         fn new_cool_function() -> u32 {
             10
         }
    }
}

Version safety checks

By default in the API trait all staging methods has got a default implementation calling unimplemented!(). This is a problem because if the developer wants to implement version 11 in the example above and forgets to add fn new_cool_function() in impl_runtime_apis! the runtime will crash when the function is executed.

To overcome this decl_runtime_apis! generates multiple dummy traits for each version it finds in the declaration. These traits contain all required methods and have no default implementations. Then when the developer implements a specific version a dummy implementation of a versioned trait (besides the actual one) is generated. If the developer has forgotten to add a method - a compilation error will be generated.

TODOs

  • Version safety check
  • Better version generation - decl_runtime_apis! should generate an array with all valid versions. impl_runtime_apis! should return one of these versions instead of what's in its attribute so that a nonexistent version can't be implemented by mistake.
  • Integration tests of primitives/api are messed up a bit. More specifically primitives/api/test/tests/decl_and_impl.rs
  • Integration UI test covering the new functionality.
  • Some duplicated code

primitives/api/proc-macro/src/attribute_names.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/test/tests/decl_and_impl.rs Show resolved Hide resolved
@tdimitrov tdimitrov added A3-in_progress Pull request is in progress. No review needed at this stage. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jul 5, 2022
@tdimitrov tdimitrov marked this pull request as ready for review July 12, 2022 13:59
@tdimitrov tdimitrov added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Jul 12, 2022
@rphmeier
Copy link
Contributor

Please add some documentation with examples to the crate

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looking really good. It ended much better and easier than I had imagined, nice!

I added some comments on stuff that still needs to improved.

Ty :)

primitives/api/proc-macro/src/attribute_names.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/utils.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/utils.rs Outdated Show resolved Hide resolved
primitives/api/src/lib.rs Outdated Show resolved Hide resolved
primitives/api/src/lib.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
@tdimitrov tdimitrov force-pushed the staging_api branch 4 times, most recently from f9c97f6 to f5c0a38 Compare August 4, 2022 06:48
@tdimitrov tdimitrov requested a review from bkchr August 4, 2022 14:12
tdimitrov and others added 8 commits August 8, 2022 10:12
Related to issue paritytech#11577

Add support for multiple versions of a Runtime API. The purpose is to
have one main version of the API, which is considered stable and
multiple unstable (aka staging) ones.

How it works
===========
Some methods of the API trait can be tagged with `#[api_version(N)]`
attribute where N is version number bigger than the main one. Let's call
them **staging methods** for brevity.

The implementor of the API decides which version to implement.

Example (from paritytech#11577 (comment)):

```
decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[api_version(11)]
         fn new_cool_function() -> u32;
    }
}
```

```
impl_runtime_apis! {
    #[api_version(11)]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }

         fn new_cool_function() -> u32 {
             10
         }
    }
}
```

Version safety checks (currently not implemented)
=================================================
By default in the API trait all staging methods has got default
implementation calling `unimplemented!()`. This is a problem because if
the developer wants to implement version 11 in the example above and
forgets to add `fn new_cool_function()` in `impl_runtime_apis!` the
runtime will crash when the function is executed.

Ideally a compilation error should be generated in such cases.

TODOs
=====

Things not working well at the moment:
[ ] Version safety check
[ ] Integration tests of `primitives/api` are messed up a bit. More
specifically `primitives/api/test/tests/decl_and_impl.rs`
[ ] Integration test covering the new functionality.
[ ] Some duplicated code
Code review feedback and formatting

Co-authored-by: asynchronous rob <rphmeier@gmail.com>
Applying suggestions from @bkchr
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looking really good! Just some last nitpicks

primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
@tdimitrov tdimitrov requested a review from bkchr August 11, 2022 20:37
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some last nitpicks, otherwise looks nice.

primitives/api/proc-macro/src/utils.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
Comment on lines 234 to 236
decl.items = decl
.items
.iter_mut()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
decl.items = decl
.items
.iter_mut()
decl
.items
.iter()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iter_mut() here should stay. The method is modified in the for_each().

primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/decl_runtime_apis.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Aug 12, 2022

Ty for all the hard work @tdimitrov :) (And especially going through all of this with me 😅)

tdimitrov and others added 2 commits August 12, 2022 23:02
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@tdimitrov
Copy link
Contributor Author

Thanks for the kind words, @bkchr

(And especially going through all of this with me sweat_smile)

On the contrary - it was fun and I learned a lot!

@bkchr bkchr merged commit c1293d4 into paritytech:master Aug 13, 2022
@tdimitrov tdimitrov deleted the staging_api branch August 13, 2022 11:59
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Runtime API versioning

Related to issue paritytech#11577

Add support for multiple versions of a Runtime API. The purpose is to
have one main version of the API, which is considered stable and
multiple unstable (aka staging) ones.

How it works
===========
Some methods of the API trait can be tagged with `#[api_version(N)]`
attribute where N is version number bigger than the main one. Let's call
them **staging methods** for brevity.

The implementor of the API decides which version to implement.

Example (from paritytech#11577 (comment)):

```
decl_runtime_apis! {
    #{api_version(10)]
    trait Test {
         fn something() -> Vec<u8>;
         #[api_version(11)]
         fn new_cool_function() -> u32;
    }
}
```

```
impl_runtime_apis! {
    #[api_version(11)]
    impl Test for Runtime {
         fn something() -> Vec<u8> { vec![1, 2, 3] }

         fn new_cool_function() -> u32 {
             10
         }
    }
}
```

Version safety checks (currently not implemented)
=================================================
By default in the API trait all staging methods has got default
implementation calling `unimplemented!()`. This is a problem because if
the developer wants to implement version 11 in the example above and
forgets to add `fn new_cool_function()` in `impl_runtime_apis!` the
runtime will crash when the function is executed.

Ideally a compilation error should be generated in such cases.

TODOs
=====

Things not working well at the moment:
[ ] Version safety check
[ ] Integration tests of `primitives/api` are messed up a bit. More
specifically `primitives/api/test/tests/decl_and_impl.rs`
[ ] Integration test covering the new functionality.
[ ] Some duplicated code

* Update primitives/api/proc-macro/src/impl_runtime_apis.rs

Code review feedback and formatting

Co-authored-by: asynchronous rob <rphmeier@gmail.com>

* Code review feedback

Applying suggestions from @bkchr

* fmt

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Code review feedback

* dummy trait -> versioned trait

* Implement only versioned traits (not compiling)

* Remove native API calls (still not compiling)

* fmt

* Fix compilation

* Comments

* Remove unused code

* Remove native runtime tests

* Remove unused code

* Fix UI tests

* Code review feedback

* Code review feedback

* attribute_names -> common

* Rework `append_api_version`

* Code review feedback

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Code review feedback

* Code review feedback

* Code review feedback

* Use type alias for the default trait - doesn't compile

* Fixes

* Better error for `method_api_ver < trait_api_version`

* fmt

* Rework how we call runtime functions

* Update UI tests

* Fix warnings

* Fix doctests

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Fix formatting and small compilation errors

* Update primitives/api/proc-macro/src/impl_runtime_apis.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: asynchronous rob <rphmeier@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants