Codecov Report

Attention: Patch coverage is 81.40604% with 283 lines in your changes missing coverage. Please review.

Project coverage is 76.82%. Comparing base (b7c07b2) to head (ed182a5).

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
+ Coverage   76.21%   76.82%   +0.60%     
==========================================
  Files          82       84       +2     
  Lines       18377    19720    +1343     
  Branches    18377    19720    +1343     
==========================================
+ Hits        14006    15149    +1143     
- Misses       2485     2589     +104     
- Partials     1886     1982      +96     

... and 1 file with indirect coverage changes

But then when deploy, it throws an error

⚙  Starting deployment with Polkadot Deployment Portal
│
└  Deployment failed: Deployment failed with status 400 Bad Request: {"error":{"issues":[{"code":"custom","message":"Invalid chainspec format: [\n  {\n    \"code\": \"invalid_type\",\n    \"expected\": \"object\",\n    \"received\": \"null\",\n    \"path\": [\n      \"properties\"\n    ],\n    \"message\": \"Expected object, received null\"\n  }\n]","path":["chainspecs","chainspecs"]},{"code":"custom","message":"ParaId in chainspec (undefined) doesn't match the provided paraId - 4727","path":["chainspecs"]}],"name":"ZodError"}}

The raw chain spec seems to have been properly generated. I've shared the details with the PDP team to investigate what might be wrong with the request.

Based on the issues in your flow, if the reserve and registration steps succeed but deployment to the provider fails, it might be useful to add a --skip-registration flag (included in 7dab5ee) and a --chain-spec parameter (introduced in
ed32ffc). The latter avoids regenerating the full chain spec and instead only generates the raw chain spec and artifacts. This would allow retrying only the deployment step without redoing the registration process.

Minor thing but if I provide --skip-registration and select "Only Register in Relay Chain", it asks me for the relaychain node URL but nothing happens. I don't think this is a bug but if --skip-registration is provided, the "Only Register in Relay Chain" should not be displayed for selection?

Minor thing but if I provide --skip-registration and select "Only Register in Relay Chain", it asks me for the relaychain node URL but nothing happens. I don't think this is a bug but if --skip-registration is provided, the "Only Register in Relay Chain" should not be displayed for selection?

Good point. 4c050a2

I think we should do two things, one, we should always try to build the required binary to make sure it is up to date with the latest changes. This prevents the amount of things that can go wrong by a lot. We want to minimise the amount of things that can go wrong, especially with a feature that one should preferably only have to use a few times in their life, and one that is costly. In addition, we need to find the cause of these unclear errors and make sure they are improved.

The errors related to generating the chain spec that you mentioned above are not related to this PR, but I agree they need to be addressed. Fixed in a separate PR.

I'm not sure if forcing a rebuild is really worth it. The code change itself is simple, just a single line at the beginning of the flow:

build_parachain(self.path.as_ref().unwrap_or(&PathBuf::from("./")), None, self.profile.as_ref().unwrap_or(&Profile::Release), None)?;

However, it introduces extra noise early in the flow, and we already trigger a build when generating the chain spec if the node hasn't been built yet. Your specific case, an error in spec generation due to a build with runtime-benchmarks is quite rare.

On a completely other note: If the build fails after the para id is reserved, we should make it very clear how to proceed with the pop up command and the reserved para id. This to ensure the user knows that is not a big deal and it can still use that para id that it reserved, as well as being helped with what command to use to make their life a breeze.

This is very good point. Improved messaging in 4c159e9

In general, would it make sense to make it possible to dry run this?

That would be amazing! For reserve/registration, we'd need integration with Chopsticks, but for deployment, I'm not sure how we could dry-run it.

Moreover, when the --skip-registration flag is provided, the artifacts are already deployed, so we shouldn't have to be queried about the chain spec etc. Moreover, we should not build all the chain specs and artifacts (perhaps even request and insert collator keys).

We need the chain spec for deployment. If the user doesn’t want to generate it again, I’ve added the --chain-spec flag to allow providing it. I agree that requesting collator keys could be skipped, however, due to the current provider API, the collator_id_key is required in the POST request and is generated on their side in the GET response to retrieve the collator keys. So for now, we can't skip this step.