chore: add sanity tests #512

Ad96el · 2023-04-26T11:03:07Z

fixes KILTProtocol/ticket#312

The sanity tests are executed after each unit test, to validate the pallets state.

One bug could be detected in the staking pallet.

Finding good variable names is difficult: If a sanity test fails, I return DispatchError::Other("Test"). I appreciate suggestions 😆 .

…e into chore/sanity-tests

pallets/attestation/src/lib.rs

pallets/delegation/src/lib.rs

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

pallets/parachain-staking/src/lib.rs

pallets/attestation/src/lib.rs

ntn-x2 · 2023-05-09T13:29:16Z

pallets/delegation/src/lib.rs

@@ -87,7 +87,7 @@ use frame_support::{
 use kilt_support::traits::StorageDepositCollector;
 use parity_scale_codec::Encode;
 use sp_runtime::{traits::Hash, DispatchError};
-use sp_std::{marker::PhantomData, vec::Vec};
+use sp_std::{marker::PhantomData, vec, vec::Vec};


This dependendency is only used in try-runtime, right? In this case it should not be pulled in regardless, and also it could either be used directly with sp_std::vec, or imported inside the function that needs it.

pallets/delegation/src/lib.rs

pallets/pallet-did-lookup/src/lib.rs

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

pallets/parachain-staking/src/lib.rs

pallets/public-credentials/src/lib.rs

ntn-x2

A second round of comments! I love the solution that runs integrity tests after every unit test case. Nevertheless, I would still like to see the following:

Clearer error messages when tests fail, especially (but not only) when inside loops, since otherwise it becomes much harder to find out when the test failed
A separate try_state.rs module for each pallet, with a single pub(crate) try_state() function, which then internally calls several different functions, if needed.

Other than that, this looks like a very very very nice improvement, which will be completed once we have cross-pallet checks in place, especially regarding deposits and locked balances.

pallets/delegation/src/lib.rs

pallets/did/src/lib.rs

ntn-x2 · 2023-05-11T07:16:43Z

pallets/pallet-did-lookup/src/lib.rs

+			ConnectedAccounts::<T>::iter().try_for_each(
+				|(did_identifier, linked_account_id, _)| -> Result<(), &'static str> {
+					let connected_did = ConnectedDids::<T>::get(linked_account_id);
+					ensure!(connected_did.is_some(), "Unknown did");


This can still be simplified by checking against Some(...).

Are you sure? I want to check a property of connected_did.

Doing something like:

connected_did == Some(ConnectionRecord { did: did_identifier, deposit: deposit })

looks to me over-engineered.

I also do not know the deposit – I needed to copy it from the connected_did variable -> I have to unwrap it...

I see, then probably calling .expect(...) on ConnectedDids::::get()` and then matching the DID is a better option, in my opinion. All cases panic anyway.

ntn-x2 · 2023-05-11T07:17:20Z

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

+			// check if for each owner there is a name stored.
+			Owner::<T>::iter().try_for_each(
+				|(w3n, ownership): (Web3NameOf<T>, Web3OwnershipOf<T>)| -> Result<(), &'static str> {
+					ensure!(Names::<T>::contains_key(ownership.owner.clone()), "W3n unknown");


This check seems redundant, given the line below, no?

Well, Names and Owner should have a 1to1 relation. It can happen that a name record is pointing to an owner, which does not exist. The check_claiming_preconditions ensures, that the relation is correct, but I think testing it does not hurt.

Owner and names should have a 1 to 1 relation. It might happen, that an owner is pointing to a name which does not exist. Iterating over only one of these storage, would not reveal these issues.

No I meant that having contains_key and get is redundant, since you are checking for the same thing. But I see that is gone now, so I'll mark this as resolved 👍

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

pallets/attestation/src/lib.rs

ntn-x2 · 2023-05-11T07:23:53Z

pallets/attestation/src/lib.rs

+				if let Some(authorization_id) = attestation_details.authorization_id {
+					ensure!(
+						ExternalAttestations::<T>::get(authorization_id, claim_hash),
+						"Unknown external attestation"


What about including some more information in the error strings? Something like format!("External attestation entry does not exist for attestation with authorization ID {:?}", authorization_id). This will save us a lot of time in case something does actually break in the future. Probably the same could be applied to most of the error messages.

pallets/attestation/src/mock.rs

pallets/public-credentials/src/lib.rs

pallets/parachain-staking/src/lib.rs

Ad96el · 2023-05-12T08:47:03Z

All requested changes should be resolved now.

ntn-x2

Apart from the offline discussion about better errors, I just have a couple more suggestions, then it looks good for me!

Ad96el · 2023-05-15T10:57:56Z

@ntn-x2 I am always happy to see your suggestions!

weichweich

still some minor requests that need changing. I'm not a fan of convert_error_message. Is this the result of a discussion with @ntn-x2?

pallets/delegation/src/try_state.rs

weichweich · 2023-05-15T08:59:16Z

pallets/parachain-staking/src/try_state.rs

+
+			// total stake should be the sum of delegators stake + colator stake.
+			ensure!(
+				sum_delegations.saturating_add(candidate.stake) == candidate.total,


In theory we want to notice a wrap here. While the normal + should panic when it overflows in debug builds, we could use checked_add with an expected(...) to ensure no overflows happen.

support/src/test.rs

pallets/parachain-staking/src/try_state.rs

ntn-x2

Few more suggestions 😄 The bigger ones are about 1. the new utility function to return static strings for testing, 2. better error messages, that also include the failed condition, where it makes sense, so that it's easier to see what failed and for which reason. Other minor points include the way of importing the required types for this new feature.

ntn-x2 · 2023-05-15T13:27:51Z

support/src/test.rs

+
+// If you feel like getting in touch with us, you can do so at [email protected]
+
+use scale_info::prelude::{boxed::Box, string::String};


I think taking them from sp_std would be cleaner, as that's supposed to be the replacement of the standard library in Substrate.

I import the Box struct from sp_std now. I think sp_std has no String struct…

ntn-x2 · 2023-05-15T13:28:56Z

support/src/lib.rs

@@ -20,6 +20,9 @@
 pub mod deposit;
 pub use deposit::{free_deposit, reserve_deposit};

+#[cfg(any(feature = "try-runtime", test))]
+pub mod test;


Not sure this is the best nemaing. This is a testing utility, right? Maybe test_utils or test_utilities would be better? We usually use test for actual tests.

Also the function name itself is not very indicative, and a comment would not hurt about its expected usage.

pallets/public-credentials/src/try_state.rs

ntn-x2 · 2023-05-15T13:47:29Z

pallets/parachain-staking/src/try_state.rs

+			let stake_total = sum_delegations.saturating_add(candidate.stake);
+			ensure!(
+				stake_total == candidate.total,
+				convert_error_message(format!("Total stake of collator {:?} does not match", candidate.id))


Should we add what the expected and returned values are? In general if we are testing a condition, we might want to also want to output, in the case of a failure, what the expected condition was and what was actually returned.

I updated all possible error messages

ntn-x2

Sehr gut!

Ad96el and others added 27 commits April 26, 2023 12:58

first draft

c1906eb

did finished

f392dfa

did finished

b7bbd01

attestation ctype

b8c3f41

delegation

f6e4eee

did-lookup

e676a1a

web3name

3f721be

public credentials

fba90dd

bug

6d65710

comments

0b3c281

public credentials

ca72f47

fmt

8e78c9e

fix: do_try_state

1ff6584

fix test

cd2291f

clippy

866f701

first draft for staking

4a3ef47

first draft for staking

2ecc686

staking pallet

b6538d2

staking pallet

5507a09

fmt

be07f97

fmt

b29ef02

Merge branch 'develop' into chore/sanity-tests

fa15fad

fix wrong test

0ba1364

clippy

0a5dd8c

Merge branch 'chore/sanity-tests' of github.com:KILTprotocol/kilt-nod…

98b079e

…e into chore/sanity-tests

refactor

da193f8

fix test

ecd05bc

Ad96el marked this pull request as ready for review May 8, 2023 13:45

Ad96el requested review from ntn-x2 and weichweich May 8, 2023 13:56

weichweich suggested changes May 9, 2023

View reviewed changes

ntn-x2 reviewed May 9, 2023

View reviewed changes

Ad96el added 4 commits May 9, 2023 17:05

first fixes

0ed0971

refactor

f61632b

3 test are failing

a646f74

refactor

06da8c5

ntn-x2 reviewed May 11, 2023

View reviewed changes

Ad96el added 3 commits May 11, 2023 10:24

neutral lang

33ef40c

refactor files

c4b8d05

refactor

afd1755

Ad96el added 2 commits May 12, 2023 10:50

merge develop into chore/sanity-tests

0719f78

remove debugging flags

0aa08f9

ntn-x2 reviewed May 12, 2023

View reviewed changes

Ad96el added 3 commits May 12, 2023 16:13

draft for error messages

dc5fe26

fmt error messages

5e5ae53

refactor

5662cb0

weichweich suggested changes May 15, 2023

View reviewed changes

ntn-x2 reviewed May 15, 2023

View reviewed changes

Ad96el and others added 5 commits May 16, 2023 09:12

fixed tests

85cb911

refactor error messages

3d49caa

comments

7c4cf6d

comments

3daa939

Merge branch 'develop' into chore/sanity-tests

3c13e2c

weichweich approved these changes May 17, 2023

View reviewed changes

ntn-x2 approved these changes May 17, 2023

View reviewed changes

Ad96el merged commit 18daba5 into develop May 17, 2023

Ad96el deleted the chore/sanity-tests branch May 17, 2023 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add sanity tests #512

chore: add sanity tests #512

Ad96el commented Apr 26, 2023 •

edited

Loading

ntn-x2 May 9, 2023

ntn-x2 left a comment

ntn-x2 May 11, 2023

Ad96el May 11, 2023

Ad96el May 11, 2023

ntn-x2 May 12, 2023

ntn-x2 May 11, 2023

Ad96el May 11, 2023

Ad96el May 12, 2023

ntn-x2 May 12, 2023

ntn-x2 May 11, 2023

Ad96el commented May 12, 2023

ntn-x2 left a comment

Ad96el commented May 15, 2023

weichweich left a comment

weichweich May 15, 2023

ntn-x2 left a comment •

edited

Loading

ntn-x2 May 15, 2023

Ad96el May 16, 2023

ntn-x2 May 15, 2023

ntn-x2 May 15, 2023

ntn-x2 May 15, 2023

Ad96el May 16, 2023

ntn-x2 left a comment


		// If you feel like getting in touch with us, you can do so at [email protected]

		use scale_info::prelude::{boxed::Box, string::String};

chore: add sanity tests #512

chore: add sanity tests #512

Conversation

Ad96el commented Apr 26, 2023 • edited Loading

fixes KILTProtocol/ticket#312

Choose a reason for hiding this comment

ntn-x2 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

Ad96el commented May 12, 2023

ntn-x2 left a comment

Choose a reason for hiding this comment

Ad96el commented May 15, 2023

weichweich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntn-x2 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntn-x2 left a comment

Choose a reason for hiding this comment

Ad96el commented Apr 26, 2023 •

edited

Loading

ntn-x2 left a comment •

edited

Loading