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

Verify raw Wasm in cargo contract verify #1551

Merged
merged 9 commits into from
Mar 21, 2024
Merged

Verify raw Wasm in cargo contract verify #1551

merged 9 commits into from
Mar 21, 2024

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Mar 20, 2024

Summary

Closes #1539

  • y/n | Does it introduce breaking changes?
  • y/n | Is it dependent on the specific version of ink or pallet-contracts?

Adds a raw verification of the contract against the reference wasm binary.

The command executes the code in the Release mode by default and then compares the wasm output with the reference contract.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@SkymanOne SkymanOne changed the title verify wasm Verify raw Wasm Mar 20, 2024
@SkymanOne SkymanOne changed the title Verify raw Wasm Verify raw Wasm in cargo contract verify Mar 20, 2024
Comment on lines 135 to 141
let mut reader = BufReader::new(file);
let mut buffer: [u8; 32] = [0; 32];
reader
.read_exact(&mut buffer)
.context(format!("Failed to read contract binary {}", path.display()))?;

let output_code_hash = CodeHash(buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you only reading the first 32 bytes of the Wasm binary here and then assume it's the hash?

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

As commented, the whole files should be compared not just the first 32 bytes.

Also would be good to add to the existing integration test here.

In the issue it says:

We should add a argument --wasm that allows passing a binary obtained via e.g. cargo contract info --contract … --binary > contract.wasm.

If that is the common use case then it would be great to fetch that automatically, so you can just specify the account id in the verify command. @cmichi

crates/cargo-contract/src/cmd/verify.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/verify.rs Outdated Show resolved Hide resolved
@SkymanOne SkymanOne requested review from ascjones and cmichi March 20, 2024 21:17
crates/cargo-contract/src/cmd/verify.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/verify.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/verify.rs Outdated Show resolved Hide resolved
@SkymanOne SkymanOne merged commit 98cf70d into master Mar 21, 2024
11 checks passed
@SkymanOne SkymanOne deleted the gn/wasm-verify branch March 21, 2024 18:56
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.

Add --wasm to cargo contract verify
3 participants