fixes and chores

nodes/common/src/public_credentials.rs

@@ -24,22 +24,6 @@ use public_credentials::CredentialEntry;
 use public_credentials_rpc::PublicCredentialsFilter;
 
 #[derive(Serialize, Deserialize)]
-#[serde(bound(
-	serialize = "
-	CTypeHash: Serialize,
-	Attester: Serialize,
-	BlockNumber: Serialize,
-	AccountId: Serialize,
-	Balance: std::fmt::Display,
-	AuthorizationId: Serialize",
-	deserialize = "
-	CTypeHash: Deserialize<'de>,
-	Attester: Deserialize<'de>,
-	BlockNumber: Deserialize<'de>,
-	AccountId: Deserialize<'de>,
-	Balance:  std::str::FromStr,
-	AuthorizationId: Deserialize<'de>"
-))]
 /// Thin wrapper around a runtime credential entry as specified in the
 /// `public-credentials` pallet. This wrapper implements all the
 /// (de-)serialization logic.
@@ -48,6 +32,10 @@ pub struct OuterCredentialEntry<CTypeHash, Attester, BlockNumber, AccountId, Bal
 	pub attester: Attester,
 	pub revoked: bool,
 	pub block_number: BlockNumber,
+	#[serde(bound(
+		serialize = "AccountId: Serialize, Balance: std::fmt::Display",
+		deserialize = "AccountId: Deserialize<'de>, Balance: std::str::FromStr"
+	))]
 	pub deposit: Deposit<AccountId, Balance>,
 	pub authorization_id: Option<AuthorizationId>,
 }
@@ -68,10 +56,13 @@ impl<CTypeHash, Attester, BlockNumber, AccountId, Balance, AuthorizationId>
 	}
 }
 
+/// Filter for public credentials retrieved for a provided subject as specified in the RPC interface.
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 pub enum PublicCredentialFilter<CTypeHash, Attester> {
+	/// Filter credentials that match a specified Ctype.
 	CtypeHash(CTypeHash),
+	/// Filter credentials that have been issued by the specified attester.
 	Attester(Attester),
 }
 
@@ -87,7 +78,7 @@ where
 		credential: &CredentialEntry<CTypeHash, Attester, BlockNumber, AccountId, Balance, AuthorizationId>,
 	) -> bool {
 		match self {
-			Self::CTypeHash(ctype_hash) => ctype_hash == &credential.ctype_hash,
+			Self::CtypeHash(ctype_hash) => ctype_hash == &credential.ctype_hash,
 			Self::Attester(attester) => attester == &credential.attester,
 		}
 	}

pallets/public-credentials/src/benchmarking.rs

@@ -69,6 +69,7 @@ benchmarks! {
 		assert!(CredentialSubjects::<T>::contains_key(&credential_id));
 	}
 
