Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add integration-test for possible migration pattern #1909

Merged
merged 20 commits into from
Jan 31, 2024
Merged

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Sep 13, 2023

Here I add an example that performs a storage migration with the following flow:

  1. Upload a migration contract with a message migrate which performs the storage migration.
  2. Set code hash to the migration contract.
  3. Upload a new version of the incrementer contract.
  4. Call migrate on the migration contract, passing the code hash of the new updated incrementer contract from 3. This must happen as a single message, because following the storage migration, the contract would not be able to be called again, since it will fail to load the migrated storage.

I imagine that a similar pattern could be applied using delegate_call instead of set_code_hash?

The migrate message looks like:

#[ink(message)]
pub fn migrate(&self, inc_by: u8, code_hash: Hash) {
    let incrementer_new = IncrementerNew {
        count: self.count as u64,
        inc_by,
    };
    const STORAGE_KEY: u32 = 0x00000000;
    ink::env::set_contract_storage(&STORAGE_KEY, &incrementer_new);

    ink::env::set_code_hash::<<Self as ink::env::ContractEnv>::Env>(&code_hash)
        .unwrap_or_else(|err| {
            panic!("Failed to `set_code_hash` to {code_hash:?} due to {err:?}")
        })
}

Note the use of &self instead than &mut self, ensuring that the storage will not be written to automatically following the invocation.

I have added this to demonstrate how we might perform a migration as it stands, without having to add any extra hooks in the language, as mentioned here #1388 (comment).

Also to demonstrate that it would still be possible to do a migration without using _reserved: Option<()> trick for schema evolution as suggested by @athei in relation to #1897

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5674b4d) 53.69% compared to head (2e8b86b) 53.71%.

❗ Current head 2e8b86b differs from pull request most recent head f7d0903. Consider uploading reports for the commit f7d0903 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1909      +/-   ##
==========================================
+ Coverage   53.69%   53.71%   +0.01%     
==========================================
  Files         223      223              
  Lines        7043     7043              
  Branches     3121     3121              
==========================================
+ Hits         3782     3783       +1     
+ Misses       3261     3260       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SkymanOne
Copy link
Contributor

SkymanOne commented Oct 24, 2023

My personal take on the migration pattern:

While this approach works, it is only feasible for trivial storage migrations. As shown in the example, we are migrating a simple, Packed, layoutful storage with 1-2 primitive fields. In production contracts, storage is much more complex, it can have multiple custom data types, Mappings, Lazy fields. Migrating this type of storage using raw operations on the storage cells is tedious and error-prone.

While storage migration is an "advanced" concept, we should still discourage developers from raw storage operations, and leave it for use only for some niche use cases, similar to how solidity devs are not advised to use EVM opcodes on a regular basis.
The whole contract gets bricked if a developer gets at least one storage cell migration wrong. It is also extremely hard to audit this kind of storage migration.

What I am proposing instead is to look into #1388 and define a migration API for the developers to work with the concrete storage layouts instead. The concept is very similar to the database migration in ORM libraries like this: https://www.sea-ql.org/SeaORM/docs/next/migration/writing-migration/

While this is obviously more work, I strongly believe, that if we "market" the "native" contract's upgradeability, we should also provide a safe workflow with it to lower the entry point of using the feature.

