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

SBP Milestone Review 3 #115

Merged
merged 2 commits into from
May 16, 2023
Merged

SBP Milestone Review 3 #115

merged 2 commits into from
May 16, 2023

Conversation

hbulgarini
Copy link
Contributor

SBP Review - Milestone 3

Pallets

The project under review has made commendable progress, especially in addressing the majority of the action items that emerged from the audit. These actions have been implemented effectively overall.
While going through the code, I identified some areas that could benefit from further refinement, particularly in the handling of percentage calculations. Enhancing the safety and robustness of these calculations would not only improve the accuracy of the operations but also increase the overall resilience of the project. Also there are some minor tweaks that can be applied.

The specific topics can be found in the comments made in the code with this format:

/// <HB SBP Review:
/// 
/// Feedback.
/// 
/// >

Weigths and benchmarking

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.

UI

The development of the User Interface (UI) is particularly noteworthy. The design and usability seem to facilitate a smooth user experience, leading users through a successful A to B process. However, I did encounter an issue during the user testing phase. Specifically, while trying to add my profile as a freelancer, the console logged an error:

Screenshot 2023-05-12 at 18 00 06

_app-562ee8d79bd28825.js:19 POST https://staging.imbue.network/api/freelancers 404

Uncaught (in promise) Error: A listener indicated an asynchronous response by returning true, but the message channel closed before a response was received

As a recommendation it would be beneficial to implement a global error catching mechanism that can report errors on the interface, such as:

function main() {
    <ErrorCatcher>
        <App />
    </ErrorCatcher>
}

In case this was implemented already, then it needs to be fixed somehow. Please take into account that as part of SBP reviews we don't do code review of javascript code.

@f-gate
Copy link
Member

f-gate commented May 16, 2023

Thanks Hector!

@f-gate f-gate changed the base branch from main to sbp3-review May 16, 2023 11:54
@f-gate f-gate merged commit 29a39f3 into ImbueNetwork:sbp3-review May 16, 2023
cuteolaf pushed a commit that referenced this pull request May 30, 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