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

Upgradeable contracts #1889

Merged
merged 9 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .config/cargo_spellcheck.dic
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ decodable
decrement
defrag
defragmentation
delegatee
delegator
deploy
dereferencing
deserialize/S
Expand Down
33 changes: 27 additions & 6 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ variables:
ALL_CRATES: "${PURELY_STD_CRATES} ${ALSO_WASM_CRATES}"
MULTI_CONTRACT_CALLER_SUBCONTRACTS: "accumulator adder subber"
LANG_ERR_INTEGRATION_CONTRACTS: "integration-flipper call-builder contract-ref constructors-return-value"
UPGRADEABLE_CONTRACTS: "delegator set-code-hash"
# TODO `cargo clippy --verbose --all-targets --all-features` for this crate
# currently fails on `stable`, but succeeds on `nightly`. This is due to
# this fix not yet in stable: https://github.com/rust-lang/rust-clippy/issues/8895.
Expand Down Expand Up @@ -120,15 +121,18 @@ examples-fmt:
# Note that we disable the license header check for the examples, since they are unlicensed.
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
cargo +nightly fmt --verbose --manifest-path ${example}/Cargo.toml -- --check;
done
- for contract in ${MULTI_CONTRACT_CALLER_SUBCONTRACTS}; do
cargo +nightly fmt --verbose --manifest-path ./integration-tests/multi-contract-caller/${contract}/Cargo.toml -- --check;
done
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo +nightly fmt --verbose --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml -- --check;
done
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo +nightly fmt --verbose --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml -- --check;
done
- cargo +nightly fmt --verbose --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml -- --check
# This file is not a part of the cargo project, so it wouldn't be formatted the usual way
- rustfmt +nightly --verbose --check ./integration-tests/psp22-extension/runtime/psp22-extension-example.rs
allow_failure: true
Expand Down Expand Up @@ -159,15 +163,18 @@ examples-clippy-std:
script:
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
cargo clippy --verbose --all-targets --manifest-path ${example}/Cargo.toml -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${MULTI_CONTRACT_CALLER_SUBCONTRACTS}; do
cargo clippy --verbose --all-targets --manifest-path ./integration-tests/multi-contract-caller/${contract}/Cargo.toml -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo clippy --verbose --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo clippy --verbose --all-targets --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml -- -D warnings -A $CLIPPY_ALLOWED;
done
- cargo clippy --verbose --all-targets --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml -- -D warnings -A $CLIPPY_ALLOWED;
allow_failure: true

examples-clippy-wasm:
Expand All @@ -177,15 +184,19 @@ examples-clippy-wasm:
script:
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
cargo clippy --verbose --manifest-path ${example}/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${MULTI_CONTRACT_CALLER_SUBCONTRACTS}; do
cargo clippy --verbose --manifest-path ./integration-tests/multi-contract-caller/${contract}/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo clippy --verbose --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
done
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo clippy --verbose --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
done
- cargo clippy --verbose --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
- cargo clippy --verbose --manifest-path ./integration-tests/upgradeable-contracts/set-code-hash/updated-incrementer/Cargo.toml --no-default-features --target wasm32-unknown-unknown -- -D warnings -A $CLIPPY_ALLOWED;
allow_failure: true


Expand Down Expand Up @@ -378,6 +389,7 @@ examples-test:
script:
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
if [ "$example" = "integration-tests/conditional-compilation/" ]; then
cargo test --verbose --manifest-path ${example}/Cargo.toml --features "foo";
cargo test --verbose --manifest-path ${example}/Cargo.toml --features "bar";
Expand All @@ -396,13 +408,15 @@ examples-test:
- for contract in ${MULTI_CONTRACT_CALLER_SUBCONTRACTS}; do
cargo test --verbose --manifest-path ./integration-tests/multi-contract-caller/${contract}/Cargo.toml;
done
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo test --verbose --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml --features e2e-tests;
done
# TODO (#1502): We need to clean before running, otherwise the CI fails with a
# linking error.
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo clean --verbose --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml;
cargo test --verbose --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml --features e2e-tests;
done
- cargo test --verbose --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml;

examples-contract-build:
stage: examples
Expand All @@ -416,6 +430,7 @@ examples-contract-build:
- cargo contract -V
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
if [ "$example" = "integration-tests/conditional-compilation/" ]; then
pushd $example &&
cargo contract build --features "foo" &&
Expand All @@ -435,7 +450,9 @@ examples-contract-build:
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo contract build --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml;
done
- cargo contract build --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo contract build --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml;
done

# TODO: Use cargo contract as soon as it has RISC-V support
examples-contract-build-riscv:
Expand Down Expand Up @@ -478,15 +495,19 @@ examples-docs:
# of this flag.
- for example in integration-tests/*/; do
if [ "$example" = "integration-tests/lang-err-integration-tests/" ]; then continue; fi;
if [ "$example" = "integration-tests/upgradeable-contracts/" ]; then continue; fi;
cargo doc --manifest-path ${example}/Cargo.toml --document-private-items --verbose --no-deps;
done
- for contract in ${MULTI_CONTRACT_CALLER_SUBCONTRACTS}; do
cargo doc --manifest-path ./integration-tests/multi-contract-caller/${contract}/Cargo.toml --document-private-items --verbose --no-deps;
done
- for contract in ${UPGRADEABLE_CONTRACTS}; do
cargo doc --manifest-path ./integration-tests/upgradeable-contracts/${contract}/Cargo.toml --document-private-items --verbose --no-deps;
done
- for contract in ${LANG_ERR_INTEGRATION_CONTRACTS}; do
cargo doc --manifest-path ./integration-tests/lang-err-integration-tests/${contract}/Cargo.toml --document-private-items --verbose --no-deps;
done
- cargo doc --manifest-path ./integration-tests/set-code-hash/updated-incrementer/Cargo.toml --document-private-items --verbose --no-deps
- cargo doc --manifest-path ./integration-tests/upgradeable-contracts/set-code-hash/updated-incrementer/Cargo.toml --document-private-items --verbose --no-deps


#### stage: ink-waterfall
Expand Down
36 changes: 36 additions & 0 deletions integration-tests/upgradeable-contracts/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Upgradeable Contracts

There are different ways a contract can be upgraded in ink!

This folder illustrates some of the common and best practices to achieve upgradeability in your contracts.

## [`set-code-hash`](set-code-hash/)

ink! provides an ability to replace the code under the given contract's address.
This is exactly what `set_code_hash()` function does.

However, developers needs to be mindful of storage compatibility.
You can read more about storage compatibility on [use.ink](https://use.ink/basics/upgradeable-contracts#replacing-contract-code-with-set_code_hash)

## [Delegator](delegator/)

Delegator patter is based around a low level cross contract call function `delegate_call`.
It allows a contract to delegate its execution to some on-chain uploaded code.
Comment on lines +15 to +18
Copy link

Choose a reason for hiding this comment

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

I think it's a mistake to present these as two separate ways of upgrading storage - set_code_hash is almost always used together with delegate_call to migrate the contract's storage after upgrading its code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While they can be used together, it is also possible to upgrade the storage using only set_code_hash and delegate_call. These are the atomic blocks for upgradeability. For more detailed practises on how to upgrade the contract in the best way, there should be a documentation page

Copy link

Choose a reason for hiding this comment

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

delegate_call doesn't upgrade the contract - i.e. it doesn't change it's logic (one could argue that if all calls delegate then it is but I'd say it's a proxy then) it can only change its data. Hence the only way to upgrade the contract is to:

  1. forward to a different contract - proxy pattern
  2. replace the code - set_code_hash.

Also, you can only delegate if that logic has been there since the beginning - i.e. since the moment the contract was uploaded. I'd argue that the contract isn't upgraded .

I think the nomenclature is important here b/c if used loosely it will only add to devs confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

delegate_call allows to delegate an execution of logic to another on-chain. If used correctly, you can introduce the upgradeability functionality where delegated code hash can be replaced. The reason why it belongs to upgradeability is because that's how it is implemented in the EVM world - via proxies.

set_code_hash is the "Polkadot" way of upgradability.

Nonetheless, these nuances should be elaborated in the documentation. The examples just provide a concrete illustration of how these "blocks" can be used individually.


It is different from a traditional cross-contract call
because the call is delegate to the **code**, not the contract.

Similarly, the storage compatibility issue is also applicable here.
However, there are certain nuances associated with using `delegate_call`.

First of all, as demonstrated in the example, if the delegated code intends to mutate the caller's storage,
a developer needs to be mindful. If the delegated code modifies layoutful storage
(i.e. it contains at least non-`Lazy`, non-`Mapping` field), the `.set_tail_call(true)` flag of `CallFlags` needs to be specified and the storage layouts must match.
This is due to the way ink! execution call stack is operated
(see [StackExchange Answer](https://substrate.stackexchange.com/a/3352/3098) for more explanation).

If the delegated code only modifies `Lazy` or `Mapping` field, the keys must be identical and `.set_tail_call(true)` is optional.
This is because `Lazy` and `Mapping` interact with the storage directly instead of loading and flushing storage states.

If your storage is completely layoutless (it only contains `Lazy` and `Mapping` fields), the order of fields and layout do not need to match for the same reason as mentioned above.
Copy link

@deuszx deuszx Aug 29, 2023

Choose a reason for hiding this comment

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

I'd avoid usage of layoutfull and layoutless since they don't have well-defined meaning. Storage under Lazy and Mapping also has a layout, it's just different. Using new terms will only confuse users even more and they're already have a lot to swallow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is hard to explain in terms of Packed and Non-Packed because storage can be Non-Packed by still layoutful. Here the "layoutfulness" plays an important role.


Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Ignore build artifacts from the local tests sub-crate.
/target/

# Ignore backup files creates by cargo fmt.
**/*.rs.bk

