feat: harmonize dispatch error names #441

trusch · 2022-12-08T13:39:34Z

fixes KILTProtocol/ticket#2290

This harmonizes the instance names in the Error enums of our pallets.

Before this change the names were kind of randomly choosen, so we had for example CTypeNotFound and DidNotPresent. Those names were bad for thwo reasons:

They mean the same thing but use different wording
They contain the module name

After this change both instances would be named NotFound.

All the other changes are something along those lines.

How to test:

Please provide a brief step-by-step instruction.
If necessary provide information about dependencies (specific configuration, branches, database dumps, etc.)

Step 1
Step 2
etc.

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)

wischli

More harmonization required. For a final statement, the document with all runtime existing KILT runtime errors is mandatory (assuming post PR state) as its too easy to oversee stuff.

pallets/pallet-web3-names/src/lib.rs

pallets/did/src/lib.rs

… AlreadyExists, regardless of the pallet

weichweich

Can you adjust the PR description? You can remove the parts of the template that don't apply? Also there should be a doc with a description of the naming and conventions? Maybe you could commit that here as a md file?

weichweich · 2023-01-09T07:16:22Z

pallets/did/src/errors.rs

 	}
 }

 /// Error involving the pallet's storage.
 #[derive(Debug, Eq, PartialEq, TypeInfo)]
