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

Support call_runtime #1641

Merged
merged 60 commits into from
Feb 27, 2023

Conversation

pmikolajczyk41
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 commented Feb 3, 2023

This PR is the first step towards supporting runtime calls directly from a contract.

Passing call

For now, we settle up with a bit inconvenient approach, where the contract is responsible for providing properly encoded call. As the next step we propose to extend Environment trait with (feature-gated) type RuntimeCall, which could be then set up to the proper enum generated by e.g. subxt. Before this however, we would like to have custom environment support in the e2e framework.

Testing locally

Launch substrate-contracts-node with type CallFilter = frame_support::traits::Everything; in the runtime declaration (for pallet_contracts configuration).

Closes #854

polkadot address: 15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY

@pmikolajczyk41 pmikolajczyk41 changed the title [WIP] Support call_runtime Support call_runtime Feb 6, 2023
@pmikolajczyk41 pmikolajczyk41 marked this pull request as ready for review February 6, 2023 14:07
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Can you please add entry in CHANGELOG.md under Unreleased section and README.md for the example?

@pmikolajczyk41
Copy link
Member Author

@SkymanOne I've added a line in the CHANGELOG and a README file for the example

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2023

Codecov Report

Merging #1641 (4e69291) into master (e144bec) will decrease coverage by 6.61%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master    #1641      +/-   ##
==========================================
- Coverage   70.76%   64.15%   -6.61%     
==========================================
  Files         206      206              
  Lines        6416     6394      -22     
==========================================
- Hits         4540     4102     -438     
- Misses       1876     2292     +416     
Impacted Files Coverage Δ
crates/env/src/api.rs 34.78% <ø> (ø)
crates/env/src/backend.rs 78.12% <ø> (ø)
crates/env/src/engine/off_chain/impls.rs 46.28% <ø> (ø)
crates/ink/src/env_access.rs 9.09% <ø> (ø)
crates/ink/ir/src/ir/contract.rs 0.00% <0.00%> (-91.67%) ⬇️
crates/ink/ir/src/ir/ink_test.rs 0.00% <0.00%> (-87.50%) ⬇️
crates/ink/ir/src/ir/trait_def/mod.rs 0.00% <0.00%> (-81.82%) ⬇️
crates/ink/ir/src/ir/storage_item/mod.rs 0.00% <0.00%> (-72.92%) ⬇️
crates/ink/ir/src/ir/storage_item/config.rs 0.00% <0.00%> (-68.75%) ⬇️
crates/ink/ir/src/ir/blake2.rs 20.00% <0.00%> (-60.00%) ⬇️
... and 30 more

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

Co-authored-by: German <[email protected]>
examples/call-runtime/lib.rs Outdated Show resolved Hide resolved
examples/call-runtime/lib.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor

Also could you please satisfy the CI? It seems that it fails on the e2e test added in this very PR.

@pmikolajczyk41
Copy link
Member Author

I have no idea why linker fails :|

@agryaznov
Copy link
Contributor

agryaznov commented Feb 17, 2023

I have no idea why linker fails :|

Alright, this wasmtime issue might cause the failure. Could be solved by changing its version down there in the dependency tree. Or, just wait for the recent substrate crates get published to crates.io (will require version bumps in ink_e2e dependencies)

@pmikolajczyk41
Copy link
Member Author

pmikolajczyk41 commented Feb 17, 2023

@agryaznov sorry, but I don't know how to properly add this wasmtime dependency (adding wasmtime = { version = "5.0.0", default-features = false, git = "https://github.com/paritytech/wasmtime.git", branch = "v5.0.0_lto_fix" } to e2e crate doesn't help :|

@HCastano
Copy link
Contributor

@agryaznov are you gonna take care of helping out wit the CI here, or do you need help from me?

Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

@pmikolajczyk41 to fix the CI bump versions in your e2e test's Cargo.toml as suggested.

Also, note that those tests have been moved to another directory (see #1659), so you need to merge master into your branch.


scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
scale-info = { version = "2.3", default-features = false, features = ["derive"], optional = true }
sp-io = { version = "12.0.0", default-features = false, features = ["disable_panic_handler", "disable_oom", "disable_allocator"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is pretty ugly.

@HCastano I've created an issue to investigate it: #1681

Feel free to dig into it

examples/call-runtime/Cargo.toml Outdated Show resolved Hide resolved
examples/call-runtime/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

A final patch: please, update the changelog as requested. Aside from that, LGTM! Thanks a lot for your contribution!

CHANGELOG.md Show resolved Hide resolved
@agryaznov agryaznov merged commit 93ee7fb into use-ink:master Feb 27, 2023
@pmikolajczyk41 pmikolajczyk41 deleted the pmikolajczyk41/call_runtime branch February 27, 2023 13:36
@cmichi
Copy link
Collaborator

cmichi commented Feb 27, 2023

@pmikolajczyk41 Thank you for this nice contribution! We would like to give you a tip for the work you've put into this PR 😊 .

Could you edit your description of this PR and add either one of those?

polkadot address: <SS58 Address>

or

kusama address: <SS58 Address>

@pmikolajczyk41
Copy link
Member Author

@cmichi address added - thank you!

@cmichi
Copy link
Collaborator

cmichi commented Feb 27, 2023

/tip medium

@substrate-tip-bot
Copy link

@cmichi A medium tip was successfully submitted for pmikolajczyk41 (15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips tip

@@ -50,7 +53,6 @@ compatible with the ink! `4.0.0` release.

For full compatibility requirements see the [migration guide](https://use.ink/faq/migrating-from-ink-3-to-4/#compatibility).

### Added
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, why was this removed? Looks accidental @pmikolajczyk41 .

Copy link
Member Author

Choose a reason for hiding this comment

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

I must have done this during some semi-automatic merging - that explains the last request for change above :|

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.

Implement seal_call_runtime
7 participants