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

Contracts remove deposit accounts #14589

Merged
merged 191 commits into from
Aug 18, 2023

Conversation

juangirini
Copy link
Contributor

@juangirini juangirini commented Jul 17, 2023

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

}

/// Charges from `origin` a storage deposit for contract instantiation.
/// Charge from `origin` a storage deposit plus the ed for contract instantiation.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@pgherveou pgherveou Aug 17, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// 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)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 17, 2023

@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
Copy link

command-bot bot commented Aug 17, 2023

@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
Copy link

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

Comment on lines -339 to -341
fn is_alive(&self) -> bool {
matches!(self.own_contribution, Contribution::Alive(_))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@juangirini juangirini Aug 18, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

frame/contracts/src/migration/v15.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
Comment on lines -339 to -341
fn is_alive(&self) -> bool {
matches!(self.own_contribution, Contribution::Alive(_))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

juangirini and others added 2 commits August 18, 2023 15:32
Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
@juangirini
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

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

@juangirini
Copy link
Contributor Author

bot merge force

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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Development

Successfully merging this pull request may close these issues.

10 participants