Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134

pepyakin · 2019-11-18T21:47:24Z

This PR features:

Removing unused variants from ApplyError, preserving the enumeration (not sure about that, should we just use TransactionValidity directly?),
Renaming the ApplyResult and ApplyOutcome to (hopefully) less confusing ones.
Adding documentation that (again hopefuly) further reduces confusion.

This change doesn't require version bump, although it changes the external API. Therefore here is the accompanying Polkadot PR.

I left the enumeration though since other elements might be used some day.

bkchr · 2019-11-18T22:05:10Z

(This breaks the block authoring of Kusama)

pepyakin · 2019-11-18T22:20:54Z

Can you shed some light on why is that?

bkchr · 2019-11-19T07:35:58Z

We can not decode ApplyResult/InclusionOutput anymore. It is solvable, but will require a little bit more effort.
We need to add a compatibility layer as shown here: #3585. This would require that all validators update before we send out the runtime update to the network. (we should increment the authoring version as well)

bkchr

In general looking good.

primitives/block-builder/runtime-api/src/lib.rs

primitives/sr-primitives/src/lib.rs

kianenigma · 2019-11-19T09:22:19Z

primitives/sr-primitives/src/lib.rs

+/// supplies the result of the extrinsic dispatch.
+///
+/// Examples of reasons preventing inclusion in a block:
+/// - More block weight is required to process the extrinsic than is left in the block being built.


I wouldn't name weight here and just say block resources. Weight is Palette specific while I suppose these error types are.

Ahhh, that's a good point. I am mistakingly assumed that we are in the pallete context.

kianenigma · 2019-11-19T09:33:32Z

primitives/sr-primitives/src/lib.rs

+///   included in the next block if it has enough spare weight available.
+/// - The sender doesn't have enough funds to pay the transaction inclusion fee. Including such
+///   a transaction in the block doesn't make sense.
+/// - The extrinsic supplied a bad signature. This transaction won't become valid ever.


I would make it very clear that a dispatch has two phases 1- pre-dispatch stuff and 2- the dispatch itself.

A signature check, and any other code that we might put in Executive::apply_extrinsic_xxx is in the first group (.check). For instance a signature check gives you Err(InclusionError)

A weight check, nonce and other things that happen either in signedExtensions or dispatch code itself are in the second group. I think a wrong nonce gives you Ok(DispatchError) (while an all okay dispatch gives Ok(())).

So I would make this distinction very clear in the docs.

