Custom runtime module errors #3433

bkchr · 2019-08-18T16:25:34Z

This pr enables runtime modules to declare a custom error type by using decl_error!.

Each module error can be converted to a DispatchError, this error carries the module index, the error index and an optional error message. Using the module index and the error index, we should be able to support much nicer error messages in the UI and as a RPC result.

Later, we could expose the decl_error! declaration via the metadata to show the actual error message.

CC @xlc @jacogr

…ponse

Co-Authored-By: Tomasz Drwięga <[email protected]>

…ponse

Co-Authored-By: Gavin Wood <[email protected]>

gavofyork

Some suggestions that would be nice to see happen

Co-Authored-By: Gavin Wood <[email protected]>

…-handling

bkchr · 2019-09-03T07:50:24Z

@gavofyork applied your feedback.

core/client/src/block_builder/block_builder.rs

node/executor/src/lib.rs

gavofyork

A few minor suggestions.

Demi-Marie · 2019-08-31T00:13:20Z

core/sr-primitives/src/lib.rs

+	/// General error to do with the permissions of the sender.
+	NoPermission,
+
+	/// General error to do with the state of the system in general.


@gavofyork Should CorruptedState be added?

core/sr-primitives/src/lib.rs

Demi-Marie · 2019-08-31T00:54:44Z

core/sr-primitives/src/lib.rs

+
+impl runtime_io::Printable for DispatchError {
+	fn print(&self) {
+		"DispatchError".print();


Yep, but we print numbers, strings etc and we don't support formatting from the runtime. So we only can do this call by call.

Can we implement formatting manually within the runtime? The runtime does support heap allocation.

Demi-Marie · 2019-09-01T21:36:39Z

core/sr-primitives/src/traits.rs

+	) -> Result<Self::Pre, crate::ApplyError> {
+		self.validate(who, call, info, len)
+			.into_result()
+			.map(|_| Self::Pre::default())


Why do we throw away the error information here? At a minimum it should be logged.

Demi-Marie · 2019-09-01T21:36:43Z

core/sr-primitives/src/traits.rs

+	) -> Result<Self::Pre, crate::ApplyError> {
+		Self::validate_unsigned(call, info, len)
+			.into_result()
+			.map(|_| Self::Pre::default())


Demi-Marie · 2019-09-03T20:05:49Z

core/sr-primitives/src/transaction_validity.rs

+				"Could not lookup some information to validate the transaction",
+			UnknownTransaction::NoUnsignedValidator =>
+				"Could not find an unsigned validator for the unsigned transaction",
+			UnknownTransaction::Custom(_) => "UnknownTransaction custom error",


This should at least include the error number. Otherwise, debugging is almost impossible.

Demi-Marie · 2019-09-03T20:06:57Z

core/test-runtime/src/system.rs

 	block.extrinsics.iter().enumerate().for_each(|(i, e)| {
 		storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &(i as u32));
-		execute_transaction_backend(e).unwrap_or_else(|_| panic!("Invalid transaction"));
+		let _ = execute_transaction_backend(e).unwrap_or_else(|_| panic!("Invalid transaction"));


Suggested change

let _ = execute_transaction_backend(e).unwrap_or_else(|_| panic!("Invalid transaction"));

let _ = execute_transaction_backend(e).unwrap();

We do not want to lose the error message here.

Demi-Marie · 2019-09-03T20:07:55Z

core/transaction-pool/src/tests.rs

-			longevity: 64,
-			propagate: true,
-		}))
+		Ok(


I think this is a spurious linebreak.

Demi-Marie · 2019-09-03T20:09:51Z

srml/contracts/src/gas.rs

 	transactor: &T::AccountId,
 	gas_limit: Gas,
 ) -> Result<(GasMeter<T>, NegativeImbalanceOf<T>), &'static str> {
-	// Check if the specified amount of gas is available in the current block.


Why is this no longer useful?

Demi-Marie · 2019-09-03T23:05:12Z

