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 III review #8

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

ayushmishra2005
Copy link

@ayushmishra2005 ayushmishra2005 commented Dec 12, 2022

General remarks


  1. README instructions should be much clear about testing the functionality of the pallet.
  2. Pallet name looks more generic. It should be more specific like what it is exactly doing.
  3. NO unit test-cases. Unit test-cases must be there for all dispatchable functions for all scenarios.
  4. Strong recommendation to run cargo fmt to improve formatting
  5. Strong recommendation to run cargo clippy to remove warnings and improve codebase.
  6. DO NOT write dispatchable function for fetching item from storage. The correct way is either to define a getter on a storage item, or to create a custom RPC method.
    For more details, please go through below links :

How to declare storage items in Substrate
Remote procedure calls
Add custom RPC to the node

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.

1 participant