-pub enum StorageError {
+pub enum Storage {


What's the reason to remove the Error suffix here? If I see an enum called Storage I'm not expected error codes.

Its part of the errors module, so its qualified name is did::errors::Storage which is beautiful. With the suffix it would be did::errors::StorageError.

did::errors::StorageError would only be used once to bring StorageError into scope. After that it's always only StorageError. So your proposal is errors::Storage vs StorageError. I personally think StorageError is better since you can't forget to have the error path. If you don't use errors::Storage but Storage this becomes very misleading.

weichweich · 2023-01-09T07:23:54Z

pallets/did/src/errors.rs

 	/// The given DID does not contain the right key to verify the signature
 	/// of a DID operation.
-	DidKeyNotPresent(DidVerificationKeyRelationship),
+	DidKeyNotFound(DidVerificationKeyRelationship),


We have NotFound, KeyNotFound and DidKeyNotFound. They all sound very similar.

In case of DidKeyNotFound, we can't execute the call since the DID doesn't have the required VerificationKeyRelationship? I would interpret that as disallowing the call and rename it to CallNotAllowed or similar.
In case of NotFound and KeyNotFound I would keep DidNotFound as it reflects that this is specific for the Did. Best would probably be to have a NotFound(Resource) error, where the Resource is an enum that contains all the resources that could be missing.

I think now its completely refactored to be in that NotFound(Resource) shape

weichweich · 2023-01-09T07:24:45Z

pallets/did/src/errors.rs

@@ -84,7 +84,7 @@ pub enum SignatureError {
 /// Error generated when some extrinsic input does not respect the pallet's
 /// constraints.
 #[derive(Debug, Eq, PartialEq, TypeInfo)]
-pub enum InputError {
+pub enum Input {


Name Input doesn't reflect that it's error codes.

same as above

weichweich · 2023-01-09T07:24:59Z

pallets/did/src/errors.rs

 }

 /// Error generated when validating a DID operation.
 #[derive(Debug, Eq, PartialEq, TypeInfo)]
-pub enum SignatureError {
+pub enum Signature {


Name Signature doesn't reflect that it's error codes

Same as with the name of did::errors::Storage, the module indicates that its all about error types

weichweich · 2023-01-09T07:25:46Z

pallets/did/src/errors.rs

 }

 /// Error generated when validating a DID operation.
 #[derive(Debug, Eq, PartialEq, TypeInfo)]
-pub enum SignatureError {
+pub enum Signature {
 	/// The signature is not in the format the verification key expects.
 	InvalidSignatureFormat,


Since we know it's a signature error, we don't require the Signature in the error name? Same with InvalidSignature even though we might want to explicitly say that it's the signature part that is wrong and not the nonce part (we also have InvalidNonce).

Suggested change

InvalidSignatureFormat,

InvalidFormat,

I removed the Signature part from the variants and changed from InvalidSignature case to InvalidData

weichweich · 2023-01-09T07:33:57Z

pallets/did/src/service_endpoints.rs

@@ -59,48 +59,45 @@ pub struct DidEndpoint<T: Config> {
 impl<T: Config> DidEndpoint<T> {
 	/// Validates a given [DidEndpoint] instance against the constraint
 	/// set in the pallet's [Config].
-	pub(crate) fn validate_against_constraints(&self) -> Result<(), InputError> {
+	pub(crate) fn validate_against_constraints(&self) -> Result<(), Input> {


If we would use the errors::Input path everywhere I would think that this change would make more sense. Seeing only Input there makes you think that the input is returned in case of an error.

Suggested change

pub(crate) fn validate_against_constraints(&self) -> Result<(), Input> {

pub(crate) fn validate_against_constraints(&self) -> Result<(), errors::Input> {

pallets/pallet-web3-names/src/lib.rs

weichweich · 2023-01-11T10:28:12Z

pallets/did/src/errors.rs

 	/// See [StorageError].
-	StorageError(StorageError),
+	Storage(Storage),
 	/// See [SignatureError].
-	SignatureError(SignatureError),
+	Signature(Signature),
 	/// See [InputError].
-	InputError(InputError),
+	Input(Input),


The comments need updating

weichweich

@ntn-x2 What do you think about StorageError vs errors::Storage? Which one do you prefer?

weichweich · 2023-01-13T09:54:18Z

pallets/did/src/errors.rs

-	SignatureError(SignatureError),
-	/// See [InputError].
-	InputError(InputError),
+pub enum Error {


Following the "no error suffixrule, this would beDidand notError`?

That's the top-level Error type for DIDs, so calling this Error in the context of the did::error module kind of makes sense. I agree that this also violates the "no error suffix" rule since technically "Error" is a suffix of "Error", but this is really an edgecase. Naming it "Did" could make sense, but it feels counter-intuitive to me. Perhaps we should modify the no-error suffix rule somehow?

weichweich · 2023-01-13T09:56:49Z

pallets/did/src/did_details.rs

+			.delegation_key
+			.take()
+			.ok_or(errors::Storage::NotFound(errors::NotFoundKind::Key(
+				errors::KeyType::AssertionMethod,


from the name of the function this should be the delegation key?

weichweich · 2023-01-13T10:17:39Z

pallets/did/src/errors.rs

+
+/// Enum describing the different did key types.
+#[derive(Debug, Eq, PartialEq, TypeInfo)]
+pub enum KeyType {


Even tho this introduces another nesting level i would maybe reuse the DidVerificationKeyRelationship here.

pub enum KeyType { Verification(DidVerificationKeyRelationship), KeyAgreement, }

You can than implement From<DidVerificationKeyRelationship> for KeyType or maybe even derive that and From<KeyType> for NotFoundKind

When creating an error gets too deeply nested you can also add a helper function to StorageError.

ntn-x2 · 2023-01-13T10:24:26Z

@ntn-x2 What do you think about StorageError vs errors::Storage? Which one do you prefer?

My two cents are that types can also be imported directly in Rust. This means that I could do use did::errors::Storage, and then use Storage directly. This could be confusing. That's why I would leave error names with the Error prefix, so that even by fully importing the type and not its enclosing module, we don't have any ambiguities.

trusch · 2023-02-09T14:04:43Z

I reverted the removal of the Error suffix for the error types in the did::errors module.

ntn-x2 · 2023-02-10T10:53:29Z

@trusch please fix the CI errors, thanks!

weichweich · 2023-02-22T15:24:25Z

pallet_error_naming_convention.md

+
+3) Avoid using the pallet name in the variant names. For example instead of "Web3NameNotFound" you should use "NotFound". 
+
+4) Try to take words from the vocabulary already defined in the code base. For example instead of introducing a new variant "NotExisting" you should use, once again, "NotFound"


could you list the vocabulary here?
e.g. NotAuthorized, NotFound, AlreadyExists, MaxXYZExceeded (thats all? i guess...)

## fixes KILTprotocol/ticket#2290 This harmonizes the instance names in the Error enums of our pallets. Before this change the names were kind of randomly choosen, so we had for example CTypeNotFound and DidNotPresent. Those names were bad for thwo reasons: 1) They mean the same thing but use different wording 2) They contain the module name After this change both instances would be named NotFound. All the other changes are something along those lines. Co-authored-by: Albrecht <albrecht@kilt.io>

trusch requested a review from weichweich December 8, 2022 13:39

wischli suggested changes Dec 12, 2022

View reviewed changes

pallets/pallet-web3-names/src/lib.rs Show resolved Hide resolved

pallets/pallet-web3-names/src/lib.rs Show resolved Hide resolved

pallets/did/src/lib.rs Show resolved Hide resolved

pallets/did/src/lib.rs Outdated Show resolved Hide resolved

trusch added 4 commits January 5, 2023 10:49

harmonize dispatch error names, so its always NotFound for example or…

eaa9cf7

… AlreadyExists, regardless of the pallet

cargo fmt

e6eb59c

make clippy happy with the tests

b274b12

refactored DID errors + one forgoten Unauthorized in attestation pallet.

788191c

trusch force-pushed the feature/harmonize-dispatch-errors branch from 2edb846 to 788191c Compare January 5, 2023 09:49

trusch added 2 commits January 5, 2023 11:15

cargo fmt

d487944

cargo clippy --fix

7df38bb

trusch marked this pull request as ready for review January 5, 2023 12:10

weichweich suggested changes Jan 9, 2023

View reviewed changes

qualify errors::Input in service_endpoints code

d9eb328

weichweich reviewed Jan 11, 2023

View reviewed changes

trusch added 8 commits January 12, 2023 10:22

unify did related not-found errors.

8188d89

cargo fmt

26922f2

update comments of did::errors::Error variants.

881abe8

further unify not found errors

aa341c1

cargo fmt

3fa7430

apply clippy suggestion

c7d99be

cargo fmt

77d674a

Merge branch 'develop' into feature/harmonize-dispatch-errors

33185e0

trusch requested a review from weichweich January 13, 2023 08:27

weichweich reviewed Jan 13, 2023

View reviewed changes

trusch added 3 commits February 9, 2023 13:27

Merge branch 'develop' into feature/harmonize-dispatch-errors

ce7fd6c

bring back the Error suffix in the did::errors module

d8af561

Merge branch 'develop' into feature/harmonize-dispatch-errors

4ff8e68

ntn-x2 self-requested a review February 10, 2023 10:51

trusch added 2 commits February 13, 2023 11:20

fix test

4b3cc82

add pallet error naning convention doc

4981ba4

weichweich reviewed Feb 22, 2023

View reviewed changes

trusch and others added 4 commits February 23, 2023 13:25

Merge branch 'develop' into feature/harmonize-dispatch-errors

563f4e8

cargo fmt

d55cdb0

add examples for common vocab

3b558d3

Merge branch 'develop' into feature/harmonize-dispatch-errors

66e5f8b

weichweich approved these changes Feb 28, 2023

View reviewed changes

trusch enabled auto-merge (squash) February 28, 2023 08:45

trusch merged commit c0da775 into develop Feb 28, 2023

trusch deleted the feature/harmonize-dispatch-errors branch February 28, 2023 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: harmonize dispatch error names #441

feat: harmonize dispatch error names #441

trusch commented Dec 8, 2022 •

edited by weichweich

Loading

wischli left a comment

weichweich left a comment

weichweich Jan 9, 2023

trusch Jan 9, 2023

weichweich Jan 13, 2023

weichweich Jan 9, 2023

trusch Jan 13, 2023

weichweich Jan 9, 2023

trusch Jan 13, 2023

weichweich Jan 9, 2023

trusch Jan 13, 2023

weichweich Jan 9, 2023

trusch Jan 13, 2023

weichweich Jan 9, 2023

trusch Jan 13, 2023

weichweich Jan 11, 2023

trusch Jan 13, 2023

weichweich left a comment

weichweich Jan 13, 2023

trusch Jan 20, 2023

weichweich Jan 13, 2023

weichweich Jan 13, 2023

ntn-x2 commented Jan 13, 2023

trusch commented Feb 9, 2023

ntn-x2 commented Feb 10, 2023

weichweich Feb 22, 2023

	pub(crate) fn validate_against_constraints(&self) -> Result<(), Input> {
	pub(crate) fn validate_against_constraints(&self) -> Result<(), errors::Input> {


		3) Avoid using the pallet name in the variant names. For example instead of "Web3NameNotFound" you should use "NotFound".

		4) Try to take words from the vocabulary already defined in the code base. For example instead of introducing a new variant "NotExisting" you should use, once again, "NotFound"

feat: harmonize dispatch error names #441

feat: harmonize dispatch error names #441

Conversation

trusch commented Dec 8, 2022 • edited by weichweich Loading

fixes KILTProtocol/ticket#2290

How to test:

Checklist:

wischli left a comment

Choose a reason for hiding this comment

weichweich 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

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

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

Choose a reason for hiding this comment

weichweich 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

ntn-x2 commented Jan 13, 2023

trusch commented Feb 9, 2023

ntn-x2 commented Feb 10, 2023

Choose a reason for hiding this comment

trusch commented Dec 8, 2022 •

edited by weichweich

Loading