-
Notifications
You must be signed in to change notification settings - Fork 119
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
Conversation
We need to fix the transcoder as it failed to decode the events from the instantiation call for Aleph Zero |
Need to address #1168 before merging this |
crates/extrinsics/src/prod_chains.rs
Outdated
pub enum ProductionChain { | ||
AlephZero = "wss://ws.azero.dev:443", | ||
Astar = "wss://rpc.astar.network:443", | ||
Shiden = "wss://rpc.shiden.astar.network:443" |
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.
Please include the other ones from https://use.ink/where-to-deploy.
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.
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' |
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.
I am not sure if the default should be 'yes'; rather, we should recommend uploading only verifiable contracts to production chains. WDYT?
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.
We do advise it, but we still want to give a user an option to upload an unverifiable contract (aka shoot their own leg)
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.
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()) |
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.
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
.
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.
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.
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.
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.
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.
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")] |
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.
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
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.
This can be a follow up PR
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.
Ok
A couple of changes are required, let's do them in follow-ups. |
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