@paritytech-cicd-pr
Copy link

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-e8c2cfc and comparing them to ink! master:

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 2.967 2.967 0 0
basic-contract-caller/other-contract 1.337 1.337 0 0
call-builder-return-value 8.688 8.688 0 0
call-runtime 1.769 1.769 0 0
conditional-compilation 1.209 1.209 0 0
contract-storage 7.124 7.124 0 0
contract-terminate 1.092 1.092 0 0
contract-transfer 1.444 1.444 0 0
custom-allocator 7.428 7.428 0 0
dns 7.249 7.249 0 0
e2e-call-runtime 1.058 1.058 0 0
e2e-runtime-only-backend 1.635 1.635 0 0
erc1155 14.026 14.02 -0.006 -0.0427777 📉
erc20 6.687 6.687 0 0
erc721 9.64 9.64 0 0
events 4.763 4.763 0 0
flipper 1.393 1.393 0 0
incrementer 1.221 1.221 0 0
lang-err-integration-tests/call-builder-delegate 2.317 2.317 0 0
lang-err-integration-tests/call-builder 4.847 4.847 0 0
lang-err-integration-tests/constructors-return-value 1.773 1.773 0 0
lang-err-integration-tests/contract-ref 4.328 4.328 0 0
lang-err-integration-tests/integration-flipper 1.571 1.571 0 0
mapping-integration-tests 3.203 3.203 0 0
mother 9.508 9.508 0 0
multi-contract-caller 5.924 5.924 0 0
multi-contract-caller/accumulator 1.095 1.095 0 0
multi-contract-caller/adder 1.669 1.669 0 0
multi-contract-caller/subber 1.689 1.689 0 0
multisig 21.471 21.471 0 0
payment-channel 5.501 5.501 0 0
sr25519-verification 0.865 0.865 0 0
static-buffer 1.405 1.405 0 0
trait-dyn-cross-contract-calls 2.466 2.466 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.305 1.305 0 0
trait-erc20 7.063 7.063 0 0
trait-flipper 1.209 1.209 0 0
trait-incrementer 1.37 1.37 0 0
upgradeable-contracts/delegator 2.908 2.908 0 0
upgradeable-contracts/delegator/delegatee 1.369 1.369 0 0
upgradeable-contracts/set-code-hash 1.464 1.464 0 0
upgradeable-contracts/set-code-hash/updated-incrementer 1.443 1.443 0 0
wildcard-selector 2.622 2.622 0 0

Link to the run | Last update: Thu Oct 26 11:34:47 CEST 2023

@ascjones
Copy link
Collaborator Author

ascjones commented Nov 8, 2023

See IIP-3 #1981 for a suggestion to make this better, specifically to allow multi-step data migration and contract upgrades, without the risk of bricking the contract.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

This is great example!

@cmichi
Copy link
Collaborator

cmichi commented Jan 30, 2024

Please add an entry here: https://github.com/paritytech/ink/tree/master/integration-tests/upgradeable-contracts.

…n/lib.rs

Co-authored-by: Michael Müller <michi@parity.io>
ascjones added a commit that referenced this pull request Jan 31, 2024
@ascjones ascjones enabled auto-merge (squash) January 31, 2024 11:52
@ascjones ascjones merged commit 8808da0 into master Jan 31, 2024
23 checks passed
@ascjones ascjones deleted the aj/upgrade-migrate branch January 31, 2024 12:07
Copy link

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract and comparing them to ink! master:

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 3.303 3.303 0 0
basic-contract-caller/other-contract 1.583 1.583 0 0
call-builder-return-value 9.169 9.169 0 0
call-runtime 2.061 2.061 0 0
combined-extension 2.137 2.12 -0.017 -0.795508 📉
conditional-compilation 1.49 1.49 0 0
contract-storage 7.568 7.568 0 0
contract-terminate 1.329 1.329 0 0
contract-transfer 1.689 1.689 0 0
custom-allocator 7.775 7.775 0 0
custom-environment 2.146 2.146 0 0
dns 7.318 7.318 0 0
e2e-call-runtime 1.296 1.296 0 0
e2e-runtime-only-backend 1.881 1.881 0 0
erc1155 14.237 14.237 0 0
erc20 6.918 6.918 0 0
erc721 9.873 9.873 0 0
events 5.258 5.258 0 0
flipper 1.639 1.639 0 0
incrementer 1.504 1.504 0 0
lang-err-integration-tests/call-builder-delegate 2.638 2.638 0 0
lang-err-integration-tests/call-builder 5.066 5.066 0 0
lang-err-integration-tests/constructors-return-value 1.985 1.985 0 0
lang-err-integration-tests/contract-ref 4.629 4.629 0 0
lang-err-integration-tests/integration-flipper 1.815 1.815 0 0
lazyvec-integration-test 4.616 4.616 0 0
mapping-integration-tests 7.999 7.999 0 0
mother 12.741 12.741 0 0
multi-contract-caller 6.106 6.106 0 0
multi-contract-caller/accumulator 1.378 1.378 0 0
multi-contract-caller/adder 1.968 1.968 0 0
multi-contract-caller/subber 1.988 1.988 0 0
multisig 21.688 21.688 0 0
payment-channel 5.659 5.659 0 0
psp22-extension 7.071 7.071 0 0
rand-extension 2.965 2.965 0 0
sr25519-verification 1.142 1.142 0 0
static-buffer 1.654 1.654 0 0
trait-dyn-cross-contract-calls 2.793 2.793 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.545 1.545 0 0
trait-erc20 7.294 7.294 0 0
trait-flipper 1.49 1.49 0 0
trait-incrementer 1.614 1.614 0 0
upgradeable-contracts/delegator 3.151 3.151 0 0
upgradeable-contracts/delegator/delegatee 1.609 1.609 0 0
upgradeable-contracts/set-code-hash-migration 1.743 1.743 0 0
upgradeable-contracts/set-code-hash-migration/migration 1.45 1.45 0 0
upgradeable-contracts/set-code-hash-migration/updated-incrementer 1.897 1.897 0 0
upgradeable-contracts/set-code-hash 1.743 1.743 0 0
upgradeable-contracts/set-code-hash/updated-incrementer 1.721 1.721 0 0
wildcard-selector 2.846 2.846 0 0

