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

Specify prod chain URL with names and check for the verifiable build upon upload #1290

Merged
merged 11 commits into from
Mar 25, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Verify raw Wasm in cargo contract verify - [#1551](https://github.com/paritytech/cargo-contract/pull/1551)
- Specify prod chain URL with names and check for the verifiable build upon upload - [#1290](https://github.com/paritytech/cargo-contract/pull/1290)

## [4.0.2]

Expand Down
9 changes: 7 additions & 2 deletions crates/cargo-contract/src/cmd/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use std::{
},
str::FromStr,
};
use url::Url;

use super::{
config::SignerConfig,
Expand Down Expand Up @@ -126,8 +127,11 @@ impl CallCommand {
.map_err(|e| anyhow::anyhow!("Failed to parse contract option: {}", e))?;
let signer = C::Signer::from_str(&self.extrinsic_cli_opts.suri)
.map_err(|_| anyhow::anyhow!("Failed to parse suri option"))?;
let token_metadata =
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?;
let token_metadata = if let Some(chain) = &self.extrinsic_cli_opts.chain {
TokenMetadata::query::<C>(&Url::parse(chain.end_point()).unwrap()).await?
} else {
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?
};
let storage_deposit_limit = self
.extrinsic_cli_opts
.storage_deposit_limit
Expand All @@ -143,6 +147,7 @@ impl CallCommand {
.file(self.extrinsic_cli_opts.file.clone())
.manifest_path(self.extrinsic_cli_opts.manifest_path.clone())
.url(self.extrinsic_cli_opts.url.clone())
.chain(self.extrinsic_cli_opts.chain.clone())
Copy link
Collaborator

@smiasojed smiasojed Mar 21, 2024

Choose a reason for hiding this comment

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

Why do we need to pass chain to extrinsics crate? I think extrinisics crate should just get url as it gets it now. We could keep all the prod chain logic in cargo-contracts crate

I always thought about the extrinsics crate to be small and reusable between other tools, like e2e tests or smart-bench.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also want to retain info about what kind of chain it is: Prod or Custom. So it is easier to encapsulate the logic this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this information in extrinsics crate? The upload check is done in cargo-contract crate.
In ExtrinsicOpts we have url and chain which is kind of data duplication.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this is not belong in extrinsics

.storage_deposit_limit(storage_deposit_limit)
.verbosity(self.extrinsic_cli_opts.verbosity()?)
.done();
Expand Down
10 changes: 9 additions & 1 deletion crates/cargo-contract/src/cmd/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use contract_extrinsics::{
url_to_string,
ContractInfo,
ErrorVariant,
ProductionChain,
TrieId,
};
use ink_env::Environment;
Expand Down Expand Up @@ -74,6 +75,9 @@ pub struct InfoCommand {
default_value = "ws://localhost:9944"
)]
url: url::Url,
/// A name of a production chain to upload or instantiate the contract on.
#[clap(name = "chain", long, conflicts_with = "url")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apart from the URL, the chain could also retain the chain configuration. So, when you specify 'alephzero', 'moonbeam', or whatever, it would also set the correct config (https://github.com/paritytech/cargo-contract/blob/master/crates/cargo-contract/src/cmd/config.rs) for the chain.

In this case chain would conflict with url and config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be a follow up PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

chain: Option<ProductionChain>,
/// Export the instantiate output in JSON format.
#[clap(name = "output-json", long)]
output_json: bool,
Expand Down Expand Up @@ -102,7 +106,11 @@ impl InfoCommand {
<<C as Config>::AccountId as FromStr>::Err:
Into<Box<(dyn std::error::Error)>> + Display,
{
let rpc_cli = RpcClient::from_url(url_to_string(&self.url)).await?;
let rpc_cli = if let Some(chain) = &self.chain {
RpcClient::from_url(chain.end_point()).await?
} else {
RpcClient::from_url(url_to_string(&self.url)).await?
};
let client = OnlineClient::<C>::from_rpc_client(rpc_cli.clone()).await?;
let rpc = LegacyRpcMethods::<C>::new(rpc_cli.clone());

Expand Down
21 changes: 18 additions & 3 deletions crates/cargo-contract/src/cmd/instantiate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use super::{
use crate::{
anyhow,
call_with_config,
cmd::prompt_confirm_unverifiable_upload,
ErrorVariant,
InstantiateExec,
Weight,
Expand All @@ -43,6 +44,7 @@ use contract_build::{
Verbosity,
};
use contract_extrinsics::{
Chain,
Code,
DisplayEvents,
ExtrinsicOptsBuilder,
Expand Down Expand Up @@ -70,6 +72,7 @@ use subxt::{
},
Config,
};
use url::Url;

#[derive(Debug, clap::Args)]
pub struct InstantiateCommand {
Expand Down Expand Up @@ -134,8 +137,12 @@ impl InstantiateCommand {
{
let signer = C::Signer::from_str(&self.extrinsic_cli_opts.suri)
.map_err(|_| anyhow::anyhow!("Failed to parse suri option"))?;
let token_metadata =
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?;
let token_metadata = if let Some(chain) = &self.extrinsic_cli_opts.chain {
TokenMetadata::query::<C>(&Url::parse(chain.end_point()).unwrap()).await?
} else {
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?
};

let storage_deposit_limit = self
.extrinsic_cli_opts
.storage_deposit_limit
Expand All @@ -151,6 +158,7 @@ impl InstantiateCommand {
.file(self.extrinsic_cli_opts.file.clone())
.manifest_path(self.extrinsic_cli_opts.manifest_path.clone())
.url(self.extrinsic_cli_opts.url.clone())
.chain(self.extrinsic_cli_opts.chain.clone())
.storage_deposit_limit(storage_deposit_limit)
.done();

Expand Down Expand Up @@ -182,7 +190,7 @@ impl InstantiateCommand {
}
Err(object) => {
if self.output_json() {
return Err(object)
return Err(object);
} else {
name_value_println!("Result", object, MAX_KEY_COL_WIDTH);
display_contract_exec_result::<_, MAX_KEY_COL_WIDTH, _>(&result)?;
Expand All @@ -191,6 +199,13 @@ impl InstantiateCommand {
}
}
} else {
if let Chain::Production(name) =
instantiate_exec.opts().chain_and_endpoint().0
{
if !instantiate_exec.opts().is_verifiable()? {
prompt_confirm_unverifiable_upload(&name)?
}
}
tracing::debug!("instantiate data {:?}", instantiate_exec.args().data());
let gas_limit = pre_submit_dry_run_gas_estimate_instantiate(
&instantiate_exec,
Expand Down
34 changes: 34 additions & 0 deletions crates/cargo-contract/src/cmd/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub(crate) use contract_extrinsics::ErrorVariant;
use contract_extrinsics::{
pallet_contracts_primitives::ContractResult,
BalanceVariant,
ProductionChain,
TokenMetadata,
};

Expand Down Expand Up @@ -127,6 +128,9 @@ pub struct CLIExtrinsicOpts {
/// Before submitting a transaction, do not ask the user for confirmation.
#[clap(short('y'), long)]
skip_confirm: bool,
/// A name of a production chain to upload or instantiate the contract on.
#[clap(name = "chain", long, conflicts_with = "url")]
chain: Option<ProductionChain>,
}

impl CLIExtrinsicOpts {
Expand Down Expand Up @@ -310,6 +314,36 @@ where
Ok(arr.into())
}

/// Prompt the user to confirm the upload of unverifiable code to the production chain.
pub fn prompt_confirm_unverifiable_upload(chain: &str) -> Result<()> {
println!(
"{} (skip with --skip-validate)",
"Confirm upload:".bright_white().bold()
);
let warning = format!(
"You are trying to upload unverifiable code to {} mainnet",
chain
)
.bold()
.yellow();
print!("{}", warning);
println!(
"{} ({}): ",
"\nContinue?".bright_white().bold(),
"Y/n".bright_white().bold()
);

let mut buf = String::new();
io::stdout().flush()?;
io::stdin().read_line(&mut buf)?;
match buf.trim().to_lowercase().as_str() {
// default is 'y'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if the default should be 'yes'; rather, we should recommend uploading only verifiable contracts to production chains. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do advise it, but we still want to give a user an option to upload an unverifiable contract (aka shoot their own leg)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just thinking to make n as default

"y" | "" => Ok(()),
"n" => Err(anyhow!("Upload canceled!")),
c => Err(anyhow!("Expected either 'y' or 'n', got '{}'", c)),
}
}

#[cfg(test)]
mod tests {
use subxt::{
Expand Down
10 changes: 9 additions & 1 deletion crates/cargo-contract/src/cmd/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use contract_build::name_value_println;
use contract_extrinsics::{
ErrorVariant,
ProductionChain,
RawParams,
RpcRequest,
};
Expand All @@ -40,14 +41,21 @@ pub struct RpcCommand {
default_value = "ws://localhost:9944"
)]
url: url::Url,
/// A name of a production chain to upload or instantiate the contract on.
#[clap(name = "chain", long, conflicts_with = "url")]
chain: Option<ProductionChain>,
/// Export the call output in JSON format.
#[clap(long)]
output_json: bool,
}

impl RpcCommand {
pub async fn run(&self) -> Result<(), ErrorVariant> {
let request = RpcRequest::new(&self.url).await?;
let request = if let Some(chain) = &self.chain {
RpcRequest::new(&url::Url::parse(chain.end_point()).unwrap()).await?
} else {
RpcRequest::new(&self.url).await?
};
let params = RawParams::new(&self.params)?;

let result = request.raw_call(&self.method, params).await;
Expand Down
16 changes: 14 additions & 2 deletions crates/cargo-contract/src/cmd/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ use super::{
config::SignerConfig,
display_dry_run_result_warning,
parse_balance,
prompt_confirm_unverifiable_upload,
CLIExtrinsicOpts,
};
use anyhow::Result;
use contract_build::name_value_println;
use contract_extrinsics::{
Chain,
DisplayEvents,
ExtrinsicOptsBuilder,
TokenMetadata,
Expand All @@ -51,6 +53,7 @@ use subxt::{
},
Config,
};
use url::Url;

#[derive(Debug, clap::Args)]
#[clap(name = "upload", about = "Upload a contract's code")]
Expand Down Expand Up @@ -88,8 +91,11 @@ impl UploadCommand {
{
let signer = C::Signer::from_str(&self.extrinsic_cli_opts.suri)
.map_err(|_| anyhow::anyhow!("Failed to parse suri option"))?;
let token_metadata =
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?;
let token_metadata = if let Some(chain) = &self.extrinsic_cli_opts.chain {
TokenMetadata::query::<C>(&Url::parse(chain.end_point()).unwrap()).await?
} else {
TokenMetadata::query::<C>(&self.extrinsic_cli_opts.url).await?
};
let storage_deposit_limit = self
.extrinsic_cli_opts
.storage_deposit_limit
Expand All @@ -103,6 +109,7 @@ impl UploadCommand {
.file(self.extrinsic_cli_opts.file.clone())
.manifest_path(self.extrinsic_cli_opts.manifest_path.clone())
.url(self.extrinsic_cli_opts.url.clone())
.chain(self.extrinsic_cli_opts.chain.clone())
.storage_deposit_limit(storage_deposit_limit)
.done();

Expand Down Expand Up @@ -136,6 +143,11 @@ impl UploadCommand {
}
}
} else {
if let Chain::Production(name) = upload_exec.opts().chain_and_endpoint().0 {
if !upload_exec.opts().is_verifiable()? {
prompt_confirm_unverifiable_upload(&name)?
}
}
let upload_result = upload_exec.upload_code().await?;
let display_events = DisplayEvents::from_events::<C, C>(
&upload_result.events,
Expand Down
2 changes: 1 addition & 1 deletion crates/extrinsics/src/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ where
let call_data = transcoder.encode(&self.message, &self.args)?;
tracing::debug!("Message data: {:?}", hex::encode(&call_data));

let url = self.extrinsic_opts.url();
let (_, url) = self.extrinsic_opts.chain_and_endpoint();
let rpc = RpcClient::from_url(&url).await?;
let client = OnlineClient::from_rpc_client(rpc.clone()).await?;
let rpc = LegacyRpcMethods::new(rpc);
Expand Down
9 changes: 9 additions & 0 deletions crates/extrinsics/src/contract_artifacts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,13 @@ impl ContractArtifacts {
ContractMessageTranscoder::try_from(metadata)
.context("Failed to deserialize ink project metadata from contract metadata")
}

/// Return true if the image is verifiable.
/// If the metadata can not be extracted then we assume that it can't be verified.
pub fn is_verifiable(&self) -> bool {
match self.metadata() {
Ok(m) => m.image.is_some(),
Err(_) => false,
}
}
}
43 changes: 43 additions & 0 deletions crates/extrinsics/src/extrinsic_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use subxt::{
use url::Url;

use crate::{
prod_chains::ProductionChain,
url_to_string,
ContractArtifacts,
};
Expand All @@ -34,6 +35,12 @@ use std::{
path::PathBuf,
};

#[derive(Debug)]
pub enum Chain {
Production(String),
Custom,
}

/// Arguments required for creating and sending an extrinsic to a substrate node.
#[derive(Derivative)]
#[derivative(Clone(bound = "E::Balance: Clone"))]
Expand All @@ -44,6 +51,7 @@ pub struct ExtrinsicOpts<C: Config, E: Environment, Signer: Clone> {
signer: Signer,
storage_deposit_limit: Option<E::Balance>,
verbosity: Verbosity,
chain: Option<ProductionChain>,
_marker: PhantomData<C>,
}

Expand All @@ -66,6 +74,7 @@ where
signer,
storage_deposit_limit: None,
verbosity: Verbosity::Default,
chain: None,
_marker: PhantomData,
},
}
Expand Down Expand Up @@ -110,6 +119,13 @@ where
this
}

/// Set the production chain.
pub fn chain(self, chain: Option<ProductionChain>) -> Self {
let mut this = self;
this.opts.chain = chain;
this
}

pub fn done(self) -> ExtrinsicOpts<C, E, Signer> {
self.opts
}
Expand Down Expand Up @@ -142,6 +158,33 @@ where
url_to_string(&self.url)
}

/// Get the chain name and its URL endpoint.
/// If the user specify the endpoint manually,
/// but it still appears to be the production chain,
/// we still convert it.
pub fn chain_and_endpoint(&self) -> (Chain, String) {
if let Some(chain) = &self.chain {
(
Chain::Production(chain.to_string()),
chain.end_point().to_string(),
)
} else {
let url = self.url();
if let Some(chain) = ProductionChain::chain_by_endpoint(&url) {
(
Chain::Production(chain.to_string()),
chain.end_point().to_string(),
)
} else {
(Chain::Custom, url)
}
}
}

pub fn is_verifiable(&self) -> Result<bool> {
Ok(self.contract_artifacts()?.is_verifiable())
}

/// Return the signer.
pub fn signer(&self) -> &Signer {
&self.signer
Expand Down
Loading
Loading