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

Make automatic storage deposits resistant against changing deposit prices #12083

Merged
merged 25 commits into from
Sep 21, 2022
Merged

Make automatic storage deposits resistant against changing deposit prices #12083

merged 25 commits into from
Sep 21, 2022

Conversation

athei
Copy link
Member

@athei athei commented Aug 22, 2022 •
edited
Loading

Fixes #12064
Cumulus companion: paritytech/cumulus#1561

See the linked issue for the motivation to do this.

High level Changes

This PR does apply the following changes:

Add FixedPointOperand to Currency::Balance

This is needed to to fixed point math on balances. Of course we could solve that by adding more trait bounds to pallet-contracts but this is a lot of noise. I argue that this new bound does not constrain the balance any more than it already is. The trait is implemented for all primitive types anyways.

Delay deposit calculation

Previously, the deposit was calculated every time changes to storage were made: The storage_deposit_limit was applied eagerly. The drawback of this is that refunds cannot offset charges that happen earlier making the order of storage changes matter with regard to the limit. With the new approach the limit is enforce at the latest point possible (when the last call frame returns) which makes the order of storage changes irrelevant. This change makes the next change much easier. This required to record all charges and apply them at the end of the contract execution.

Make refunds pro rata of consumed storage

This is the actual change we wanted to make. See the linked issue for an explanation.

Storage Migration

This change requires a hefty storage migration. It needs to count all storage items of each contract. Generally, this cannot be done in a single block. However, we argue that it is early enough to do this change within a single block (no chain in existence which contains too many contracts). Please check if the upgrade would succeed within a block on your own chain before upgrading with try-runtime. The migration contains pre and post checks.

Porting Guide

This also introduces a small API change with regard to migrations: The migrate module is now private and instead we expose a type named Migration which can be put into Executive directly. No need to manually implement OnRuntimeUpgrade anymore.

@athei athei added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Aug 22, 2022
@athei athei mentioned this pull request Aug 22, 2022
Merged
@cmichi
Copy link
Contributor

cmichi commented Sep 19, 2022

Should also update substrate-contracts-node as follow-up.

agryaznov
agryaznov reviewed Sep 20, 2022
View reviewed changes
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Show resolved Hide resolved
frame/contracts/src/storage/meter.rs Outdated Show resolved Hide resolved
@athei
Copy link
Member Author

athei commented Sep 21, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 493b58b into master Sep 21, 2022
@paritytech-processbot paritytech-processbot bot deleted the at/deposit branch September 21, 2022 17:31
ordian added a commit that referenced this pull request Sep 23, 2022
@ordian
Merge branch 'master' into ao-update-kvdb-and-co
45adc12 
* master:
  [Fix] parameter_types! dead code errors (#12340)
  [Feature] Sequential migration execution for try-runtime (#12319)
  bench: Use `_` instead  of `::` in auto-generated file names (#12332)
  Fast Unstake Pallet (#12129)
  Rename anonymous to pure proxy (#12283)
  Migrate remaining old decl_* macros to the new pallet attribute macros (#12271)
  pallet-utility: Disallow none origin (#12321)
  Make automatic storage deposits resistant against changing deposit prices (#12083)
  Format templates and fix `--steps` default value (#12286)
  Bump `wasmtime` to 1.0.0 (#12317)
  Introduce 'intermediate_insert' method to hide implementation details (#12215)
  Bound staking storage items (#12230)
  Use `array-bytes` for All Array/Bytes/Hex Operations (#12190)
  BREAKING: Rename Origin (#12258)
  Use temporary db for benchmarking (#12254)
  rpc: Implement `chainSpec` RPC API (#12261)
  Import target block body during warp sync (#12300)
  Proper naming wrt expectations (#12311)
  [ci] Revert cancel-pipeline job (#12309)
@athei athei mentioned this pull request Sep 27, 2022
Merged
@jasl jasl mentioned this pull request Oct 8, 2022
Merged
@4meta5 4meta5 mentioned this pull request Oct 17, 2022
Closed
1 task
@shunsukew shunsukew mentioned this pull request Oct 30, 2022
Merged
5 tasks
@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Closed
@github-actions github-actions bot mentioned this pull request Nov 23, 2022
Closed
@hussein-aitlahcen hussein-aitlahcen mentioned this pull request Jan 16, 2023
Merged
7 tasks
vimukthi-git pushed a commit to ComposableFi/composable that referenced this pull request Jan 18, 2023
@hussein-aitlahcen @cor
[PICA-34] - Upgrade to polkadot v0.9.30 (#2913)
2ec221a 
#### Intro

Upgrade polkadot from v0.9.27 to v0.9.30 as a checkpoint, then to
v0.9.33 (latest version without workspace dependencies)

Require ComposableFi/composable-ibc#176 to be merged
first.

#### Migrations
- v0.9.28
    - [x] paritytech/polkadot#5582
        - Nomination not present in our runtimes
- v0.9.29
    - [x] paritytech/substrate#12095
        - Nomination not present in our runtimes
- v0.9.30
    - [x] paritytech/substrate#12034
        - BagList/Staking not present in our runtimes
    - [x] paritytech/polkadot#5930
        - Nomination/BagList/Staking not present in our runtimes
    - [x] paritytech/substrate#12230
        - Staking not present in our runtimes
    - [x] paritytech/polkadot#5996
        - Staking not present in our runtimes
    - [x] paritytech/substrate#12083
        - Contracts not present in our runtimes

Signed-off-by: cor <cor@pruijs.dev>
Co-authored-by: cor <cor@pruijs.dev>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@athei @agryaznov @ark0f
Make automatic storage deposits resistant against changing deposit pr…
652c65a 
…ices (paritytech#12083)

* Require `FixedPointOperand` for Balances

* Delay deposit calculation

* Make refunds pro rata of consumed storage

* Add storage migration

* Fix clippy

* Add liquidity checks

* Fixe delayed deposit limit enforcement

* Defer charges

* Import Vec

* Add try-runtime hooks for migration

* Fix warning

* Adapt to new OnRuntimeUpgrade trait

* Apply suggestions from code review

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

* fmt

* Apply suggestions from code review

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

* More suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@agryaznov agryaznov agryaznov left review comments

@shawntabrizi shawntabrizi shawntabrizi left review comments

@HCastano HCastano HCastano approved these changes

@cmichi cmichi cmichi approved these changes

@ascjones ascjones Awaiting requested review from ascjones

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Smart Contracts
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make automatic storage deposits resistant against changing deposit prices
5 participants
@athei @cmichi @shawntabrizi @agryaznov @HCastano