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

Upgradeable contracts #1889

merged 9 commits into from
Aug 28, 2023

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Aug 24, 2023

Addresses #1826

Turns out there are a few hiccups when using delegate_call that a develop needs to know. This PR re-introduces the examples and documentation for the upgradeable contracts.

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023

Codecov Report

Merging #1889 (3e674f7) into master (e37f1f2) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head 3e674f7 differs from pull request most recent head b6434c4. Consider uploading reports for the commit b6434c4 to get more accurate results

@@            Coverage Diff             @@
##           master    #1889      +/-   ##
==========================================
- Coverage   52.97%   52.96%   -0.02%     
==========================================
  Files         215      215              
  Lines        6782     6782              
==========================================
- Hits         3593     3592       -1     
- Misses       3189     3190       +1     

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SkymanOne SkymanOne linked an issue Aug 25, 2023 that may be closed by this pull request
@SkymanOne SkymanOne marked this pull request as ready for review August 25, 2023 19:33
@SkymanOne SkymanOne requested review from cmichi, ascjones and a team as code owners August 25, 2023 19:33
@SkymanOne SkymanOne requested a review from xgreenx August 25, 2023 19:34
Comment on lines +66 to +67
/// Note that we don't need `set_tail_call(true)` flag
/// because `Mapping` updates the storage instantly on-demand.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we want to elaborate more on that part. Maybe put a link to the documentation that describes what it means to update the state in the RAM vs Storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have such documentation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, but maybe we want to add something like this=)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's on my task-list to add a page on the use.ink

@SkymanOne SkymanOne merged commit 9b39318 into master Aug 28, 2023
@SkymanOne SkymanOne deleted the gn/delegator-example branch August 28, 2023 16:44
Comment on lines +15 to +18
## [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.
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.

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.

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.

DelegateCall does not mutate caller's layoutful storage
4 participants