(take the above with a grain of slat and double check, I didn't look super deep into the code.)

Based on this, without any explanation, I find it confusing that signature check and weight check are enumerated next to each other, as it seems that they are natively different types of checks.

Yeah, I would redirect the reader from this documentation to how apply/dispatch + SignedExtensions work in general to get more details into the lifecycle of extrinsic.

Yeah, they are different checks in practice, but we are dealing with the non-pallete context, from the block builder API perspective, from that PoV they are essentially the same, isn't it This also relates to referencing the user to the documentation of SignedExtensions and co.

tomusdrw

Strictly better than what we have now, but still a lot of room for improvement imho. But I'm happy to merge asap, cause otherwise we may end up with never ending story of doc improvements through the entire codebase.

tomusdrw · 2019-11-19T09:55:40Z

client/block-builder/src/lib.rs

@@ -43,7 +43,7 @@ use sr_api::{Core, ApiExt, ApiErrorFor};
 pub use runtime_api::BlockBuilder as BlockBuilderApi;

 /// Error when the runtime failed to apply an extrinsic.
-pub struct ApplyExtrinsicFailed(pub sr_primitives::ApplyError);
+pub struct ApplyExtrinsicFailed(pub sr_primitives::InclusionError);


Should be renamed as well?

My logic was that I'd leave this as is to denote that apply_extrinsic failed because of the following reason which is represented by InclusionError.

I thought maybe it would be better to name apply_extrinsic as include_extrinsic, but that is getting out of hand...

That said, I am not strong on it. Give me a sign (thumbs up will do) and I will make the change

paint/executive/src/lib.rs

primitives/block-builder/runtime-api/src/lib.rs

primitives/sr-primitives/src/lib.rs

tomusdrw · 2019-11-19T10:06:07Z

primitives/sr-primitives/src/lib.rs

+///   included in the next block if it has enough spare weight available.
+/// - The sender doesn't have enough funds to pay the transaction inclusion fee. Including such
+///   a transaction in the block doesn't make sense.
+/// - The extrinsic supplied a bad signature. This transaction won't become valid ever.


Yeah, I would redirect the reader from this documentation to how apply/dispatch + SignedExtensions work in general to get more details into the lifecycle of extrinsic.

primitives/sr-primitives/src/lib.rs

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

should be → is

pepyakin · 2019-11-19T17:08:29Z

So I guess if we are more or less happy with this refactoring, as the next step I'd like to get some directions on how to cleanly integrate this PR into polkadot/kusama.

Apart from updating the accompanying PR with updating the authoring version there, what else should we do here? Can you guide me on this one, @bkchr ?

gavofyork · 2019-11-22T12:21:13Z

@pepyakin re: compatibility with kusama, it's more about just ensuring that UI people are given appropriate warning.

conflicts need sorting, but otherwise should be good.

# Conflicts: # bin/node/executor/src/lib.rs # palette/executive/src/lib.rs # palette/system/src/lib.rs # primitives/sr-primitives/src/testing.rs # primitives/sr-primitives/src/traits.rs

client/block-builder/src/lib.rs

bkchr

Besides the whitespaces, it looks good.

pepyakin added 5 commits November 18, 2019 20:19

Remove superflous errors from the system module

7572b98

Rename and document InclusionOutcome

8e4a5e2

Rename InclusionError

a62d4f3

Remove unused inclusion errors.

b8c7ec1

I left the enumeration though since other elements might be used some day.

Rename and document DispatchOutcome

14bdeb5

pepyakin added A0-please_review Pull request needs code review. B2-breaksapi labels Nov 18, 2019

pepyakin requested review from kianenigma and gui1117 as code owners November 18, 2019 21:47

pepyakin mentioned this pull request Nov 18, 2019

Update to the latest of Block Builder API paritytech/polkadot#596

Closed

pepyakin requested review from bkchr and tomusdrw November 18, 2019 21:58

bkchr reviewed Nov 19, 2019

View reviewed changes

primitives/block-builder/runtime-api/src/lib.rs Outdated Show resolved Hide resolved

primitives/sr-primitives/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Nov 19, 2019

View reviewed changes

tomusdrw approved these changes Nov 19, 2019

View reviewed changes

Apply suggestions from code review

d8ee205

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

pepyakin added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Nov 19, 2019

TransactionValidityError instead of InclusionError

3dcbb48

lamafab mentioned this pull request Nov 19, 2019

Spec requested runtime entries w3f/polkadot-spec#80

Merged

pepyakin added 3 commits November 19, 2019 14:24

Rename InclusionOutcome to ApplyExtrinsicResult

56e530c

Update docs.

7e04dc6

Update lib.rs

0459481

should be → is

pepyakin added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 19, 2019

Bump the block builder API version.

c53659b

pepyakin force-pushed the ser-apply-clean branch from a54ced2 to c53659b Compare November 20, 2019 16:26

pepyakin added 2 commits November 20, 2019 20:27

Fix the should_return_runtime_version test

d916bc4

Merge remote-tracking branch 'origin/master' into ser-apply-clean

d034332

gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Nov 22, 2019

gavofyork approved these changes Nov 22, 2019

View reviewed changes

gavofyork added the A3-needsresolving label Nov 22, 2019

Merge 'origin/master' into ser-apply-clean

89cd7f4

# Conflicts: # bin/node/executor/src/lib.rs # palette/executive/src/lib.rs # palette/system/src/lib.rs # primitives/sr-primitives/src/testing.rs # primitives/sr-primitives/src/traits.rs

bkchr reviewed Nov 22, 2019

View reviewed changes

client/block-builder/src/lib.rs Outdated Show resolved Hide resolved

bkchr approved these changes Nov 22, 2019

View reviewed changes

Clean the evidence

7c40059

pepyakin merged commit d88fff5 into master Nov 22, 2019

pepyakin deleted the ser-apply-clean branch November 22, 2019 16:16

jacogr mentioned this pull request Nov 23, 2019

Add DispatchInfo polkadot-js/api#1601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134

Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134

pepyakin commented Nov 18, 2019 •

edited

Loading

bkchr commented Nov 18, 2019

pepyakin commented Nov 18, 2019

bkchr commented Nov 19, 2019

bkchr left a comment

kianenigma Nov 19, 2019

pepyakin Nov 19, 2019

kianenigma Nov 19, 2019 •

edited

Loading

kianenigma Nov 19, 2019

tomusdrw Nov 19, 2019

pepyakin Nov 19, 2019

tomusdrw left a comment

tomusdrw Nov 19, 2019

pepyakin Nov 19, 2019

pepyakin Nov 19, 2019

tomusdrw Nov 19, 2019

pepyakin commented Nov 19, 2019 •

edited

Loading

gavofyork commented Nov 22, 2019

bkchr left a comment

Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134

Renaming and documentation for ApplyResult, ApplyOutcome and et al #4134

Conversation

pepyakin commented Nov 18, 2019 • edited Loading

bkchr commented Nov 18, 2019

pepyakin commented Nov 18, 2019

bkchr commented Nov 19, 2019

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Nov 19, 2019 • edited Loading

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

tomusdrw 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

pepyakin commented Nov 19, 2019 • edited Loading

gavofyork commented Nov 22, 2019

bkchr left a comment

Choose a reason for hiding this comment

pepyakin commented Nov 18, 2019 •

edited

Loading

kianenigma Nov 19, 2019 •

edited

Loading

pepyakin commented Nov 19, 2019 •

edited

Loading