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

Contracts: migrate to fungible traits #14020

Merged

Conversation

juangirini
Copy link
Contributor

@juangirini juangirini commented Apr 26, 2023

Fix #13643

Cumulus companion: paritytech/cumulus#2841

The Currency (and ReservableCurrency) traits are deprecated after this PR in favour of the fungible traits.

A few things have changed:

  • ReservableCurrency::reserve becomes fungible::hold::Mutate::hold; and ReservableCurrency::unreserve becomes fungible::hold::Mutate::release

Holds are roughly analagous to reserves, but are now explicitly designed to be infallibly slashed. They do not contribute to the ED but do require a provider reference, removing any possibility of account reference counting from being problematic for a slash. They are also always named, ensuring different holds do not accidentally slash each other's balances.

Also they require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

In the Pallet Contracts context we only have one identifier only HoldReason::CodeUploadDepositReserve

/// A reason for the pallet contracts placing a hold on funds.
#[pallet::composite_enum]
pub enum HoldReason {
    /// The Pallet has reserved it for storage deposit.
    CodeUploadDepositReserve,
}
  • Due to the changes from reserve/unreserve to hold/release some events have been changed:

    • pallet_balances::Event::Reserved is not fired anymore in the context of Contracts, instead pallet_contracts::Event::CodeStored contains a deposit_held field.
    • analogously pallet_balances::Event::Unreserved is not fired anymore in the context of Contracts, instead pallet_contracts::Event::CodeRemoved contains a deposit_held field.
      Please note that these events belong to pallet_contracts as the fungible traits don't emit any events on hold and release.
  • The ExistenceRequirement enum is not used anymore, it is replaced by Preservation. They don't map exactly therefore a few adaptations have been made.

/// Simple boolean for whether an account needs to be kept in existence.
#[derive(Copy, Clone, RuntimeDebug, Eq, PartialEq)]
pub enum ExistenceRequirement {
    /// Operation must not result in the account going out of existence.
    ///
    /// Note this implies that if the account never existed in the first place, then the operation
    /// may legitimately leave the account unchanged and still non-existent.
    KeepAlive,
    /// Operation may result in account going out of existence.
    AllowDeath,
}
/// The mode by which we describe whether an operation should keep an account alive.
#[derive(Copy, Clone, RuntimeDebug, Eq, PartialEq)]
pub enum Preservation {
    /// We don't care if the account gets killed by this operation.
    Expendable,
    /// The account may not be killed, but we don't care if the balance gets dusted.
    Protect,
    /// The account may not be killed and our provider reference must remain (in the context of
    /// tokens, this means that the account may not be dusted).
    Preserve,
}
  • Currency::transfer() does not perform the sanity it used to anymore, so those are now addressed before calling Currency::transfer()
if !value.is_zero() && from != to {
    T::Currency::transfer(from, to, value, preservation)
        .map_err(|_| Error::<T>::TransferFailed)?;
}

Upgrade notes

The pallet_contracts::Config trait now requires new type RuntimeHoldReason:

impl pallet_contracts::Config for Runtime {
	...
	type RuntimeHoldReason = RuntimeHoldReason;
	...
}

Also, construct_runtime needs pallet_contracts::HoldReason:

construct_runtime!(
	pub enum Runtime where
		...
	{
		...
		// Smart Contracts.
		Contracts: pallet_contracts::{Pallet, Call, Storage, Event<T>, HoldReason} = ...,
		...
	}
)

Migrations generic OldCurrency

Some migrations require a generic OldCurrency that implements the traits that the previous T::Currency implemented. Most commonly we could just use pallet_balances::Pallet<T>.


Deposit account

After this PR we should no longer need the deposit accounts, as the new "held" balance does not contribute to the ED.

This PR does not alters the deposit accounts and its functionality. There is this other PR #14589 that takes care of that.

@juangirini juangirini linked an issue Apr 26, 2023 that may be closed by this pull request
@juangirini juangirini self-assigned this Apr 26, 2023
@juangirini juangirini added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 26, 2023
@juangirini juangirini changed the title Jg/13643 contracts migrate to fungible traits [WIP] Jg/13643 contracts migrate to fungible traits Apr 26, 2023
@athei
Copy link
Member

athei commented May 10, 2023

Needs a migration. So we wait with merging until #14045 is merged so we can include the migration in this PR.

frame/contracts/src/migration/v12.rs Outdated Show resolved Hide resolved
frame/contracts/src/migration/v14.rs Outdated Show resolved Hide resolved
frame/contracts/src/migration/v14.rs Outdated Show resolved Hide resolved
Comment on lines +264 to +268
log::info!(
target: LOG_TARGET,
"Total held amount for storage deposit: {:?}",
total_held
);
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't comparing the total held vs reserved amount be enough here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, as there could be several reasons the code owner could have balance reserved, not just for uploaded code deposit. Therefore the "old" reserved balance should be >= than the "new" held balance under the HoldReason::CodeUploadDepositReserve, and that's a loose comparison to make.

frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
@@ -4256,12 +4225,15 @@ fn set_code_extrinsic() {
#[test]
fn slash_cannot_kill_account() {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this test at all as slashing can not kill account by design now that we moved to fungible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I would agree, but the fact that this extra test is there does not hurt, and it adds an extra shield that can potentially prevent a situation in which we are not thinking of at the moment. (It might sound stupid, but maybe a situation where the Currency type used in the test does not honour the actual definition of the trait implementation)

@juangirini juangirini requested a review from athei July 31, 2023 21:03
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
@juangirini juangirini requested a review from athei August 2, 2023 08:40
@juangirini
Copy link
Contributor Author

bot merge

1 similar comment
@juangirini
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f7e81b9 into master Aug 2, 2023
49 of 50 checks passed
@paritytech-processbot paritytech-processbot bot deleted the jg/13643-contracts-migrate-to-fungible-traits branch August 2, 2023 13:51
dmitry-markin added a commit to paritytech/cumulus that referenced this pull request Aug 2, 2023
paritytech-processbot bot pushed a commit to paritytech/cumulus that referenced this pull request Aug 2, 2023
* Init `PeerStore` in `build_collator_network`

* kick CI

* Update Cargo.lock for substrate

* Companion for substrate 14373

* Companion for paritytech/substrate#14020

* update lockfile for {"substrate", "polkadot"}

---------

Co-authored-by: Davide Galassi <davxy@datawok.net>
Co-authored-by: parity-processbot <>
@juangirini juangirini changed the title Jg/13643 contracts migrate to fungible traits Contracts: migrate to fungible traits Aug 3, 2023
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 C3-medium PR touches the given topic and has a medium impact on builders. D2-breaksapi F3-breaks_API This PR changes public API; next release should be major. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contracts: Migrate to fungible traits
9 participants