Contracts remove deposit accounts by juangirini · Pull Request #14589 · paritytech/substrate

juangirini · 2023-07-17T15:16:41Z

In this PR we are removing the deposit accounts.

This is a follow up of #14020 in which we got rid of the Currency traits in favour of the fungible traits, and therefore we hold deposits instead of reserving them.

ED doesn't contribute to held balances and this balance can only be slashed by the same HoldReason it was created under, removing the need of maintaining the deposit accounts.

Migrations

Migrations v10 and v13 operated over deposit accounts and they have been updated to be kept backwards compatible for those chain which haven't ran it yet.

A new migration v15 has been added that moved the balance on the deposit accounts back to the contract accounts and hold them there under the StorageDepositReserve reason.

This new migration needs to be added to the Config

impl Config for Runtime {
	// ...
	type Migrations = (
		// ...
		v15::Migration<Runtime>,
	);
	// ...
}

Cumulus companion: paritytech/cumulus#2973
Complementary PR #14767

…migrate-to-fungible-traits

…-deposit-account

pgherveou · 2023-08-17T06:56:46Z

frame/contracts/src/storage/meter.rs

 	}

-	/// Charges from `origin` a storage deposit for contract instantiation.
+	/// Charge from `origin` a storage deposit plus the ed for contract instantiation.


We transfer ed then charge deposit minus ed, so this comment or the code seems wrong 🤔

charge is in itself a transfer, so I would not distinguish from transfer and charge. In fact we are taking "storage deposit + ed" from the origin.
How can we rephrase this to make it more clear?

In fact we are taking "storage deposit + ed" from the origin.

are you?
we do

T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?; //... E::charge(origin, contract, &deposit.saturating_sub(&Deposit::Charge(ed)), false)?;

you take ed and then deposit - ed, am I overlooking something here?

also since we transfer ed directly, couldn't we then use fn charge_deposit to delay the charge until later?

you take ed and then deposit - ed, am I overlooking something here?

yes, you're right

also since we transfer ed directly, couldn't we then use fn charge_deposit to delay the charge until later?

absolutely, good point

pgherveou · 2023-08-17T06:57:36Z

frame/contracts/src/storage/meter.rs

-		// We also need to make sure that the contract's account itself exists.
-		T::Currency::transfer(origin, contract, ed, Preservation::Preserve)?;
-		System::<T>::inc_consumers(contract)?;
+		E::charge(origin, contract, &deposit.saturating_sub(&Deposit::Charge(ed)), false)?;


comment above seems off as well now, since you start by transferring ed

Yeah, I don't think we need any comment at all there, maybe just the fact that we are excluding the ed from the deposit because it was transferred in the previous step. Would you add anything else?

yep let's just remove it, also see my comment above

* transfer to beneficiary after transfer_on_hold * wip * add consumer comment * review updates * fix typo * make clippy happy * refactor `Terminated` * rename ContractStatus to ContractState * rename status to state * replace Contribution::Alive to ContractState::Alive

juangirini · 2023-08-17T14:03:18Z

bot fmt

command-bot · 2023-08-17T14:03:23Z

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3408730 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 48-374b1838-ce5c-45c8-b223-3377bd57b5f7 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-08-17T14:05:44Z

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3408730 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3408730/artifacts/download.

paritytech-cicd-pr · 2023-08-17T14:13:54Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3408790

pgherveou · 2023-08-18T09:07:34Z

frame/contracts/src/storage/meter.rs

-	fn is_alive(&self) -> bool {
-		matches!(self.own_contribution, Contribution::Alive(_))
-	}


I would keep it around seems easier to read than in lining matches!(self.contract_state(), ContractState::Alive) in a few places

could be, though given the fact that we have ContractState::Alive and Contribution::Alive() the inline seems more meaningful... but I don't have a strong opinion here
@agryaznov wdyt?

it's about contract's state, not RawMeter's. I think it's ok like this.

frame/contracts/src/migration/v15.rs

frame/contracts/src/storage/meter.rs

agryaznov · 2023-08-18T12:31:52Z

frame/contracts/src/storage/meter.rs

-	fn is_alive(&self) -> bool {
-		matches!(self.own_contribution, Contribution::Alive(_))
-	}


it's about contract's state, not RawMeter's. I think it's ok like this.

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

juangirini · 2023-08-18T13:33:09Z

bot merge

paritytech-processbot · 2023-08-18T13:33:17Z

Waiting for commit status.

paritytech-processbot · 2023-08-18T14:13:52Z

Merge cancelled due to error. Error: Statuses failed for 63ae66a

juangirini · 2023-08-18T14:24:48Z

bot merge force

