Explicitly note that existing AccountIdConversion is truncating and…

… add fallible `try_into...` (paritytech#10719)

* note truncating, add fallible try_into

* fmt

* migrate all to `truncating`

* typo

* uno mas

* Update primitives/runtime/src/traits.rs

Co-authored-by: Kian Paimani <[email protected]>

* check the bytes before and after are sensible

* fmt

* Update lib.rs

* Update primitives/runtime/src/traits.rs

Co-authored-by: Bastian Köcher <[email protected]>

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

frame/bounties/src/lib.rs

@@ -799,14 +799,14 @@ impl<T: Config> Pallet<T> {
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn account_id() -> T::AccountId {
-		T::PalletId::get().into_account()
+		T::PalletId::get().into_account_truncating()
 	}
 
 	/// The account ID of a bounty account
 	pub fn bounty_account_id(id: BountyIndex) -> T::AccountId {
 		// only use two byte prefix to support 16 byte account id (used by test)
 		// "modl" ++ "py/trsry" ++ "bt" is 14 bytes, and two bytes remaining for bounty index
-		T::PalletId::get().into_sub_account(("bt", id))
+		T::PalletId::get().into_sub_account_truncating(("bt", id))
 	}
 
 	fn create_bounty(

frame/child-bounties/src/lib.rs

@@ -786,7 +786,7 @@ impl<T: Config> Pallet<T> {
 		// This function is taken from the parent (bounties) pallet, but the
 		// prefix is changed to have different AccountId when the index of
 		// parent and child is same.
-		T::PalletId::get().into_sub_account(("cb", id))
+		T::PalletId::get().into_sub_account_truncating(("cb", id))
 	}
 
 	fn create_child_bounty(

frame/lottery/src/lib.rs

@@ -389,7 +389,7 @@ impl<T: Config> Pallet<T> {
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn account_id() -> T::AccountId {
-		T::PalletId::get().into_account()
+		T::PalletId::get().into_account_truncating()
 	}
 
 	/// Return the pot account and amount of money in the pot.

frame/nomination-pools/src/lib.rs

@@ -2018,14 +2018,14 @@ impl<T: Config> Pallet<T> {
 
 	/// Create the main, bonded account of a pool with the given id.
 	pub fn create_bonded_account(id: PoolId) -> T::AccountId {
-		T::PalletId::get().into_sub_account((AccountType::Bonded, id))
+		T::PalletId::get().into_sub_account_truncating((AccountType::Bonded, id))
 	}
 
 	/// Create the reward account of a pool with the given id.
 	pub fn create_reward_account(id: PoolId) -> T::AccountId {
 		// NOTE: in order to have a distinction in the test account id type (u128), we put
 		// account_type first so it does not get truncated out.
-		T::PalletId::get().into_sub_account((AccountType::Reward, id))
+		T::PalletId::get().into_sub_account_truncating((AccountType::Reward, id))
 	}
 
 	/// Get the member with their associated bonded and reward pool.

frame/society/src/lib.rs

@@ -1726,15 +1726,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn account_id() -> T::AccountId {
-		T::PalletId::get().into_account()
+		T::PalletId::get().into_account_truncating()
 	}
 
 	/// The account ID of the payouts pot. This is where payouts are made from.
 	///
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn payouts() -> T::AccountId {
-		T::PalletId::get().into_sub_account(b"payouts")
+		T::PalletId::get().into_sub_account_truncating(b"payouts")
 	}
 
 	/// Return the duration of the lock, in blocks, with the given number of members.

frame/tips/src/lib.rs

@@ -467,7 +467,7 @@ impl<T: Config> Pallet<T> {
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn account_id() -> T::AccountId {
-		T::PalletId::get().into_account()
+		T::PalletId::get().into_account_truncating()
 	}
 
 	/// Given a mutable reference to an `OpenTip`, insert the tip into it and check whether it

frame/treasury/src/lib.rs

@@ -437,7 +437,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// This actually does computation. If you need to keep using it, then make sure you cache the
 	/// value and only call this once.
 	pub fn account_id() -> T::AccountId {
-		T::PalletId::get().into_account()
+		T::PalletId::get().into_account_truncating()
 	}
 
 	/// The needed bond for a proposal whose spend is `value`.

primitives/runtime/src/traits.rs

@@ -1243,38 +1243,71 @@ impl<'a> codec::Input for TrailingZeroInput<'a> {
 
 /// This type can be converted into and possibly from an AccountId (which itself is generic).
 pub trait AccountIdConversion<AccountId>: Sized {
-	/// Convert into an account ID. This is infallible.
-	fn into_account(&self) -> AccountId {
-		self.into_sub_account(&())
+	/// Convert into an account ID. This is infallible, and may truncate bytes to provide a result.
+	/// This may lead to duplicate accounts if the size of `AccountId` is less than the seed.
+	fn into_account_truncating(&self) -> AccountId {
+		self.into_sub_account_truncating(&())
+	}
+
+	/// Convert into an account ID, checking that all bytes of the seed are being used in the final
+	/// `AccountId` generated. If any bytes are dropped, this returns `None`.
+	fn try_into_account(&self) -> Option<AccountId> {
+		self.try_into_sub_account(&())
 	}
 
 	/// Try to convert an account ID into this type. Might not succeed.
 	fn try_from_account(a: &AccountId) -> Option<Self> {
 		Self::try_from_sub_account::<()>(a).map(|x| x.0)
 	}
 
-	/// Convert this value amalgamated with the a secondary "sub" value into an account ID. This is
-	/// infallible.
+	/// Convert this value amalgamated with the a secondary "sub" value into an account ID,
+	/// truncating any unused bytes. This is infallible.
 	///
 	/// NOTE: The account IDs from this and from `into_account` are *not* guaranteed to be distinct
 	/// for any given value of `self`, nor are different invocations to this with different types
 	/// `T`. For example, the following will all encode to the same account ID value:
 	/// - `self.into_sub_account(0u32)`
 	/// - `self.into_sub_account(vec![0u8; 0])`
 	/// - `self.into_account()`
-	fn into_sub_account<S: Encode>(&self, sub: S) -> AccountId;
+	///
+	/// Also, if the seed provided to this function is greater than the number of bytes which fit
+	/// into this `AccountId` type, then it will lead to truncation of the seed, and potentially
+	/// non-unique accounts.
+	fn into_sub_account_truncating<S: Encode>(&self, sub: S) -> AccountId;
+
+	/// Same as `into_sub_account_truncating`, but ensuring that all bytes of the account's seed are
+	/// used when generating an account. This can help guarantee that different accounts are unique,
+	/// besides types which encode the same as noted above.
+	fn try_into_sub_account<S: Encode>(&self, sub: S) -> Option<AccountId>;
 
 	/// Try to convert an account ID into this type. Might not succeed.
 	fn try_from_sub_account<S: Decode>(x: &AccountId) -> Option<(Self, S)>;
 }
 
-/// Format is TYPE_ID ++ encode(parachain ID) ++ 00.... where 00... is indefinite trailing zeroes to
+/// Format is TYPE_ID ++ encode(sub-seed) ++ 00.... where 00... is indefinite trailing zeroes to
 /// fill AccountId.
 impl<T: Encode + Decode, Id: Encode + Decode + TypeId> AccountIdConversion<T> for Id {
-	fn into_sub_account<S: Encode>(&self, sub: S) -> T {
+	// Take the `sub` seed, and put as much of it as possible into the generated account, but
+	// allowing truncation of the seed if it would not fit into the account id in full. This can
+	// lead to two different `sub` seeds with the same account generated.
+	fn into_sub_account_truncating<S: Encode>(&self, sub: S) -> T {
 		(Id::TYPE_ID, self, sub)
 			.using_encoded(|b| T::decode(&mut TrailingZeroInput(b)))
-			.expect("`AccountId` type is never greater than 32 bytes; qed")
+			.expect("All byte sequences are valid `AccountIds`; qed")
+	}
+
+	// Same as `into_sub_account_truncating`, but returns `None` if any bytes would be truncated.
+	fn try_into_sub_account<S: Encode>(&self, sub: S) -> Option<T> {
+		let encoded_seed = (Id::TYPE_ID, self, sub).encode();
+		let account = T::decode(&mut TrailingZeroInput(&encoded_seed))
+			.expect("All byte sequences are valid `AccountIds`; qed");
+		// If the `account` generated has less bytes than the `encoded_seed`, then we know that
+		// bytes were truncated, and we return `None`.
+		if encoded_seed.len() <= account.encoded_size() {
+			Some(account)
+		} else {
+			None
+		}
 	}
 
 	fn try_from_sub_account<S: Decode>(x: &T) -> Option<(Self, S)> {
@@ -1652,6 +1685,13 @@ mod tests {
 		let _ = s.verify(&[0u8; 100][..], &Public::unchecked_from([0; 32]));
 	}
 
+	#[derive(Encode, Decode, Default, PartialEq, Debug)]
+	struct U128Value(u128);
+	impl super::TypeId for U128Value {
+		const TYPE_ID: [u8; 4] = [0x0d, 0xf0, 0x0d, 0xf0];
+	}
+	// f00df00d
+
 	#[derive(Encode, Decode, Default, PartialEq, Debug)]
 	struct U32Value(u32);
 	impl super::TypeId for U32Value {
@@ -1669,9 +1709,19 @@ mod tests {
 	type AccountId = u64;
 
 	#[test]
-	fn into_account_should_work() {
-		let r: AccountId = U32Value::into_account(&U32Value(0xdeadbeef));
+	fn into_account_truncating_should_work() {
+		let r: AccountId = U32Value::into_account_truncating(&U32Value(0xdeadbeef));
+		assert_eq!(r, 0x_deadbeef_cafef00d);
+	}
+
+	#[test]
+	fn try_into_account_should_work() {
+		let r: AccountId = U32Value::try_into_account(&U32Value(0xdeadbeef)).unwrap();
 		assert_eq!(r, 0x_deadbeef_cafef00d);
+
+		// u128 is bigger than u64 would fit
+		let maybe: Option<AccountId> = U128Value::try_into_account(&U128Value(u128::MAX));
+		assert!(maybe.is_none());
 	}
 
 	#[test]
@@ -1681,9 +1731,22 @@ mod tests {
 	}
 
 	#[test]
-	fn into_account_with_fill_should_work() {
-		let r: AccountId = U16Value::into_account(&U16Value(0xc0da));
+	fn into_account_truncating_with_fill_should_work() {
+		let r: AccountId = U16Value::into_account_truncating(&U16Value(0xc0da));
+		assert_eq!(r, 0x_0000_c0da_f00dcafe);
+	}
+
+	#[test]
+	fn try_into_sub_account_should_work() {
+		let r: AccountId = U16Value::try_into_account(&U16Value(0xc0da)).unwrap();
 		assert_eq!(r, 0x_0000_c0da_f00dcafe);
+
+		let maybe: Option<AccountId> = U16Value::try_into_sub_account(
+			&U16Value(0xc0da),
+			"a really large amount of additional encoded information which will certainly overflow the account id type ;)"
+		);
+
+		assert!(maybe.is_none())
 	}
 
 	#[test]

utils/frame/frame-utilities-cli/src/pallet_id.rs

@@ -72,7 +72,7 @@ impl PalletIdCmd {
 			"Cannot convert argument to palletid: argument should be 8-character string"
 		})?;
 
-		let account_id: R::AccountId = PalletId(id_fixed_array).into_account();
+		let account_id: R::AccountId = PalletId(id_fixed_array).into_account_truncating();
 
 		with_crypto_scheme!(
 			self.crypto_scheme.scheme,