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

BREAKING - Try-runtime: Use proper error types#13993

Merged
paritytech-processbot[bot] merged 38 commits into
paritytech:masterfrom
Szegoo:try-state-error
May 23, 2023
Merged

BREAKING - Try-runtime: Use proper error types#13993
paritytech-processbot[bot] merged 38 commits into
paritytech:masterfrom
Szegoo:try-state-error

Conversation

@Szegoo

@Szegoo Szegoo commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

Updates the return types of:

  • try_state to DispatchResult
  • pre_upgrade to Result<_, DispatchError>
  • post_upgrade to DispatchResult
  • try_on_runtime_upgrade to Result<Weight, DispatchError>

TODO:

  • Check for places where some existing error variants can be used.

This PR introduces a new type inside sp_runtime called TryRuntimeError. This new type is essentially just an alias to the DispatchError type. This only improves the readability of the code.

All of the hooks that now have a different error type should no longer use assert! or similar. The reason for this is that we don't want the code in these macros to panic, instead in case of an error we want to return the TryRuntimeError type.
Since TryRuntimeError is an alias for DispatchError it allows us to use some existing error variants, but in case there isn't an existing variant that could convey the error message &'static str should be used which will get converted to TryRuntimeError during compile time.

Polkadot companion: paritytech/polkadot#7146
Cumulus companion: paritytech/cumulus#2615

kusama address: DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS

Closes: #13736

@Szegoo Szegoo marked this pull request as ready for review April 27, 2023 13:48
@Szegoo Szegoo requested a review from a team April 27, 2023 13:48
@Szegoo Szegoo requested a review from athei as a code owner April 27, 2023 13:48
@Szegoo

Szegoo commented Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

@kianenigma @liamaharon PTAL.

@liamaharon liamaharon added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 27, 2023

@liamaharon liamaharon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Just some errors on cargo c --features try-runtime still to fix it looks like.

Comment thread frame/staking/src/migrations.rs Outdated
Szegoo and others added 3 commits April 28, 2023 11:06
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
@mordamax

Copy link
Copy Markdown
Contributor

@Szegoo yes, it should work now. I fixed len for Lookup and we deployed today. I guess we need to re-tip you in the other PR again )

@mordamax

Copy link
Copy Markdown
Contributor

/tip medium

@substrate-tip-bot

Copy link
Copy Markdown

@mordamax Could not submit tip :( Notify someone here.

@mordamax

Copy link
Copy Markdown
Contributor

/tip medium

@substrate-tip-bot

Copy link
Copy Markdown

@mordamax Could not submit tip :( Notify someone here.

@mordamax

Copy link
Copy Markdown
Contributor

/tip medium

@substrate-tip-bot

Copy link
Copy Markdown

@mordamax Could not submit tip :( Notify someone here.

@mordamax

Copy link
Copy Markdown
Contributor

/tip medium

@substrate-tip-bot

Copy link
Copy Markdown

@mordamax A medium tip was successfully submitted for @Szegoo (DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda tip

@mordamax

Copy link
Copy Markdown
Contributor

So now looks like all good. I will send tips for previous wasted proposals with 0 len

image

@liamaharon

Copy link
Copy Markdown
Contributor

Thanks @mordamax !

@pgherveou

Copy link
Copy Markdown
Contributor

Hey @Szegoo did you use a script to rewrite all the changes ?
I am asking as I need to fix some conflict, in one of our pending PR

@Szegoo

Szegoo commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

@pgherveou I used basic vscode search and replace for pre_upgrade. For the other ones, I used a regex in vscode to search and replace all of them.

You can do something like this:

Search pattern: fn try_on_runtime_upgrade\((.*?)\) -> Result<Weight, &'static str>
Replacement: fn try_on_runtime_upgrade($1) -> Result<Weight, TryRuntimeError>

@pgherveou

Copy link
Copy Markdown
Contributor

Ty ended up doing the same, was not too much trouble

@pgherveou

pgherveou commented May 26, 2023

Copy link
Copy Markdown
Contributor

You also updated all the assert_eq -> ensure right in this pre/post upgrade methods

@Szegoo

Szegoo commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

You also updated all the assert_eq -> ensure right

Yes, that also needs to be updated.

gpestana pushed a commit that referenced this pull request May 27, 2023
* Try-state: DispatchResult as return type

* try_state for the rest of the pallets

* pre_upgrade

* post_upgrade

* try_runtime_upgrade

* fixes

* bags-list fix

* fix

* update test

* warning fix

* ...

* final fixes 🤞

* warning..

* frame-support

* warnings

* Update frame/staking/src/migrations.rs

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* fix

* fix warning

* nit fix

* merge fixes

* small fix

* should be good now

* missed these ones

* introduce TryRuntimeError and TryRuntimeResult

* fixes

* fix

* removed TryRuntimeResult & made some fixes

* fix testsg

* tests passing

* unnecessary imports

* Update frame/assets/src/migration.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
@Szegoo

Szegoo commented May 28, 2023

Copy link
Copy Markdown
Contributor Author

@mordamax From my understanding by looking at this: https://kusama.subsquare.io/referenda/referendum/205 the proposal is approved and executed. But it seems like the tokens didn't get transferred from the treasury 🤔
The same goes for my other tip.

@mordamax

Copy link
Copy Markdown
Contributor

@Szegoo thank you for pinging me back!
@bkchr helped to find a problem and here's paritytech/substrate-tip-bot#74 issue to fix it.
We'll follow-up with both tips again after fix 🙏
cc @rzadp

@rzadp

rzadp commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

/tip medium

@substrate-tip-bot

Copy link
Copy Markdown

@rzadp A medium (5 KSM) tip was successfully submitted for @Szegoo (DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda tip

Ank4n pushed a commit that referenced this pull request Jul 8, 2023
* Try-state: DispatchResult as return type

* try_state for the rest of the pallets

* pre_upgrade

* post_upgrade

* try_runtime_upgrade

* fixes

* bags-list fix

* fix

* update test

* warning fix

* ...

* final fixes 🤞

* warning..

* frame-support

* warnings

* Update frame/staking/src/migrations.rs

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* fix

* fix warning

* nit fix

* merge fixes

* small fix

* should be good now

* missed these ones

* introduce TryRuntimeError and TryRuntimeResult

* fixes

* fix

* removed TryRuntimeResult & made some fixes

* fix testsg

* tests passing

* unnecessary imports

* Update frame/assets/src/migration.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Try-state: DispatchResult as return type

* try_state for the rest of the pallets

* pre_upgrade

* post_upgrade

* try_runtime_upgrade

* fixes

* bags-list fix

* fix

* update test

* warning fix

* ...

* final fixes 🤞

* warning..

* frame-support

* warnings

* Update frame/staking/src/migrations.rs

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* fix

* fix warning

* nit fix

* merge fixes

* small fix

* should be good now

* missed these ones

* introduce TryRuntimeError and TryRuntimeResult

* fixes

* fix

* removed TryRuntimeResult & made some fixes

* fix testsg

* tests passing

* unnecessary imports

* Update frame/assets/src/migration.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

try-runtime proper error types

9 participants