XCM: Derive remote accounts according to polkadot standards #2315
Conversation
librelois
added
B7-runtimenoteworthy
|
Coverage generated "Thu May 25 14:39:08 UTC 2023": Master coverage: 70.88% |
| impl From<[u8; 32]> for AccountId20 { | ||
| fn from(bytes: [u8; 32]) -> Self { | ||
| let mut buffer = [0u8; 20]; | ||
| buffer.copy_from_slice(&bytes[..20]); | ||
| Self(buffer) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this could be misleading to put here because there are many (most) cases where we don't want to create and use an AccountId20 from an arbitrary 32-byte array.
Whenever we use an AccountId20 in a trusted way, it's important that it come from something that a user either doesn't have influence over or is extremely hard to guess (e.g. guessing random private keys). I worry that putting this simple conversion could lead to misuse by using an AccountId20 that was generated from an insecure source of bytes.
There was a problem hiding this comment.
Overall you are right, but this conversion is only made because the conversion built by polkadot works with 32 byte accounts, so we need to pass from there to a 20 byte account.
This has no security concerns because the 32 byte account is derived with a hash in the first place.
There was a problem hiding this comment.
The particular usage of it isn't what concerns me, it's having it in the first place. Couldn't we create a fn that needs to be explicitly called, something like unsafe_from_bytes_32 or from_known_secure_bytes or something like that? Doing so would obviously avoid any possible confusion.
There was a problem hiding this comment.
Or is there a requirement that there be a From impl for it to work?
There was a problem hiding this comment.
@notlesh the From<[u8; 32]> impl is a requirement from ForeignChainAliasAccount, it can't compile without it
| ...new Uint8Array([32]), | ||
| ...new TextEncoder().encode("multiloc"), | ||
| ...derivedMultiLocation.toU8a(), | ||
| ...new TextEncoder().encode("ForeignChainAliasAccountPrefix_Para32"), |
There was a problem hiding this comment.
What happens if we need to test the conversion of an address coming from the relay? Shouldn't we add an extra method (e.g descendOriginFromRelay) for that? And include the ForeignChainAliasAccountPrefix_Relay.
There was a problem hiding this comment.
@Agusrodri, yes we will need to add a new helper function descendOriginFromRelay
* update to v0.9.40 * update benchmarking weight template * fix build * make test compile * Includes page heap fixes * compile runtime-benchmarks * make warp sync work * toml sort * fix editorconfig * use new substrate version * fix warp sync * sort * fix --dev * remove duplicate SetMembersOrigin * toml-sort * remove kitchensink-runtime * fix builkd * use new weights * set manual weights for xcm fungible * use Weight::from_parts * use 0 pov_size for ref_time weight * update nimbus * exclude generated weight files from editorconfig * fmt * fmt * fix rust tests * fix import * fix tests * use Weight part pov_size to 0 * make dalek test work * fix transfer tests * use BoundedVec for auto compound delegations * fix modexp test * fix modexp test * fix tests * fix weight tests * fix staking tests via chunking * fix modexp test * fix lint and test * fix rust weight tests * fix partial ts tests * temp fix for xcm v2 * Fixes weight until benchmarking is fixed * set manual weight, fix ts tests * Adds temp hack for xcm tests * Use RefundSurplus as the no-op for saturating the queue, which does not have pov * Update evm to 0.39 * Revert "Update evm to 0.39" This reverts commit 882b85e. * upgrade polkadot for better support of xcm v2 * prettier * prettier * Revert temp fix for XCM weight * upgrade polkadot fork * Fixing hrmp-mock tests * clean up * update polkadot fork to expose ForeignChainAliasAccount * Generate remote accounts according to polkadot standards * prettier * fix rust test * ts tests first pass --------- Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com> Co-authored-by: crystalin <alan.sapede@gmail.com> Co-authored-by: girazoki <gorka.irazoki@gmail.com> Co-authored-by: tgmichel <telmo@purestake.com>
notlesh
added
D1-audited👍
What does it do?
Cherry pick paritytech/polkadot#6662 on our polkadot fork and replace our custom non-standard hash derivation by the new type
ForeignChainAliasAccountThe
DescendOrigininstruction now derives accounts in a different way:new behavior:
blake2_256("ForeignChainAliasAccountPrefix_Para20" | paraId | address | parents)old behavior:
blake2_256("multiloc" | multilocation)What important points reviewers should know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?