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

contracts: Reduce the API surface #8359

Merged
9 commits merged into from
Mar 24, 2021
Merged

contracts: Reduce the API surface #8359

9 commits merged into from
Mar 24, 2021

Conversation

athei
Copy link
Member

@athei athei commented Mar 15, 2021

Originally, all storage items and the fields of the Schedule data structure were public. This means that every minor change like adding a new contract callable function would require releasing a new major version. This is because every contract callable function requires a new entry in the Schedule.

With this PR users of the crate are required to construct the Schedule through its Default implementation. Changes to this default constructed Schedule can be done by calling associated function on the Schedule. This allows for changes to the Schedule without releasing a new major version. When the need arises we will add further functions to the Schedule or convert it to a builder pattern.

The same is true for other storage items. When someone comes forward with a use case for accessing the raw storage of pallet_contracts we will consider adding a public function to do so. But by default the data should be private in order to control the public API and prevent churning through major versions.

@athei athei added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Mar 15, 2021
@athei athei added this to In Progress in Smart Contracts via automation Mar 15, 2021
@athei athei added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 22, 2021
Copy link
Contributor

@thiolliere thiolliere left a comment

Choose a reason for hiding this comment

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

changes are good.

But it is a bit unexpect to me to have Schedule fields not part of the public API of the crate.

frame/contracts/src/schedule.rs Show resolved Hide resolved
@athei athei added D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 23, 2021
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks fine to me, I prefer your move of all this data to a Config trait, so looking forward to that next.

@athei
Copy link
Member Author

athei commented Mar 24, 2021

bot merge

@ghost
Copy link

ghost commented Mar 24, 2021

Trying merge.

@ghost ghost merged commit 30ad918 into master Mar 24, 2021
Smart Contracts automation moved this from In Progress to Done Mar 24, 2021
@ghost ghost deleted the at-contracts-api branch March 24, 2021 08:09
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* contracts: Remove types and storage from the public interface

* contracts: Remove current_schedule() getter

* contracts: Improve documentation

* Update README.md

* Fix integration test
This pull request was closed.
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. 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
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants