Make into_sub_account check the seed length and add hash_into_sub_account function #8672
Conversation
| @@ -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() } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If unwrap is safe, then you should put .expect("proof why it is safe. q.e.d.")
There was a problem hiding this comment.
I don't think it is safe, in case size of T is less than size of Self then it always returns none.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe the new MaxEncodedLen can be used here.
| @@ -1134,7 +1136,11 @@ pub trait AccountIdConversion<AccountId>: Sized { | |||
| /// - `self.into_sub_account(0u32)` | |||
| /// - `self.into_sub_account(vec![0u8; 0])` | |||
| /// - `self.into_account()` | |||
| 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
maybe instead of using TrailingZeroInput you can use a specific struct which checks that all the bytes of b were read.
| 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, I also noticed that as a sloppy implementation.
|
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
A5-stale
|
I'm unsure how to continue here. There seems to be no consensus on a good design. Maybe we can discuss again? |
stale
bot
removed
the
A5-stale
|
I still think we should come up with a design which doesn't give a I gave some thought there #8564 (comment) |
|
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
A5-stale
fixes #8564
TODO:
into_sub_accountbe renamed totry_into_sub_accountto be consistent with the rest of the trait?Optionfromhash_into_sub_accountto returnNoneif no part of the seed was included in the returnedAccountId?