feat: revive compatibility with feature flag #500

AlexD10S · 2025-03-28T12:25:06Z

This PR introduces support for ink! v6 smart contracts by integrating pallet-revive using feature flags for temporary support of both ink! v6 and ink! v5 smart contracts..

This PR continues the work started by Frank, who introduced feature flag support in #491, enabling users to switch between different ink! versions based on their needs. The pop-contracts crate is split via v5 (default) and v6 features. The pop-cli crate is split into wasm-contracts (default) and polkavm-contracts features

Note for reviewers:
The large number of changes is mostly due to temporary code duplication required to support both contract types. This setup is not permanent, WASM contract support will likely be removed in the coming months. However, since ink! v6 and the pallet_revive are still in a highly experimental state, they are currently kept behind a feature flag.

~~While it's probably easiest to just review this PR directly, I've also created a separate draft PR that isolates only the ink! v6 support for easier comparison and review: #493~~

Installation instructions (from Frank PR)
Default install (wasm contracts):

cargo install --path ./crates/pop-cli --locked

PolkaVM contracts only:

cargo install --path ./crates/pop-cli --locked --no-default-features -F polkavm-contracts

PolkaVM and parachain support:

cargo install --path ./crates/pop-cli --locked --no-default-features -F polkavm-contracts,parachain

[sc-2509]

crates/pop-contracts/tests/files/testing_wasm.contract

crates/pop-common/src/git.rs

crates/pop-cli/src/commands/call/contract.rs

Copilot

Pull Request Overview

This PR revives compatibility with flags by integrating support for both ink! v6 and ink! v5 smart contracts, using feature flags to conditionally compile code for PolkaVM and Wasm contracts. Key changes include updating conditional compilation attributes across tests, commands, and common modules; revising Cargo.toml dependencies and feature definitions; and altering error messages and comments to reflect the new contract support.

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/pop-cli/tests/parachain.rs	Added cfg attribute to restrict test execution to parachain feature.
crates/pop-cli/tests/contract.rs	Introduced conditional assertions to verify generation of contract artifacts.
crates/pop-cli/src/style.rs	Extended cfg attributes to include both PolkaVM and Wasm contracts.
crates/pop-cli/src/main.rs	Updated compile_error messages with revised feature flag conditions.
crates/pop-cli/src/common/wallet.rs	Modified import block for conditional wallet integration logic.
crates/pop-cli/src/common/contracts.rs	Updated binary naming constant usage in check_and_prompt function.
crates/pop-cli/src/common/builds.rs	Added cfg attribute to conditionally run tests.
crates/pop-cli/src/common/binary.rs	Refactored tests to use the dynamic binary name based on active features.
crates/pop-cli/src/commands/up/mod.rs	Replaced deprecated conditional attributes with updated feature flags.
crates/pop-cli/src/commands/up/contract.rs	Refactored contract deployment flow to support both contract types.
crates/pop-cli/src/commands/call/contract.rs	Adjusted placeholder and validation logic based on feature flags.
crates/pop-cli/src/commands/build/mod.rs	Updated feature attributes consistent with new contract flags.
crates/pop-cli/Cargo.toml	Modified dependency options and feature definitions for contracts.
Cargo.toml	Updated dependencies, added ink! v6 related packages, and refined features.

Comments suppressed due to low confidence (1)

crates/pop-cli/src/commands/up/contract.rs:330

It appears that 'map_account' is invoked twice within the contract deployment flow for the polkavm-contracts feature, which may lead to duplicate user prompts. Consider consolidating this call to ensure the account mapping check is performed only once.

map_account(instantiate_exec.opts(), &mut Cli).await?

crates/pop-cli/src/common/contracts.rs

crates/pop-cli/src/assets/index.html

crates/pop-contracts/src/node.rs

Copilot

Pull Request Overview

This PR revives compatibility with experimental ink! v6 support by introducing new feature flags and conditional configurations for both wasm‑based and PolkaVM smart contracts. Key changes include:

Updates to conditional compilation attributes and module imports to support the new "wasm‑contracts" and "polkavm‑contracts" features.
Adjustments to error messages, build scripts, command implementations, and tests to correctly handle dual smart contract backends.
Revisions to dependency definitions and feature flags in Cargo.toml to isolate ink! v5 and ink! v6 functionality.

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
crates/pop-cli/tests/parachain.rs	Added feature flag for parachain tests
crates/pop-cli/tests/contract.rs	Updated artifact checks to differentiate between wasm and polkavm builds
crates/pop-cli/src/style.rs	Expanded conditional attributes for style to support new contract features
crates/pop-cli/src/main.rs	Adjusted compile_error and module inclusions for new feature flags
crates/pop-cli/src/common/*	Updated imports and conditional imports to support new contract features
crates/pop-cli/src/commands/*	Modified command implementations and tests to use the new feature flags
crates/pop-cli/Cargo.toml	Revised dependency configurations and feature definitions
Cargo.toml	Updated dependency versions and added ink! v6-related dependencies

Comments suppressed due to low confidence (3)

crates/pop-cli/src/commands/call/contract.rs:266

[nitpick] Using conditional attributes inline within the placeholder argument may affect readability and maintainability. Consider computing the placeholder string outside the method call (e.g. using an if/else assignment based on feature flags) to improve clarity.

.placeholder(

crates/pop-cli/src/commands/up/contract.rs:330

It appears that map_account is being called twice in similar contexts. Consider consolidating the account mapping prompt to avoid duplicate prompts or unintended duplicate calls for polkavm-contracts.

map_account(instantiate_exec.opts(), &mut Cli).await?

crates/pop-cli/Cargo.toml:67

[nitpick] The new feature names 'wasm-contracts' and 'polkavm-contracts' replace the old 'contract' feature; consider updating documentation or using consistent naming (e.g. using 'contracts' consistently) to reduce potential confusion.

default = ["parachain", "telemetry", "wasm-contracts"]

crates/pop-cli/src/commands/up/contract.rs

crates/pop-cli/src/common/contracts.rs

crates/pop-cli/tests/parachain.rs

crates/pop-common/src/account_id.rs

crates/pop-contracts/src/templates.rs

crates/pop-contracts/src/up.rs

codecov · 2025-03-31T08:48:24Z

Codecov Report

Attention: Patch coverage is 63.87435% with 138 lines in your changes missing coverage. Please review.

Project coverage is 78.54%. Comparing base (59926ba) to head (8e2840f).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/pop-contracts/src/up.rs	49.49%	45 Missing and 5 partials ⚠️
crates/pop-cli/src/commands/up/contract.rs	16.27%	36 Missing ⚠️
crates/pop-cli/src/commands/call/contract.rs	70.00%	10 Missing and 11 partials ⚠️
crates/pop-cli/src/commands/build/mod.rs	0.00%	6 Missing ⚠️
crates/pop-cli/src/commands/mod.rs	87.17%	5 Missing ⚠️
crates/pop-cli/src/common/wallet.rs	0.00%	5 Missing ⚠️
crates/pop-common/src/account_id.rs	79.16%	4 Missing and 1 partial ⚠️
crates/pop-contracts/src/node.rs	77.77%	0 Missing and 4 partials ⚠️
crates/pop-common/src/git.rs	50.00%	0 Missing and 3 partials ⚠️
crates/pop-cli/src/commands/bench/pallet.rs	0.00%	0 Missing and 1 partial ⚠️
... and 2 more

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
- Coverage   79.08%   78.54%   -0.55%     
==========================================
  Files         101      101              
  Lines       23724    23864     +140     
  Branches    23724    23864     +140     
==========================================
- Hits        18763    18744      -19     
- Misses       2743     2907     +164     
+ Partials     2218     2213       -5

Files with missing lines	Coverage Δ
crates/pop-cli/src/commands/call/mod.rs	`100.00% <ø> (ø)`
crates/pop-cli/src/commands/install/mod.rs	`0.00% <ø> (ø)`
crates/pop-cli/src/commands/new/mod.rs	`100.00% <ø> (ø)`
...rates/pop-cli/src/commands/test/create_snapshot.rs	`59.80% <ø> (-30.40%)`	⬇️
crates/pop-cli/src/commands/up/mod.rs	`83.20% <100.00%> (ø)`
crates/pop-cli/src/common/binary.rs	`78.91% <100.00%> (+0.70%)`	⬆️
crates/pop-cli/src/common/builds.rs	`81.66% <ø> (ø)`
crates/pop-cli/src/common/contracts.rs	`78.72% <100.00%> (-1.08%)`	⬇️
crates/pop-cli/src/common/mod.rs	`100.00% <ø> (ø)`
crates/pop-cli/src/main.rs	`88.03% <100.00%> (ø)`
... and 19 more

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

al3mart

Not changes from my side.

I was able to instantiate a new contract, build it, deploy it and interact with it.
Tests passed and I wasn't able to spot anything that required addressing.

👌

One comment from my side is that in this PR you mention ink v5 support being potentially deprecated from the CLI. Just a thought, but I would argue that there can be users interested in having v5 compatibility.

crates/pop-contracts/src/templates.rs

AlexD10S · 2025-04-02T11:49:41Z

Blocked until stable2503 is released and both ink! and cargo-contract reach version 6 on crates.io. The only remaining task is to update the dependencies.

A separate branch feat/polkavm-compatibility has been created for external evaluation and testing, in case any dependency updates introduced in this PR cause issues.

Once this is merged, we’ll proceed with generating release v0.8.0 and updating the documentation here:
🔗 https://learn.onpop.io/contracts/guides/migrating-to-inkv6

evilrobot-01 · 2025-04-07T11:55:39Z

Can the conflicts be resolved here please?

al3mart · 2025-04-14T10:53:49Z

I noticed the following: #525

Might not be something to tackle within this PR, but something that needs attention as we roll out the revive compatibility

…res in coverage

.github/workflows/ci.yml

crates/pop-cli/src/commands/test/create_snapshot.rs

crates/pop-cli/src/wallet_integration.rs

Cargo.toml

deny.toml

crates/pop-cli/src/commands/bench/pallet.rs

Daanvdplas

Well done! Super excited to get this out!

al3mart

Looking good!
I could deploy and interact with a contract on palle-revive without problems.

Regarding the code changes not much to point out. I've left a pair of comments but I think it is good to go.

crates/pop-cli/src/commands/test/create_snapshot.rs

crates/pop-cli/Cargo.toml

AlexD10S · 2025-05-07T06:46:55Z

Can't reply directly to #500 (comment) But there is an issue created to solve that issue: #535

AlexD10S commented Mar 28, 2025

View reviewed changes

crates/pop-contracts/tests/files/testing_wasm.contract Show resolved Hide resolved

AlexD10S commented Mar 28, 2025

View reviewed changes

crates/pop-common/src/git.rs Show resolved Hide resolved

AlexD10S commented Mar 28, 2025

View reviewed changes

crates/pop-cli/src/commands/call/contract.rs Show resolved Hide resolved

AlexD10S requested a review from Copilot March 28, 2025 13:21

Copilot AI reviewed Mar 28, 2025

View reviewed changes

crates/pop-cli/src/common/contracts.rs Outdated Show resolved Hide resolved

AlexD10S commented Mar 28, 2025

View reviewed changes

crates/pop-cli/src/assets/index.html Show resolved Hide resolved

AlexD10S changed the title ~~feat: revive compatibility with flags~~ feat: revive compatibility with feature flag Mar 28, 2025

AlexD10S commented Mar 28, 2025

View reviewed changes

crates/pop-contracts/src/node.rs Show resolved Hide resolved

AlexD10S requested a review from Copilot March 28, 2025 22:50

Copilot AI reviewed Mar 28, 2025

View reviewed changes

AlexD10S added the ready-for-final-review The PR is ready for final review label Mar 30, 2025

Daanvdplas approved these changes Mar 31, 2025

View reviewed changes

AlexD10S requested a review from al3mart March 31, 2025 11:16

al3mart approved these changes Mar 31, 2025

View reviewed changes

crates/pop-contracts/src/templates.rs Outdated Show resolved Hide resolved

This was referenced Apr 1, 2025

refactor: support wasm and polkavm contracts #491

Closed

feat: revive compatibility #493

Closed

AlexD10S added blocked Blocked by a third party and removed ready-for-final-review The PR is ready for final review labels Apr 2, 2025

AlexD10S force-pushed the feat/revive-compatibility-with-flags branch 3 times, most recently from 260c23b to 597451c Compare April 9, 2025 10:41

AlexD10S force-pushed the feat/revive-compatibility-with-flags branch 4 times, most recently from 5b82d9d to aeb2662 Compare April 21, 2025 15:37

AlexD10S mentioned this pull request Apr 22, 2025

Refactor contracts related code such that we don't depend on information emitted via events when targeting pallet-revive. #525

Closed

fix: rebase formatting issue

a43dbc6

AlexD10S force-pushed the feat/revive-compatibility-with-flags branch from 0eb9626 to a43dbc6 Compare April 25, 2025 09:59

AlexD10S added 2 commits April 25, 2025 14:26

fix: tests and CI

cbf6360

chore: add check for polkavm-contracts in the CI and remove all-featu…

61452aa

…res in coverage

github-advanced-security bot found potential problems Apr 25, 2025

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

AlexD10S added 2 commits April 25, 2025 18:14

test: fix integration tests

514d1d2

chore: update dependencies

1bedee8

AlexD10S force-pushed the feat/revive-compatibility-with-flags branch 2 times, most recently from 8d7b214 to 1bedee8 Compare May 4, 2025 19:18

AlexD10S added 2 commits May 4, 2025 21:24

fix: deny in the CI and cleanups

a96ea25

fix: CI issues

f0d779b