srml/contracts/src/lib.rs

+					Ok(ValidTransaction::default())
+				}
+			},
+			Call::__PhantomItem(_, _)  => unreachable!("Variant is never constructed"),


This should be enforced at compile-time. We can add an empty enum to this variant, for example.

Demi-Marie · 2019-09-04T00:46:29Z

srml/contracts/src/lib.rs

+					Ok(ValidTransaction::default())
+				}
+			},
+			Call::__PhantomItem(_, _)  => unreachable!("Variant is never constructed"),


This should be caught at compile-time.

Co-Authored-By: Gavin Wood <[email protected]> Co-Authored-By: DemiMarie-parity <[email protected]>

…-handling

core/sr-primitives/src/generic/checked_extrinsic.rs

Done

shawntabrizi · 2019-09-04T14:24:04Z

Epic!

bedeho · 2019-09-12T08:44:18Z

Looking forward to using this!

* srml-system checks * wip * more modules compiles * node-runtime checks * build.sh passes * include dispatch error in failed event * revert some unnecessary changes * refactor based on comments * more compile error fixes * avoid unnecessary into * reorder code * fixes some tests * manually implement encode & decode to avoid i8 workaround * more test fixes * more fixes * more error fixes * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <[email protected]> * address comments * test for DispatchError encoding * tyep alias for democracy * make error printable * line width * fix balances tests * fix executive test * fix system tests * bump version * ensure consistent method signature * Apply suggestions from code review Co-Authored-By: Gavin Wood <[email protected]> * changes based on review * Add issue number for TODOs * fix * line width * fix test * Update core/sr-primitives/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update core/sr-primitives/src/traits.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update srml/council/src/motions.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update srml/council/src/motions.rs Co-Authored-By: Bastian Köcher <[email protected]> * update based on review * More concrete macro matching * fix test build issue * Update hex-literal dependency version. (paritytech#3141) * Update hex-literal dep version. * Update lock file. * Start to rework the new error handling * More work to get it back compiling * Start to fix after master merge * The great transaction error handling refactoring * Make `decl_error` errors convertible to `&'static str` * Make srml-executive build again * Fix `sr-primitives` tests * More fixes * Last round of fix ups * Fix build * Fix build * Apply suggestions from code review Co-Authored-By: Tomasz Drwięga <[email protected]> * Rename some stuff * Fixes after master merge * Adds `CheckBlockGasLimit` signed extension * Remove debug stuff * Fix srml-balances test * Rename `InvalidIndex` to `CannotLookup` * Remove weird generic parameters * Rename function again * Fix import * Document the signed extension * Change from `Into` to `From` * Update srml/contracts/src/lib.rs Co-Authored-By: Sergei Pepyakin <[email protected]> * Fix compilation * Update srml/contracts/src/lib.rs Co-Authored-By: Tomasz Drwięga <[email protected]> * Update core/sr-primitives/src/transaction_validity.rs Co-Authored-By: Tomasz Drwięga <[email protected]> * Remove unused code * Fix compilation * Some cleanups * Fix compile errors * Make `TransactionValidity` a `Result` * Apply suggestions from code review Co-Authored-By: Gavin Wood <[email protected]> * Beautify the code a little bit and fix test * Make `CannotLookup` an inherent error declared by `decl_error!` * Adds some documentation * Make `ApplyOutcome` a result * Up the spec_version * Apply suggestions from code review Co-Authored-By: Gavin Wood <[email protected]> Co-Authored-By: DemiMarie-parity <[email protected]>

xlc and others added 30 commits June 19, 2019 14:24

srml-system checks

ba48a0a

wip

52c6b90

more modules compiles

bd7228f

node-runtime checks

c0cece1

build.sh passes

a93fedd

include dispatch error in failed event

3039e7c

revert some unnecessary changes

c5c2e5a

refactor based on comments

1198fb7

Merge remote-tracking branch 'upstream/master' into improve-error-res…

ed20244

…ponse

more compile error fixes

1f964d1

avoid unnecessary into

83975c0

reorder code

e03db0c

fixes some tests

7e01497

manually implement encode & decode to avoid i8 workaround

a6c989b

more test fixes

9d2dbbb

more fixes

31c9c84

more error fixes

6cad33a

Apply suggestions from code review

ba42e6a

Co-Authored-By: Tomasz Drwięga <[email protected]>

address comments

0f01cc4

Merge remote-tracking branch 'upstream/master' into improve-error-res…

cce7c08

…ponse

test for DispatchError encoding

c7ae1b1

tyep alias for democracy

0e424c1

make error printable

a145f27

line width

ef4eecd

fix balances tests

33668d8

fix executive test

d128a1a

fix system tests

a4a8cdc

bump version

f4387fb

ensure consistent method signature

ef2eaa6

Apply suggestions from code review

31c998f

Co-Authored-By: Gavin Wood <[email protected]>

gavofyork reviewed Sep 2, 2019

View reviewed changes

bkchr and others added 2 commits September 2, 2019 21:20

Apply suggestions from code review

91b0869

Co-Authored-By: Gavin Wood <[email protected]>

Beautify the code a little bit and fix test

86ef4eb

tomusdrw approved these changes Sep 2, 2019

View reviewed changes

bkchr added 4 commits September 3, 2019 08:13

Make CannotLookup an inherent error declared by decl_error!

354e05f

Adds some documentation

6743719

Make ApplyOutcome a result

b9fc817

Merge remote-tracking branch 'origin/master' into bkchr-runtime-error…

d5dc034

…-handling

Up the spec_version

e1bd019

paritytech-ci added the B2-breaksapi label Sep 3, 2019

gavofyork reviewed Sep 3, 2019

View reviewed changes

core/client/src/block_builder/block_builder.rs Outdated Show resolved Hide resolved

gavofyork reviewed Sep 3, 2019

View reviewed changes

node/executor/src/lib.rs Outdated Show resolved Hide resolved

gavofyork reviewed Sep 3, 2019

View reviewed changes

node/executor/src/lib.rs Outdated Show resolved Hide resolved

gavofyork previously requested changes Sep 3, 2019

View reviewed changes

Demi-Marie reviewed Sep 3, 2019

View reviewed changes

Demi-Marie reviewed Sep 4, 2019

View reviewed changes

bkchr and others added 2 commits September 4, 2019 09:27

Apply suggestions from code review

e0fa417

Co-Authored-By: Gavin Wood <[email protected]> Co-Authored-By: DemiMarie-parity <[email protected]>

Merge remote-tracking branch 'origin/master' into bkchr-runtime-error…

c25ac83

…-handling

kianenigma reviewed Sep 4, 2019

View reviewed changes

core/sr-primitives/src/generic/checked_extrinsic.rs Show resolved Hide resolved

bkchr merged commit 5420de3 into master Sep 4, 2019

bkchr deleted the bkchr-runtime-error-handling branch September 4, 2019 14:21

This was referenced Oct 18, 2019

Propagate extrinsic errors over JSON-RPC for locally submitted transactions #2246

Closed

improve RPC error message "Invalid Transaction" #2485

Closed

xlc mentioned this pull request Nov 7, 2019

construct_runtime macro needs to generate runtime Error type #4040

Closed

shawntabrizi mentioned this pull request Dec 11, 2019

Update Frame Pallets to use decl_error! #4365

Closed

36 tasks

	let _ = execute_transaction_backend(e).unwrap_or_else(\|_\| panic!("Invalid transaction"));
	let _ = execute_transaction_backend(e).unwrap();

Custom runtime module errors #3433

Custom runtime module errors #3433

Uh oh!

Conversation

bkchr commented Aug 18, 2019

Uh oh!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Sep 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shawntabrizi commented Sep 4, 2019

Uh oh!

bedeho commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants