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

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Aug 21, 2023

Closes #1274

This PR introduces an additional flag --chain that allows you conveniently choose which production chain to deploy a contract to instead of specifying the endpoint URL. Currently, there are endpoints for Aleph Zero, Astar and Shiden according to the list of production chains on the contracts UI.

On the backend, I put up a macro that allows automatically generating required methods and trait implementations for new endpoints in the future.

Additionally, there the user is prompted with a warning if they try to deploy a contract which doesn't have an image for the verifiable build specified in the metadata file or doesn't have a metadata file at all.
If the user tries to specify the URL which appears to be a known endpoint, we recognise it as an attempt to make a call to a production chain

@SkymanOne SkymanOne marked this pull request as ready for review August 21, 2023 22:10
@SkymanOne
Copy link
Contributor Author

We need to fix the transcoder as it failed to decode the events from the instantiation call for Aleph Zero

@SkymanOne
Copy link
Contributor Author

Need to address #1168 before merging this

@SkymanOne SkymanOne marked this pull request as draft August 23, 2023 10:48
@SkymanOne SkymanOne marked this pull request as ready for review March 21, 2024 14:39
pub enum ProductionChain {
AlephZero = "wss://ws.azero.dev:443",
Astar = "wss://rpc.astar.network:443",
Shiden = "wss://rpc.shiden.astar.network:443"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include the other ones from https://use.ink/where-to-deploy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the list from the contrast-ui. Out of the list only Krest allows me to submit an extrinsic.

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

@@ -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

@@ -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

@cmichi
Copy link
Collaborator

cmichi commented Mar 25, 2024

A couple of changes are required, let's do them in follow-ups.

@cmichi cmichi merged commit eae1cf2 into master Mar 25, 2024
10 checks passed
@cmichi cmichi deleted the gn/upload-check branch March 25, 2024 11:57
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.

Warn if un-verifiable contract is getting deployed on a production chain
4 participants