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

Include pallet-contracts into the runtime #1012

Merged
merged 22 commits into from Jun 21, 2023

Conversation

sea212
Copy link
Member

@sea212 sea212 commented May 28, 2023

Includes pallet-contracts into the runtime and uses as sensible configuration based on the reference contracts runtime "kitchensink".
Alex@Parity:

Just look at the kitchensink runtime. It uses our recommended Config.

Runtime benchmarks and apis have been added as well. Proper weights will be added during preparation of the release.

The contract upload is permissioned on the mainnet. The root origin has to dispatch the upload on behalf of an account using utility.dispatch_as.

Contracts was tested on a dev instance of the chain. Everything works as expected.

@sea212 sea212 added the s:in-progress The pull requests is currently being worked on label May 28, 2023
@sea212 sea212 added this to the v0.3.9 milestone May 28, 2023
@sea212 sea212 self-assigned this May 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2023

Codecov Report

Merging #1012 (fa7c6b5) into main (5f8602c) will not change coverage.
The diff coverage is n/a.

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

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main    #1012   +/-   ##
=======================================
  Coverage   94.46%   94.46%           
=======================================
  Files          93       93           
  Lines       21177    21177           
=======================================
  Hits        20004    20004           
  Misses       1173     1173           
Flag Coverage Δ
tests 94.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sea212 sea212 added i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version labels Jun 16, 2023
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Jun 16, 2023
Comment on lines 149 to 154
RuntimeCall::Contracts(inner_call) => match inner_call {
instantiate_with_code { .. } => false,
instantiate_with_code_old_weight { .. } => false,
upload_code { .. } => false,
_ => true,
},
Copy link

@pgherveou pgherveou Jun 16, 2023

Choose a reason for hiding this comment

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

I would flip the logic here and specify the one you want to authorize (I assume just call) and then use _ => false to deny everything else

Copy link
Member Author

Choose a reason for hiding this comment

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

runtime/common/src/lib.rs Show resolved Hide resolved
runtime/battery-station/src/parameters.rs Outdated Show resolved Hide resolved
runtime/battery-station/src/parameters.rs Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
Comment on lines 148 to 154
// Permissioned contracts: Only deployable via utility.dispatch_as(...)
RuntimeCall::Contracts(inner_call) => match inner_call {
instantiate_with_code { .. } => false,
instantiate_with_code_old_weight { .. } => false,
upload_code { .. } => false,
_ => true,
},
Copy link
Member

Choose a reason for hiding this comment

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

Will later look into this more specific to find potential calls that are not covered.

runtime/zeitgeist/src/parameters.rs Show resolved Hide resolved
runtime/zeitgeist/src/parameters.rs Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
runtime/zeitgeist/src/lib.rs Show resolved Hide resolved
@Chralt98 Chralt98 self-requested a review June 20, 2023 11:43
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Please regard the "approve" comment above.

EDIT: everything is fine

@Chralt98 Chralt98 self-requested a review June 20, 2023 13:17
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jun 21, 2023
@sea212 sea212 merged commit 663388a into main Jun 21, 2023
10 checks passed
@sea212 sea212 deleted the sea212-add-smart-contract-compatibility branch June 21, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants