diff --git a/frame/bounties/src/lib.rs b/frame/bounties/src/lib.rs index dfeef36a1fae0..b01a36b430a4e 100644 --- a/frame/bounties/src/lib.rs +++ b/frame/bounties/src/lib.rs @@ -799,14 +799,14 @@ impl Pallet { /// 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( diff --git a/frame/child-bounties/src/lib.rs b/frame/child-bounties/src/lib.rs index a8496d4e7af91..4f25fdcf8903a 100644 --- a/frame/child-bounties/src/lib.rs +++ b/frame/child-bounties/src/lib.rs @@ -786,7 +786,7 @@ impl Pallet { // 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( diff --git a/frame/lottery/src/lib.rs b/frame/lottery/src/lib.rs index bc96a029a4210..02df65a3336bf 100644 --- a/frame/lottery/src/lib.rs +++ b/frame/lottery/src/lib.rs @@ -389,7 +389,7 @@ impl Pallet { /// 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. diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index cdc3bdaf34389..ff77949cf6952 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -2018,14 +2018,14 @@ impl Pallet { /// 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. diff --git a/frame/society/src/lib.rs b/frame/society/src/lib.rs index 645a0b8ba1715..2973ecd68092e 100644 --- a/frame/society/src/lib.rs +++ b/frame/society/src/lib.rs @@ -1726,7 +1726,7 @@ impl, I: 'static> Pallet { /// 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. @@ -1734,7 +1734,7 @@ impl, I: 'static> Pallet { /// 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. diff --git a/frame/tips/src/lib.rs b/frame/tips/src/lib.rs index f43545e2b4fe8..e1c7b5e77c062 100644 --- a/frame/tips/src/lib.rs +++ b/frame/tips/src/lib.rs @@ -467,7 +467,7 @@ impl Pallet { /// 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 diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index d080d346ba3de..419970ed18afa 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -437,7 +437,7 @@ impl, I: 'static> Pallet { /// 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`. diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index 393de086f3c58..1b21e7c65ddf7 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -1243,9 +1243,16 @@ 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: 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 { + self.try_into_sub_account(&()) } /// Try to convert an account ID into this type. Might not succeed. @@ -1253,8 +1260,8 @@ pub trait AccountIdConversion: Sized { 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 @@ -1262,19 +1269,45 @@ pub trait AccountIdConversion: Sized { /// - `self.into_sub_account(0u32)` /// - `self.into_sub_account(vec![0u8; 0])` /// - `self.into_account()` - fn into_sub_account(&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(&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(&self, sub: S) -> Option; /// Try to convert an account ID into this type. Might not succeed. fn try_from_sub_account(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 AccountIdConversion for Id { - fn into_sub_account(&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(&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(&self, sub: S) -> Option { + 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(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 = 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 = 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] diff --git a/utils/frame/frame-utilities-cli/src/pallet_id.rs b/utils/frame/frame-utilities-cli/src/pallet_id.rs index c0a221d4bcd11..4ad82a01c2433 100644 --- a/utils/frame/frame-utilities-cli/src/pallet_id.rs +++ b/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,