-
Notifications
You must be signed in to change notification settings - Fork 40
feat: Build verifiable contracts #800
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
Changes from all commits
ea47304
faae645
233422e
cd8601e
74fde11
9f2ad37
6fdeafa
062fd13
044881f
3b9250b
23daed5
25e97d6
ca08e8d
d765c04
e099c9c
661a65f
8b5ca42
0bf0202
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,7 +47,7 @@ pub(crate) struct BuildArgs { | |
| #[cfg(feature = "chain")] | ||
| pub command: Option<Command>, | ||
| /// Directory path with flag for your project [default: current directory] | ||
| #[arg(long)] | ||
| #[arg(long, visible_alias = "manifest-path")] | ||
tsenovilla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub(crate) path: Option<PathBuf>, | ||
| /// Directory path without flag for your project [default: current directory] | ||
| #[arg(value_name = "PATH", index = 1, conflicts_with = "path")] | ||
|
|
@@ -84,10 +84,15 @@ pub(crate) struct BuildArgs { | |
| #[clap(long, help_heading = CONTRACT_HELP_HEADER)] | ||
| #[cfg(feature = "contract")] | ||
| pub(crate) metadata: Option<MetadataSpec>, | ||
| /// Whether to build in a way that the contract is verifiable | ||
| ///#[clap(long, help_heading = CONTRACT_HELP_HEADER)] | ||
| ///#[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 | ||
tsenovilla marked this conversation as resolved.
Show resolved
Hide resolved
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the I was thinking on simply disabling
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see. The cargo-contract is a very confusing implementation no?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The line you mentioned is actually useless for verifiable contracts, as if the user specifies So yeah it's not optimal but Idk if we have something better |
||
| #[clap(long, conflicts_with_all = ["release", "profile"], help_heading = CONTRACT_HELP_HEADER)] | ||
| #[cfg(feature = "contract")] | ||
| pub(crate) verifiable: bool, | ||
| /// Custom image for verifiable builds | ||
| #[clap(long, requires = "verifiable", help_heading = CONTRACT_HELP_HEADER)] | ||
| #[cfg(feature = "contract")] | ||
| pub(crate) image: Option<String>, | ||
| } | ||
|
|
||
| /// Subcommand for building chain artifacts. | ||
|
|
@@ -120,12 +125,10 @@ impl Command { | |
|
|
||
| #[cfg(feature = "contract")] | ||
| if pop_contracts::is_supported(&project_path)? { | ||
| // All commands originating from root command are valid | ||
| let release = match &args.profile { | ||
| Some(profile) => (*profile).into(), | ||
| None => args.release, | ||
| }; | ||
| BuildContract { path: project_path, release, metadata: args.metadata }.execute()?; | ||
| let build_mode = contract::resolve_build_mode(args); | ||
| let image = contract::resolve_image(args)?; | ||
| BuildContract { path: project_path, build_mode, metadata: args.metadata, image } | ||
| .execute()?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
|
|
@@ -361,6 +364,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| }, | ||
| project_path, | ||
| &mut cli, | ||
|
|
@@ -429,6 +436,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
|
|
||
| Ok(()) | ||
|
|
@@ -474,6 +485,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
|
|
||
| // Test 2: Execute with production profile | ||
|
|
@@ -496,6 +511,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
|
|
||
| // Test 3: Execute with custom features | ||
|
|
@@ -515,6 +534,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
| } | ||
|
|
||
|
|
@@ -538,6 +561,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
|
|
||
| // Test 5: Execute with path_pos instead of path | ||
|
|
@@ -560,6 +587,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
|
|
||
| // Test 6: Execute with benchmark and try_runtime flags | ||
|
|
@@ -579,6 +610,10 @@ mod tests { | |
| only_runtime: false, | ||
| #[cfg(feature = "contract")] | ||
| metadata: None, | ||
| #[cfg(feature = "contract")] | ||
| verifiable: false, | ||
| #[cfg(feature = "contract")] | ||
| image: None, | ||
| })?; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The
resolve_build_modefunction doesn't handle the case where bothverifiable=trueand a profile is specified (e.g.,Profile::Release+verifiable=true). Due to the clapconflicts_with_allconstraint 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 returnBuildMode::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:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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,
clapwill handle this conflict, so I wouldn't add a specific case just for tests