+	// Very similar setup as `remove`
 	revoke {
 		let sender: T::AccountId = account("sender", 0, SEED);
 		let attester: T::AttesterId = account("attester", 0, SEED);
@@ -96,6 +97,7 @@ benchmarks! {
 		assert!(Credentials::<T>::get(subject_id, &credential_id).expect("Credential should be present in storage").revoked);
 	}
 
+	// Very similar setup as `remove`
 	unrevoke {
 		let sender: T::AccountId = account("sender", 0, SEED);
 		let attester: T::AttesterId = account("attester", 0, SEED);

pallets/public-credentials/src/lib.rs

@@ -136,7 +136,7 @@ pub mod pallet {
 		type CredentialHash: Hash<Output = Self::CredentialId>;
 		/// The type of a credential identifier.
 		type CredentialId: Parameter + MaxEncodedLen;
-		/// The currency that is used to reserve funds for each attestation.
+		/// The currency that is used to reserve funds for each credential.
 		type Currency: ReservableCurrency<AccountIdOf<Self>>;
 		/// The type of the origin when successfully converted from the outer
 		/// origin.
@@ -272,7 +272,7 @@ pub mod pallet {
 				ctype::Error::<T>::CTypeNotFound
 			);
 
-			// Credential ID = H(<encoded_credential_input> || <attester_identifier>)
+			// Credential ID = H(<scale_encoded_credential_input> || <scale_encoded_attester_identifier>)
 			let credential_id =
 				T::CredentialHash::hash(&[&credential.encode()[..], &attester.encode()[..]].concat()[..]);
 
@@ -359,16 +359,17 @@ pub mod pallet {
 					if let Some(credential) = credential_entry {
 						// Additional weight is 0 if the caller is the attester, otherwise it's the
 						// value returned by the access control check, if it does not fail.
-						let additional_weight = if caller != credential.attester {
+						let additional_weight = if caller == credential.attester {
+							0
+						} else {
 							let credential_auth_id =
 								credential.authorization_id.as_ref().ok_or(Error::<T>::Unauthorized)?;
 							authorization
 								.ok_or(Error::<T>::Unauthorized)?
 								.can_revoke(&caller, &credential.ctype_hash, &credential_id, credential_auth_id)
 								.map_err(|_| Error::<T>::Unauthorized)?
-						} else {
-							0
 						};
+						// If authorization checks are ok, update the revocation status.
 						credential.revoked = true;
 						Ok(additional_weight)
 					} else {
@@ -414,16 +415,17 @@ pub mod pallet {
 					if let Some(credential) = credential_entry {
 						// Additional weight is 0 if the caller is the attester, otherwise it's the
 						// value returned by the access control check, if it does not fail.
-						let additional_weight = if caller != credential.attester {
+						let additional_weight = if caller == credential.attester {
+							0
+						} else {
 							let credential_auth_id =
 								credential.authorization_id.as_ref().ok_or(Error::<T>::Unauthorized)?;
 							authorization
 								.ok_or(Error::<T>::Unauthorized)?
 								.can_revoke(&caller, &credential.ctype_hash, &credential_id, credential_auth_id)
 								.map_err(|_| Error::<T>::Unauthorized)?
-						} else {
-							0
 						};
+						// If authorization checks are ok, update the revocation status.
 						credential.revoked = false;
 						Ok(additional_weight)
 					} else {
@@ -469,7 +471,9 @@ pub mod pallet {
 
 			let (credential_subject, credential_entry) = Self::retrieve_credential_entry(&credential_id)?;
 
-			let ac_weight_used = if credential_entry.attester != caller {
+			let ac_weight_used = if credential_entry.attester == caller {
+				0
+			} else {
 				let credential_auth_id = credential_entry
 					.authorization_id
 					.as_ref()
@@ -483,8 +487,6 @@ pub mod pallet {
 						credential_auth_id,
 					)
 					.map_err(|_| Error::<T>::Unauthorized)?
-			} else {
-				0
 			};
 
 			Self::remove_credential_entry(credential_subject, credential_id, credential_entry);
@@ -511,19 +513,6 @@ pub mod pallet {
 	}
 
 	impl<T: Config> Pallet<T> {
-		fn retrieve_credential_entry(
-			credential_id: &CredentialIdOf<T>,
-		) -> Result<(T::SubjectId, CredentialEntryOf<T>), Error<T>> {
-			// Verify that the credential exists
-			let credential_subject =
-				CredentialSubjects::<T>::get(&credential_id).ok_or(Error::<T>::CredentialNotFound)?;
-
-			// Should never happen if the line above succeeds
-			Credentials::<T>::get(&credential_subject, &credential_id)
-				.map(|entry| (credential_subject, entry))
-				.ok_or(Error::<T>::InternalError)
-		}
-
 		// Simple wrapper to remove entries from both storages when deleting a
 		// credential.
 		fn remove_credential_entry(
@@ -540,5 +529,18 @@ pub mod pallet {
 				credential_id,
 			});
 		}
+
+		fn retrieve_credential_entry(
+			credential_id: &CredentialIdOf<T>,
+		) -> Result<(T::SubjectId, CredentialEntryOf<T>), Error<T>> {
+			// Verify that the credential exists
+			let credential_subject =
+				CredentialSubjects::<T>::get(&credential_id).ok_or(Error::<T>::CredentialNotFound)?;
+
+			// Should never happen if the line above succeeds
+			Credentials::<T>::get(&credential_subject, &credential_id)
+				.map(|entry| (credential_subject, entry))
+				.ok_or(Error::<T>::InternalError)
+		}
 	}
 }

rpc/public-credentials/src/lib.rs

@@ -31,6 +31,7 @@ use sp_api::ProvideRuntimeApi;
 use sp_blockchain::HeaderBackend;
 use sp_runtime::{generic::BlockId, traits::Block as BlockT};
 
+/// Filter that can be used after the credentials for a given subject have been retrieved from the blockchain state.
 pub trait PublicCredentialsFilter<Credential> {
 	fn should_include(&self, credential: &Credential) -> bool;
 }
@@ -49,7 +50,7 @@ pub trait PublicCredentialsApi<BlockHash, OuterSubjectId, OuterCredentialId, Out
 
 	/// Return all the credentials issued to the provided subject, optionally
 	/// filtering with the provided logic. The result is a vector of (credential
-	/// root hash, credential entry).
+	/// identifier, credential entry).
 	#[method(name = "get_credentials")]
 	fn get_credentials(
 		&self,
@@ -71,6 +72,7 @@ pub struct PublicCredentialsQuery<
 	CredentialFilter,
 > {
 	client: Arc<Client>,
+	#[allow(clippy::type_complexity)]
 	_marker: std::marker::PhantomData<(
 		Block,
 		OuterSubjectId,

runtimes/common/Cargo.toml

@@ -43,7 +43,6 @@ runtime-benchmarks = [
   "frame-support/runtime-benchmarks",
   "kilt-support/runtime-benchmarks",
   "parachain-staking/runtime-benchmarks",
-  "public-credentials/runtime-benchmarks",
   "sp-runtime/runtime-benchmarks",
 ]
 std = [

runtimes/common/src/constants.rs

@@ -434,7 +434,7 @@ pub mod public_credentials {
 	use super::*;
 
 	/// The size is checked in the runtime by a test.
-	pub const MAX_PUBLIC_CREDENTIAL_STORAGE_LENGTH: u32 = 257;
+	pub const MAX_PUBLIC_CREDENTIAL_STORAGE_LENGTH: u32 = 355;
 	// Each credential would have a different deposit, so no multiplier here
 	pub const PUBLIC_CREDENTIAL_DEPOSIT: Balance = deposit(1, MAX_PUBLIC_CREDENTIAL_STORAGE_LENGTH);
 

runtimes/peregrine/src/lib.rs

@@ -664,7 +664,7 @@ impl public_credentials::Config for Runtime {
 	type OriginSuccess = did::DidRawOrigin<AccountId, DidIdentifier>;
 	type Event = Event;
 	type SubjectId = runtime_common::assets::AssetDid;
-	type WeightInfo = ();
+	type WeightInfo = weights::public_credentials::WeightInfo<Runtime>;
 }
 
 /// The type used to represent the kinds of proxying allowed.

runtimes/spiritnet/src/lib.rs

@@ -659,7 +659,7 @@ impl public_credentials::Config for Runtime {
 	type OriginSuccess = did::DidRawOrigin<AccountId, DidIdentifier>;
 	type Event = Event;
 	type SubjectId = runtime_common::assets::AssetDid;
-	type WeightInfo = ();
+	type WeightInfo = weights::public_credentials::WeightInfo<Runtime>;
 }
 
 /// The type used to represent the kinds of proxying allowed.