HB Substrate Builder Program - Milestone Review 2 #569
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HB Substrate Builder Program - Milestone Review 2
Documentation
The documentation is well-structured and covers the components of their solution in a clear way (https://docs.webb.tools/docs/). However, my constructive feedback around the documentation is that bridging and privacy are one of the most complex topics in the industry, and the documentation lacks a more introductory part to help the end user understand the big picture of the product offering and its benefits. Some of these aspects are covered in the manifesto, but even with my developer (not privacy-oriented) profile, I struggled initially to understand the product offering. Therefore, I recommend starting the documentation with some basic examples or analogies to help users build their mental model of how the product works.
Weights
The code base was upgraded to release 0.9.39, indicating that you are following the release pace consistently. However, you are still running weights v1 since the weights are still using the methods
from_ref_time
instead of thefrom_parts
which involves the two fields from weights v2.I recommend going through the following PR to migrate into Weights v2 properly:
paritytech/substrate#11637
You might also consider creating a weights template like this one to have more control over the weights process generation coming from the benchmarking.
Pending SBP Milestone Review 1 Topics
In the previous SBP Milestone review, it was pointed out some issues/recommendations that have already been taken into account. However, I noticed that there are still many
unwrap
s that can lead to panics in the runtime, which should be avoided.DKG Metadata Pallet
This is a vague recommendation based on general good programming practices. I think that the DKG Metadata pallet is handling a diverse group of responsibilities, and the code readability might become hard to follow if it keeps growing. As a first step, I recommend moving the functions out of the main pallet file into its own file. Secondly, I would like to challenge if the pallet might be split into smaller pallets with more dedicated responsibilities, such as:
I understand that this would involve a major refactor on a core functionality and might not be urgent or a blocker for deployment, but I just wanted to share this idea with the team.
XCM
I agree that, at this point in time, trying to push the communication within chains using XCM might become a little bit cumbersome since, at the moment, XCM shines on transferring assets, not arbitrary data as needed in this project. For standalone chains that will have the intention to bridge to any parachain in Polkadot/Kusama, having the xcm folder available in the code base of the project might be useful for creating the XCM set of messages so they can be bridged as data (Snowfork is doing something similar AFAIK).
Offchain Workers
In the Delivery Services team we have detected some design assumptions & vulnerabilities that worths reading: https://forum.polkadot.network/t/offchain-workers-design-assumptions-vulnerabilities/2548
Going through the points described there might be useful for this project since it relays considerably on this functionality.