juangirini added 30 commits April 26, 2023 10:55

contracts: refactor currency to use fungible traits

7179359

contracts: refactor currency to use fungible traits

c0a95d2

contracts: merge master and resolve conflicts

da7db01

Merge branch 'master' into jg/13643-contracts-migrate-to-fungible-traits

9a1e824

contracts: add minor improvements

5eee859

contracts: merge master and fix conflicts

713a426

contracts: max holds config set

4acbbe5

contracts: fix some typos

e6d82d1

contracts: map token errors

c47ac0d

fix typo

4e8a7cc

contracts: add 0 balance transfer to test

33229ff

contracts: not transfer if value is zero

a1fd020

contracts: [WIP] add StorageDepositHold

0967556

Merge branch 'master' into jg/13643-contracts-migrate-to-fungible-traits

3acdcbb

contracts: add storage deposit held event

bcb9077

contracts: clean up some code and comments

8f244c4

contracts: add deposit storage released event

73b9285

contracts: update comment

26472d5

contracts: update slash cannot kill account test

c442b79

contracts: fix tests

3c515ab

contracts: add some comments to the slashing test

4d68a48

contracts: add some comments to the slashing test

78fc19e

contracts: remove references to Currency

2c1e2ea

Merge branch 'master' into jg/13643-contracts-migrate-to-fungible-traits

e6509d7

contracts: do not transfer if from equals to

1c82bef

wip

9eb19ef

Merge remote-tracking branch 'origin/master' into jg/13643-contracts-…

e4853e9

…migrate-to-fungible-traits

bound BalanceOf<T>

921d89c

added FixedPointOperand to Balance trait

a103057

move migrate sequence to config

5c14529

juangirini mentioned this pull request Aug 15, 2023

Transfer to beneficiary after transfer_on_hold #14767

Merged

juangirini added 2 commits August 15, 2023 18:00

Merge remote-tracking branch 'origin/master' into jg/contracts-remove…

7c4f0c1

…-deposit-account

wip

8d75ebc

pgherveou reviewed Aug 17, 2023

View reviewed changes

juangirini requested a review from pgherveou August 17, 2023 09:24

juangirini added 3 commits August 17, 2023 13:44

defer storage deposit charge

155dc66

Merge branch 'master' into jg/contracts-remove-deposit-account

f826196

".git/.scripts/commands/fmt/fmt.sh"

e2da7b6

remove unused imports

70fb25a

pgherveou approved these changes Aug 18, 2023

View reviewed changes

agryaznov approved these changes Aug 18, 2023

View reviewed changes

athei approved these changes Aug 18, 2023

View reviewed changes

juangirini and others added 2 commits August 18, 2023 15:32

Update frame/contracts/src/migration/v15.rs

6597a09

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

Update frame/contracts/src/storage/meter.rs

63ae66a

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

paritytech-processbot bot merged commit 94be94b into master Aug 18, 2023

paritytech-processbot bot deleted the jg/contracts-remove-deposit-account branch August 18, 2023 14:24

juangirini mentioned this pull request Oct 2, 2023

Contracts: migration v15 acc dec_consumers check paritytech/polkadot-sdk#1696

Closed

crystalin mentioned this pull request Oct 20, 2023

Update substrate/polkadot/cumulus from v0.9.43 to v1.1.0 moonbeam-foundation/moonbeam#2535

Closed

Marcin-Radecki mentioned this pull request Feb 8, 2024

A0-4022: Enable pallet contract migrations v12 to v15 Cardinal-Cryptography/aleph-node#1610

Merged

ashutoshvarma mentioned this pull request Feb 27, 2024

feat: update to polkadot v1.1.0 release AstarNetwork/Astar#1182

Merged

13 tasks

yahortsaryk mentioned this pull request May 3, 2024

Feature: Substrate 1.1.0 Cerebellum-Network/blockchain-node#281

Merged

14 tasks

Conversation

juangirini commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

In this PR we are removing the deposit accounts.

Migrations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgherveou Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juangirini commented Aug 17, 2023

Uh oh!

command-bot bot commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

command-bot bot commented Aug 17, 2023

Uh oh!

paritytech-cicd-pr commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juangirini Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juangirini commented Aug 18, 2023

Uh oh!

paritytech-processbot bot commented Aug 18, 2023

Uh oh!

paritytech-processbot bot commented Aug 18, 2023

Uh oh!

juangirini commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

juangirini commented Jul 17, 2023 •

edited

Loading

pgherveou Aug 17, 2023 •

edited

Loading

command-bot bot commented Aug 17, 2023 •

edited

Loading

juangirini Aug 18, 2023 •

edited

Loading