Link to the run | Last update: Wed Jan 31 13:34:18 CET 2024

ascjones added a commit that referenced this pull request Jan 31, 2024
* add e2e tests to basic-contract-caller example

* Fix basic-contract-caller e2e tests

* Remove `call_builder` change

* Remove `basic_contract_caller` integration test, moved to #1909

* Revert "Remove `basic_contract_caller` integration test, moved to #1909"

This reverts commit 8f3ab31.

* fmt
ascjones added a commit that referenced this pull request Feb 1, 2024
* WIP

* Update versions

* WIP

* WIP migration

* WIP

* Make test pass

* Move e2e tests mod to own file

* Update comment

* Update example for new e2e API

* Update integration-tests/upgradeable-contracts/set-code-hash-migration/lib.rs

Co-authored-by: Michael Müller <michi@parity.io>

* Top level gitignore

* Fix tests update comments

* Update upgradeable contracts README.md

* spelling

---------

Co-authored-by: Michael Müller <michi@parity.io>
ascjones added a commit that referenced this pull request Feb 1, 2024
* add e2e tests to basic-contract-caller example

* Fix basic-contract-caller e2e tests

* Remove `call_builder` change

* Remove `basic_contract_caller` integration test, moved to #1909

* Revert "Remove `basic_contract_caller` integration test, moved to #1909"

This reverts commit 8f3ab31.

* fmt
ascjones added a commit that referenced this pull request Feb 8, 2024
* init call_v2 methods and types

* add e2e tests to basic-contract-caller example

* Add storage_deposit

* Fix basic-contract-caller e2e tests

* Remove `basic_contract_caller` integration test, moved to #1909

* WIP adding cross_contract_calls test

* Add `integration-test` for possible migration pattern (#1909)

* WIP

* Update versions

* WIP

* WIP migration

* WIP

* Make test pass

* Move e2e tests mod to own file

* Update comment

* Update example for new e2e API

* Update integration-tests/upgradeable-contracts/set-code-hash-migration/lib.rs

Co-authored-by: Michael Müller <michi@parity.io>

* Top level gitignore

* Fix tests update comments

* Update upgradeable contracts README.md

* spelling

---------

Co-authored-by: Michael Müller <michi@parity.io>

* Add `basic-contract-caller` E2E test (#2085)

* add e2e tests to basic-contract-caller example

* Fix basic-contract-caller e2e tests

* Remove `call_builder` change

* Remove `basic_contract_caller` integration test, moved to #1909

* Revert "Remove `basic_contract_caller` integration test, moved to #1909"

This reverts commit 8f3ab31.

* fmt

* Only need one .gitignore

* WIP adding create v2 API

* Revert "WIP adding create v2 API"

This reverts commit bd83e18.

* WIP e2e tests

* Add CallV2 builder methods

* Pass weight limit as params

* Allow deprecated

* Add storage_deposit_limit

* Clippy

* Use struct update syntax

* Remove space

* CHANGELOG

* fmt

* Import OtherContract directly instead of reexporting

* Make other_contract pub

* Revert prev

* docs

* top level gitignore for integration-tests

* Remove unused setters

* Use ContractRef

* integration-test comments

* Rename to `call_v2`

* Comments and builder method

* SP

* comments

* fix doc test

---------

Co-authored-by: Michael Müller <michi@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants