Skip to content

Conversation

@tsenovilla
Copy link
Collaborator

@tsenovilla tsenovilla commented Nov 26, 2025

Closes:

cargo contract already contains most of the logic, so this one should be easy to review

@tsenovilla tsenovilla linked an issue Nov 26, 2025 that may be closed by this pull request
@tsenovilla tsenovilla changed the base branch from main to feat/contract-verification November 26, 2025 09:26
@tsenovilla tsenovilla force-pushed the feat/build-verifiable-contracts branch from 05c0d27 to ea47304 Compare November 26, 2025 12:26
Cargo.toml Outdated
Comment on lines 60 to 62
contract-build = { git = "https://github.com/use-ink/cargo-contract", rev = "c7aba2e", default-features = false }
contract-extrinsics = { git = "https://github.com/use-ink/cargo-contract", rev = "c7aba2e", default-features = false }
contract-transcode = { git = "https://github.com/use-ink/cargo-contract", rev = "c7aba2e", default-features = false }
Copy link
Collaborator Author

@tsenovilla tsenovilla Nov 27, 2025

Choose a reason for hiding this comment

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

Temporarily needed til these changes are released. Please note that the base branch for this PR isn't main but feat/contract-verification so we can merge as soon as the review is done

os:
- ubuntu-latest
- macos-latest
- macos-13
Copy link
Collaborator Author

@tsenovilla tsenovilla Nov 27, 2025

Choose a reason for hiding this comment

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

We need a docker daemon running for the new verifiable_contract_lifecycle test.
Apparently, GitHub Actions doesn't support virtualization for ARM chips (reference run https://github.com/r0gue-io/pop-cli/actions/runs/19732309848/job/56536182023), so we need to either:

  1. Use an Intel chip version.
  2. Skip that test on MacOs.

I prefer 1, skipping a test smells super weird. But it's true that the job is running far slower than the Linux one due to all this virtualization stuff, not the test itself. So option 2 might also be considered

@tsenovilla tsenovilla marked this pull request as ready for review November 27, 2025 10:42
@tsenovilla tsenovilla force-pushed the feat/build-verifiable-contracts branch 2 times, most recently from 19d213e to 8645ec0 Compare November 27, 2025 12:01
@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

❌ Patch coverage is 70.58824% with 40 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feat/contract-verification@4ca7e75). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/pop-cli/src/commands/build/contract.rs 84.21% 7 Missing and 2 partials ⚠️
crates/pop-contracts/src/build.rs 0.00% 9 Missing ⚠️
crates/pop-cli/src/commands/call/contract.rs 0.00% 7 Missing ⚠️
crates/pop-cli/src/commands/up/contract.rs 0.00% 7 Missing ⚠️
crates/pop-cli/src/commands/build/mod.rs 88.88% 4 Missing ⚠️
crates/pop-contracts/src/utils/mod.rs 80.00% 0 Missing and 4 partials ⚠️
@@                      Coverage Diff                      @@
##             feat/contract-verification     #800   +/-   ##
=============================================================
  Coverage                              ?   76.05%           
=============================================================
  Files                                 ?      115           
  Lines                                 ?    26423           
  Branches                              ?    26423           
=============================================================
  Hits                                  ?    20095           
  Misses                                ?     4190           
  Partials                              ?     2138           
Files with missing lines Coverage Δ
crates/pop-cli/src/commands/build/mod.rs 85.82% <88.88%> (ø)
crates/pop-contracts/src/utils/mod.rs 82.05% <80.00%> (ø)
crates/pop-cli/src/commands/call/contract.rs 72.21% <0.00%> (ø)
crates/pop-cli/src/commands/up/contract.rs 14.35% <0.00%> (ø)
crates/pop-cli/src/commands/build/contract.rs 72.72% <84.21%> (ø)
crates/pop-contracts/src/build.rs 29.03% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsenovilla tsenovilla force-pushed the feat/build-verifiable-contracts branch 10 times, most recently from 6c8bec3 to 8472867 Compare November 27, 2025 18:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for building verifiable smart contracts using Docker-based deterministic builds, leveraging the verifiable build functionality from cargo-contract. The feature enables reproducible builds through the new --verifiable flag and optional custom Docker images via the --image flag.

Key Changes:

  • Changed the build API from accepting a boolean release parameter to a BuildMode enum (Debug/Release/Verifiable)
  • Added Docker support for macOS CI testing using Colima with QEMU
  • Introduced --manifest-path as an alias for --path to support direct Cargo.toml references

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
crates/pop-contracts/src/build.rs Updated build_smart_contract to accept BuildMode and ImageVariant; added tokio runtime handling for verifiable builds
crates/pop-contracts/src/utils/mod.rs Enhanced get_manifest_path to accept either directory paths or direct Cargo.toml paths
crates/pop-contracts/src/lib.rs Exported new types BuildMode and ImageVariant from the public API
crates/pop-contracts/README.md Updated example code to reflect new BuildMode parameter
crates/pop-cli/src/commands/build/mod.rs Added --verifiable and --image CLI flags with --manifest-path alias; updated test fixtures
crates/pop-cli/src/commands/build/contract.rs Added resolve_build_mode and resolve_image helper functions with comprehensive unit tests
crates/pop-cli/src/commands/up/contract.rs Updated to use BuildMode::Release instead of boolean
crates/pop-cli/src/commands/call/contract.rs Updated to use BuildMode::Release instead of boolean
crates/pop-cli/tests/contract.rs Added integration test for verifiable contract build with metadata validation
Cargo.toml Changed contract dependencies from crates.io versions to git revision c7aba2e
Cargo.lock Updated dependency sources to match Cargo.toml changes
.github/workflows/integration-tests.yml Added Docker/Colima setup for macOS-13; configured TMPDIR for Docker compatibility

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

match (&args.profile, args.verifiable) {
(Some(Profile::Release), false) | (Some(Profile::Production), false) => BuildMode::Release,
(None, true) => BuildMode::Verifiable,
(None, false) if args.release => BuildMode::Release,
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The resolve_build_mode function doesn't handle the case where both verifiable=true and a profile is specified (e.g., Profile::Release + verifiable=true). Due to the clap conflicts_with_all constraint on line 89 of mod.rs, this should be prevented at the CLI level. However, if this function is called programmatically in tests or other code paths, it would silently fall through to the _ catch-all and return BuildMode::Debug, which is incorrect.

Consider adding a comment explaining that this case is prevented by CLI validation, or add an explicit match arm that clarifies the behavior:

(Some(_), true) => BuildMode::Debug, // Unreachable due to CLI conflicts_with_all
Suggested change
(None, false) if args.release => BuildMode::Release,
(None, false) if args.release => BuildMode::Release,
// This case is unreachable due to CLI `conflicts_with_all` validation,
// but is included for clarity in case the function is called programmatically.
(Some(_), true) => BuildMode::Debug, // Unreachable due to CLI conflicts_with_all

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@tsenovilla tsenovilla Nov 28, 2025

Choose a reason for hiding this comment

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

While the review is right, the fallback is there to reduce the casuistry and just default to the simplest mode instead of failing if an invalid combination of args is used. As the review says, clap will handle this conflict, so I wouldn't add a specific case just for tests


#[tokio::test]
async fn verifiable_contract_lifecycle() -> Result<()> {
// TODO: Incomplete test, we'll be adding more steps as the development of the feature advance
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The comment contains a typo: "advance" should be "advances" (or better yet, "progresses").

Suggested correction:

// TODO: Incomplete test, we'll be adding more steps as the development of the feature progresses
Suggested change
// TODO: Incomplete test, we'll be adding more steps as the development of the feature advance
// TODO: Incomplete test, we'll be adding more steps as the development of the feature progresses

Copilot uses AI. Check for mistakes.
Comment on lines 277 to 278
async fn verifiable_contract_lifecycle() -> Result<()> {
// TODO: Incomplete test, we'll be adding more steps as the development of the feature advance
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The test name verifiable_contract_lifecycle is misleading because it only tests the build step, not a full lifecycle like the contract_lifecycle test above it. The test doesn't cover deployment, instantiation, or calling the contract.

Consider renaming to something more accurate like verifiable_contract_build or updating the TODO comment to clarify that this will eventually become a full lifecycle test.

Suggested change
async fn verifiable_contract_lifecycle() -> Result<()> {
// TODO: Incomplete test, we'll be adding more steps as the development of the feature advance
async fn verifiable_contract_build() -> Result<()> {
// TODO: This test currently only covers the verifiable build step. We'll add deployment, instantiation, and calling as the feature develops, and rename the test accordingly.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a lifecycle of a verifiable contract in the sense build -> publish -> verify, we won't be including calls, deployment and so on as that's already tested by contract_lifecycle. The other steps will be added as the feature development progresses

Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Great job! I left some comments on the code.

Regarding DevEx, it's not an easy thing to fix (we have a similar issue with --deterministic for chains), but just something to keep in mind: the user needs to know that Docker must be running on their machine. If they only run pop build --verifiable, the error message is confusing:

Error: Socket not found: /var/run/docker.sock
Do you have the docker engine installed in path?

#[cfg(feature = "contract")]
pub(crate) verifiable: bool,
/// Custom image for verifiable builds
#[clap(long, help_heading = CONTRACT_HELP_HEADER)]
Copy link
Collaborator

@AlexD10S AlexD10S Dec 1, 2025

Choose a reason for hiding this comment

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

Here you can add requires = "verifiable", image is useless if verifiable is not indicated no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, let me fix it!

///#[cfg(feature = "contract")]
///pub(crate) verifiable: bool
/// Whether to build in a way that the contract is verifiable. For verifiable contracts, use
/// --manifest-path instead of --path to directly point to your contracts Cargo.toml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? --manifest-path instead of --path to directly point to your contracts Cargo.toml
Can we not abstract this to the user?

Copy link
Collaborator Author

@tsenovilla tsenovilla Dec 1, 2025

Choose a reason for hiding this comment

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

Because the contract-build function we're using captures what's passed to the CLI: https://github.com/use-ink/cargo-contract/blob/master/crates/build/src/docker.rs#L523 and then pass it to cargo-contract CLI inside the Docker image. If we pass --path that CLI called inside docker will simply fail as it doesn't expect --path, you can try it yourself.

I was thinking on simply disabling --path if --verifiable is used, but I think it's still worth it to don't force the user to run the command inside the contract's dir.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see. The cargo-contract is a very confusing implementation no?
Because here we already get the path to the manifest path: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-contracts/src/build.rs#L22 and then we can call the build function very clean.
Not sure the best solution here to be honest, for the user might be confusing to be forced to use --manifest-path but i see your point

Copy link
Collaborator Author

@tsenovilla tsenovilla Dec 1, 2025

Choose a reason for hiding this comment

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

yeah it's not the best for third party integration, but normal anyway as it's designed to call cargo-contract inside the image :/

The line you mentioned is actually useless for verifiable contracts, as if the user specifies --manifest-path it's captured within the image, not passed to contract_build::execute at all.

So yeah it's not optimal but Idk if we have something better

@tsenovilla
Copy link
Collaborator Author

If they only run pop build --verifiable, the error message is confusing

Agreed with @AlexD10S. This message error comes directly from cargo-contract. I was thinking that maybe we should run Docker for the user, but checking how --deterministic works I saw we're not doing that, that's why I didn't add it here.
But maybe it's worth it checking if docker is running and just returning a nicer message (tho Do you have the docker engine installed in path? is descriptive enough anyway), what do you think?

@tsenovilla tsenovilla force-pushed the feat/build-verifiable-contracts branch from 0df52e3 to 0bf0202 Compare December 1, 2025 11:43
@AlexD10S
Copy link
Collaborator

AlexD10S commented Dec 1, 2025

If they only run pop build --verifiable, the error message is confusing

Agreed with @AlexD10S. This message error comes directly from cargo-contract. I was thinking that maybe we should run Docker for the user, but checking how --deterministic works I saw we're not doing that, that's why I didn't add it here. But maybe it's worth it checking if docker is running and just returning a nicer message (tho Do you have the docker engine installed in path? is descriptive enough anyway), what do you think?

That would be great, it can be done in another PR if you prefer, and the same logic implemented for the --deterministic feature.

@tsenovilla tsenovilla merged commit dc2dc57 into feat/contract-verification Dec 1, 2025
22 checks passed
@tsenovilla tsenovilla deleted the feat/build-verifiable-contracts branch December 1, 2025 12:41
@tsenovilla
Copy link
Collaborator Author

That would be great, it can be done in another PR if you prefer, and the same logic implemented for the --deterministic feature.

#810 && #811

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.

feat: pop build --verifiable

3 participants