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

HB Substrate Builder Program - Milestone Review 2 #569

Closed
wants to merge 1 commit into from

Conversation

hbulgarini
Copy link

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 the from_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 unwraps 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:

  • DKG Key Pallet: Focusing on key management, this pallet would store the active and next DKG public keys, as well as the signatures for these keys.
  • DKG Threshold and Refresh Pallet: This pallet would handle the DKG signature threshold and key refresh process.
  • DKG Misbehavior and Reputation Pallet: This pallet would focus on managing misbehavior reporting and reputation tracking for DKG authorities.

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.

@1xstj
Copy link
Contributor

1xstj commented Apr 13, 2023

Thanks for the feedback, opened a tracking issue here : #573

@1xstj 1xstj closed this Apr 13, 2023
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.

2 participants