# Remove Cargo.lock when creating an executable, leave it for libraries
# More information here http://doc.crates.io/guide.html#cargotoml-vs-cargolock
Cargo.lock
29 changes: 29 additions & 0 deletions integration-tests/upgradeable-contracts/delegator/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
[package]
name = "delegator"
version = "4.2.0"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2021"
publish = false

[dependencies]
ink = { path = "../../../crates/ink", default-features = false }
delegatee = { path = "delegatee", default-features = false, features = ["ink-as-dependency"] }

scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
scale-info = { version = "2.6", default-features = false, features = ["derive"], optional = true }

[dev-dependencies]
ink_e2e = { path = "../../../crates/e2e" }

[lib]
path = "lib.rs"

[features]
default = ["std"]
std = [
"ink/std",
"scale/std",
"scale-info/std",
]
ink-as-dependency = []
e2e-tests = []
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Ignore build artifacts from the local tests sub-crate.
/target/

# Ignore backup files creates by cargo fmt.
**/*.rs.bk

# Remove Cargo.lock when creating an executable, leave it for libraries
# More information here http://doc.crates.io/guide.html#cargotoml-vs-cargolock
Cargo.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
[package]
name = "delegatee"
version = "4.2.0"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2021"
publish = false

[dependencies]
ink = { path = "../../../../crates/ink", default-features = false }

scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
scale-info = { version = "2.6", default-features = false, features = ["derive"], optional = true }

[lib]
path = "lib.rs"

[features]
default = ["std"]
std = [
"ink/std",
"scale/std",
"scale-info/std",
]
ink-as-dependency = []
e2e-tests = []
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![cfg_attr(not(feature = "std"), no_std, no_main)]

#[ink::contract]
pub mod delegatee {
use ink::storage::{Mapping, traits::ManualKey};
#[ink(storage)]
pub struct Delegatee {
addresses: Mapping<AccountId, i32, ManualKey<0x23>>,
counter: i32,
// Uncommenting below line will break storage compatibility.
// flag: bool,
}

impl Delegatee {
/// When using the delegate call. You only upload the code of the delegatee contract.
/// However, the code and storage do not get initialized.
///
/// Because of this. The constructor actually never gets called.
#[allow(clippy::new_without_default)]
#[ink(constructor)]
pub fn new() -> Self {
unreachable!(
"Constructors are not called when upgrading using `set_code_hash`."
)
}

/// Increments the current value.
#[ink(message)]
pub fn inc(&mut self) {
self.counter += 2;
}


/// Adds current value of counter to the `addresses`
#[ink(message)]
pub fn append_address_value(&mut self) {
let caller = self.env().caller();
self.addresses.insert(caller, &self.counter);
}
}
}
Loading