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

Remove Default bound on AccountId types under the xcm directory #4712

Merged
merged 10 commits into from
Jan 21, 2022

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Jan 13, 2022

Fixes #4687.

cumulus companion: paritytech/cumulus#901

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 13, 2022
@KiChjang KiChjang added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jan 13, 2022
Comment on lines 46 to 47
Ok(AccountId::decode(&mut TrailingZeroInput::zeroes())
.expect("infinite length input; no invalid inputs for type; qed"))
Copy link
Member

Choose a reason for hiding this comment

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

For what is this account being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it is only used in testing, in cases where the test wants to convert a message originating from a parent location to just the zero account, i.e. a "don't care but signed" account.

As such, there's not really a huge issue leaving it as ParentIsDefault, other than to keep in line with the Default bound removal on AccountId.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this trait from the public api and make it test only.

Copy link
Contributor

@xlc xlc Jan 14, 2022

Choose a reason for hiding this comment

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

https://github.com/paritytech/cumulus/blob/2d9123d456af0b121cd588d96c5ff3040a03f576/polkadot-parachains/statemine/src/lib.rs#L445-L446

Given we want to avoid the default / zero accounts, should we make it ParentIs<AccountId, A: Get<AccountId>> and use something else for parachain runtime configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I don't even know why the default was being used here. I had assumed that default account == zero account, but it doesn't seem to be the case for me anymore via reading the comments. Is there somehow a default non-zero account associated with AccountIds in statemint/e?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think default account is always zero account.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah Default == Zero.

I'm also for removing this or doing it like @xlc proposed.

@bkchr
Copy link
Member

bkchr commented Jan 17, 2022

@KiChjang what is the status on this? I need this for my Substrate pr that removes the Default for AccountId32

@KiChjang
Copy link
Contributor Author

@bkchr I'll need to figure out what the parent accounts are for Statemint/e, currently in the test networks I still have them set to the zero account, and it is definitely not going to be the zero account on production networks.

@shawntabrizi
Copy link
Member

@KiChjang you could use something like hash("parent_parachain") or hash("polkadot/kusama") or something like that. Otherwise speak to Gav to finalize an answer.

xcm/xcm-builder/src/currency_adapter.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested review from gavofyork and removed request for adoerr January 18, 2022 15:15
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@KiChjang
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit aacb8f6 into master Jan 21, 2022
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/remove-default-on-account-id branch January 21, 2022 11:03
paritytech-processbot bot pushed a commit to paritytech/cumulus that referenced this pull request Jan 21, 2022
* Rename ParentIsDefault to ParentIsAllZeroes

* Fixes

* Create ParentAccounts for respective networks

* Fixes

* Use b"Parent" as the basis for generating parent AccountId

* Fixes

* Use preset parent account ID

* update lockfile for {"polkadot"}

Co-authored-by: parity-processbot <>
bkchr added a commit to paritytech/cumulus that referenced this pull request Jan 28, 2022
gilescope pushed a commit to paritytech/cumulus that referenced this pull request Feb 1, 2022
gilescope pushed a commit to paritytech/cumulus that referenced this pull request Feb 1, 2022
gilescope added a commit to paritytech/cumulus that referenced this pull request Feb 1, 2022
* Revert "Companion for substrate#10632 (#895)"

This reverts commit fd14576.

* Revert "Companion for paritytech/polkadot#4712 (#901)"

This reverts commit 9c977d6.

* Prepare branch

* Make sure to use `state_version = 0` for now

* Fix lock file

* Minimising changes to cargo lock

* updating to include weights update.

Co-authored-by: Bastian Köcher <info@kchr.de>
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
…aritytech#4712)

* Refactor ParentIsDefault to ParentIsAllZeroes

* Remove Default bound on all AccountId types under the xcm directory

* Change to ParentIs<A: Get<AccountId>, AccountId>

* Provide a better account for ParentIs

* Fixes

* Fixes

* Fixes

* Fixes

* Update xcm/xcm-builder/src/currency_adapter.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Use preset account ID value for parent MultiLocations

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
WebWizrd8 added a commit to WebWizrd8/cumulus that referenced this pull request Nov 21, 2022
* Rename ParentIsDefault to ParentIsAllZeroes

* Fixes

* Create ParentAccounts for respective networks

* Fixes

* Use b"Parent" as the basis for generating parent AccountId

* Fixes

* Use preset parent account ID

* update lockfile for {"polkadot"}

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Mar 10, 2023
* Rename ParentIsDefault to ParentIsAllZeroes

* Fixes

* Create ParentAccounts for respective networks

* Fixes

* Use b"Parent" as the basis for generating parent AccountId

* Fixes

* Use preset parent account ID

* update lockfile for {"polkadot"}

Co-authored-by: parity-processbot <>
ggwpez pushed a commit to ggwpez/runtimes that referenced this pull request Mar 14, 2023
* Rename ParentIsDefault to ParentIsAllZeroes

* Fixes

* Create ParentAccounts for respective networks

* Fixes

* Use b"Parent" as the basis for generating parent AccountId

* Fixes

* Use preset parent account ID

* update lockfile for {"polkadot"}

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implications of Using AccountId::default() as parent AccountId for Location-Conversion
4 participants