Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make into_sub_account check the seed length and add hash_into_sub_account function #8672

Closed
wants to merge 2 commits into from
Closed

Make into_sub_account check the seed length and add hash_into_sub_account function #8672

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b673f29
Make into_sub_account check the seed length and add hash_into_sub_acc…
hirschenberger Apr 27, 2021
7405e9e
Formatting
hirschenberger Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 48 additions & 5 deletions primitives/runtime/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
//! Primitives for the runtime modules.

use sp_std::prelude::*;
use sp_std::{self, marker::PhantomData, convert::{TryFrom, TryInto}, fmt::Debug};
use sp_std::{
self, marker::PhantomData, convert::{TryFrom, TryInto}, fmt::Debug, mem::{size_of, size_of_val}
};
#[cfg(feature = "std")]
use std::fmt::Display;
#[cfg(feature = "std")]
Expand Down Expand Up @@ -1118,7 +1120,7 @@ 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(&()) }
fn into_account(&self) -> AccountId { self.into_sub_account(&()).unwrap() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap here seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the PR is just a WIP, with the intention to discuss the implementation with @shawntabrizi . I added some of my discussion points in the description.

The unwrap is safe here, bc. &() adds no additional size to the encoding and will never trigger a None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If unwrap is safe, then you should put .expect("proof why it is safe. q.e.d.")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is safe, in case size of T is less than size of Self then it always returns none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I assumed that in the previous implementation it always fits, but that was also not true. I think there need to be more tests especially for the corner cases. Or a new interface as you suggested in the ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the new MaxEncodedLen can be used here.


/// Try to convert an account ID into this type. Might not succeed.
fn try_from_account(a: &AccountId) -> Option<Self> {
Expand All @@ -1134,7 +1136,11 @@ pub trait AccountIdConversion<AccountId>: Sized {
/// - `self.into_sub_account(0u32)`
/// - `self.into_sub_account(vec![0u8; 0])`
/// - `self.into_account()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc is outdated

fn into_sub_account<S: Encode>(&self, sub: S) -> AccountId;
fn into_sub_account<S: Encode>(&self, sub: S) -> Option<AccountId>;

fn hash_into_sub_account<H: Hash, S: Encode + AsRef<[u8]>>(&self, sub: S) -> AccountId
where
H::Output: Encode;

/// Try to convert an account ID into this type. Might not succeed.
fn try_from_sub_account<S: Decode>(x: &AccountId) -> Option<(Self, S)>;
Expand All @@ -1143,8 +1149,21 @@ pub trait AccountIdConversion<AccountId>: Sized {
/// Format is TYPE_ID ++ encode(parachain ID) ++ 00.... where 00... is indefinite trailing zeroes to
/// fill AccountId.
impl<T: Encode + Decode + Default, Id: Encode + Decode + TypeId> AccountIdConversion<T> for Id {
fn into_sub_account<S: Encode>(&self, sub: S) -> T {
(Id::TYPE_ID, self, sub).using_encoded(|b|
fn into_sub_account<S: Encode>(&self, sub: S) -> Option<T> {
if size_of::<T>() < size_of_val(&Id::TYPE_ID) + size_of::<Self>() + sub.size_hint() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not sure that encoded size of self is size_of::<Self>(). some type have different encoded size that their type.

also size_hint is a hint, it is not exact at all.

Copy link
Contributor Author

@hirschenberger hirschenberger Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, in the tests Idid it worked as expected but I'll check again with other types.

None
} else {
(Id::TYPE_ID, self, sub).using_encoded(|b|
T::decode(&mut TrailingZeroInput(b))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe instead of using TrailingZeroInput you can use a specific struct which checks that all the bytes of b were read.

).ok()
}
}

fn hash_into_sub_account<H: Hash, S: Encode + AsRef<[u8]>>(&self, sub: S) -> T
where
H::Output: Encode
{
(Id::TYPE_ID, self, <H as Hash>::hash(sub.as_ref())).using_encoded(|b|
T::decode(&mut TrailingZeroInput(b))
).unwrap_or_default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is not from this PR but the unwrap_or_default here is also suspicious to me.
if T doesn't decode any from kind of byte slice then this will fail and always return default without anybody noticing.

I feel we can bring a better trait in order to generate an account id safely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also noticed that as a sloppy implementation.

}
Expand Down Expand Up @@ -1479,6 +1498,30 @@ mod tests {
assert!(r.is_none());
}

#[test]
fn into_subaccount_should_work() {
let r = U16Value::try_from_account(&0x0000_c0da_f00dcafe_u64).unwrap();
assert_eq!(r, U16Value(0xc0da));
let sub: Option<AccountId> = r.into_sub_account(0xbeef_u16);
assert_eq!(sub, Some(0x_beef_c0da_f00dcafe));
}

#[test]
fn into_subaccount_should_fail_on_seed_excess() {
let r = U16Value::try_from_account(&0x0000_c0da_f00dcafe_u64).unwrap();
assert_eq!(r, U16Value(0xc0da));
let sub: Option<AccountId> = r.into_sub_account(0xdeadbeef_u32);
assert!(sub.is_none());
}

#[test]
fn hash_into_subaccount_should_work() {
let r = U16Value::try_from_account(&0x0000_c0da_f00dcafe_u64).unwrap();
assert_eq!(r, U16Value(0xc0da));
let sub: AccountId = r.hash_into_sub_account::<BlakeTwo256, _>([0xef, 0xbe]);
assert_eq!(sub, 0xa50a_c0da_f00dcafe);
}

#[test]
fn trailing_zero_should_work() {
let mut t = super::TrailingZeroInput(&[1, 2, 3]);
Expand Down