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

Allow Creation of Asset Accounts That Don't Exist Yet and Add Blocked Status #13843

Merged
merged 44 commits into from
May 8, 2023

Conversation

joepetrowski
Copy link
Contributor

@joepetrowski joepetrowski commented Apr 6, 2023

  • Adds new dispatchables touch_other and refund_other, allowing an asset class's Freezer or Admin to create an account with zero balance in the asset class by placing a deposit; and to remove/refund. The existence of the account is represented by a new ExistenceReason::DepositFrom variant.
  • Adds a new trait AccountTouch that allows other pallets to touch an account into existence for an asset class.
  • Adds a new account status of Blocked (invoked by the asset class's Freezer via the new dispatchable block). Unlike Frozen, which prevents withdrawals from an account, Blocked prevents both withdrawals from and deposits into the blocked account. The account can be returned to its unrestricted (Liquid) state via the existing thaw function.

Cumulus Companion: paritytech/cumulus#2437

Made in collaborating with @muharem

@joepetrowski joepetrowski added A0-please_review Pull request needs code review. 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. B1-note_worthy Changes should be noted in the release notes labels Apr 6, 2023
@joepetrowski joepetrowski added the T1-runtime This PR/Issue is related to the topic “runtime”. label Apr 6, 2023
frame/assets/src/lib.rs Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
frame/assets/src/functions.rs Outdated Show resolved Hide resolved
NachoPal

This comment was marked as resolved.

@joepetrowski
Copy link
Contributor Author

I am missing a test to check that an account does not get reaped in case reason matches ExistenceReason::DepositHeld(_) for refund() and refund_other()

Good call, if someone places a deposit, someone else shouldn't be able to remove that account. Updated logic / test.

Copy link
Contributor

@jsidorenko jsidorenko left a comment

Choose a reason for hiding this comment

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

Unblocking, as it turned out, the migration is not needed since the encoded value stays the same despite the AssetAccount.status field had got changed

@joepetrowski
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-processbot paritytech-processbot bot requested a review from a team May 7, 2023 05:42
@joepetrowski joepetrowski changed the title Allow Freezing (and Creation) of Asset Accounts That Don't Exist Yet Allow Creation of Asset Accounts That Don't Exist Yet and Add Blocked Status May 8, 2023
@joepetrowski
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/cumulus#2437 is not mergeable

@muharem
Copy link
Contributor

muharem commented May 8, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem
Copy link
Contributor

muharem commented May 8, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit cf1fb89 into master May 8, 2023
@paritytech-processbot paritytech-processbot bot deleted the joe-asset-freezecreating branch May 8, 2023 10:17
EgorPopelyaev pushed a commit that referenced this pull request May 11, 2023
…d` Status (#13843)

* prevent frozen accounts from receiving assets

* refund deposits correctly

* plus refund_other

* add benchmarks

* start migration work

* docs

* add migration logic

* fix freeze_creating benchmark

* support instanced migrations

* review

* correct deposit refund

* only allow depositor, admin, or account origin to refund deposits

* make sure refund actually removes account

* do refund changes

* Asset's account deposit owner (#13874)

* assets deposit owner

* doc typo

* remove migration

* empty commit

* can transfer to frozen account

* remove allow_burn from refund_other

* storage version back to 1

* update doc

* fix benches

* update docs

* more tests

* Update frame/assets/src/types.rs

* refund updating the reason

* refactor

* separate refund and refund_foreign

* refunds, touch_other, tests

* fixes

* fmt

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_assets

* tests: asserts asset account counts

* Account touch trait (#14063)

* assets touch trait

* docs

* move touch trait into support/traits

* permissionless flag for do_touch

* Apply suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>

* move trait to misc, drop option

* Apply suggestions from code review

Co-authored-by: Gavin Wood <[email protected]>

* correct doc

* Update frame/assets/src/functions.rs

---------

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Gavin Wood <[email protected]>
Co-authored-by: joe petrowski <[email protected]>

* Block asset account (#14070)

* replace is_fronzen flag by status enum

* block asset account

* remove redundant brackets

* fix typo

* fmt

* Apply suggestions from code review

Co-authored-by: Jegor Sidorenko <[email protected]>

* rename permissionless to check_depositor

* doc fix

* use account id lookup instead account id

* add benchmark for touch_other

---------

Co-authored-by: muharem <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Gavin Wood <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
@gilescope
Copy link
Contributor

gilescope commented May 12, 2023

What happens if a local asset's deposit is different to a foreign asset? Doesn't AccountTouch::deposit_required() need to take an AssetId to be able to answer that question?

@gavofyork
Copy link
Member

Seems a reasonable point; @muharem ^^^?

@muharem
Copy link
Contributor

muharem commented May 15, 2023

What happens if a local asset's deposit is different to a foreign asset? Doesn't AccountTouch::deposit_required() need to take an AssetId to be able to answer that question?

#14147

@louismerlin louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 6, 2023
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jul 11, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…d` Status (paritytech#13843)

* prevent frozen accounts from receiving assets

* refund deposits correctly

* plus refund_other

* add benchmarks

* start migration work

* docs

* add migration logic

* fix freeze_creating benchmark

* support instanced migrations

* review

* correct deposit refund

* only allow depositor, admin, or account origin to refund deposits

* make sure refund actually removes account

* do refund changes

* Asset's account deposit owner (paritytech#13874)

* assets deposit owner

* doc typo

* remove migration

* empty commit

* can transfer to frozen account

* remove allow_burn from refund_other

* storage version back to 1

* update doc

* fix benches

* update docs

* more tests

* Update frame/assets/src/types.rs

* refund updating the reason

* refactor

* separate refund and refund_foreign

* refunds, touch_other, tests

* fixes

* fmt

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_assets

* tests: asserts asset account counts

* Account touch trait (paritytech#14063)

* assets touch trait

* docs

* move touch trait into support/traits

* permissionless flag for do_touch

* Apply suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>

* move trait to misc, drop option

* Apply suggestions from code review

Co-authored-by: Gavin Wood <[email protected]>

* correct doc

* Update frame/assets/src/functions.rs

---------

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Gavin Wood <[email protected]>
Co-authored-by: joe petrowski <[email protected]>

* Block asset account (paritytech#14070)

* replace is_fronzen flag by status enum

* block asset account

* remove redundant brackets

* fix typo

* fmt

* Apply suggestions from code review

Co-authored-by: Jegor Sidorenko <[email protected]>

* rename permissionless to check_depositor

* doc fix

* use account id lookup instead account id

* add benchmark for touch_other

---------

Co-authored-by: muharem <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Gavin Wood <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants