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 feedback #73

Open
Immanuel-john opened this issue May 8, 2023 · 0 comments
Open

SBP milestone review feedback #73

Immanuel-john opened this issue May 8, 2023 · 0 comments

Comments

@Immanuel-john
Copy link
Contributor

Immanuel-john commented May 8, 2023

  1. Recommends to run/use cargo clippy + cargo +nightly fmt in an automated CI/CD (github actions).
    Working on it
  2. Recommend creating some integration tests using zombienet.
    Since Ternoa is not having parachain setup yet this stands invalid. But it's in our roadmap
  3. Use a const to refer to big numbers (Balance amounts):
    DONE! Here is the [PR](https://github.com/capsule-corp-ternoa/ternoa-node/pull/306) for it.
  4. Usage of a global NFT/Collection/Marketplace id as a counter (u32, ~4.3B) can be handy for some logic but not that flexible.
    We don't see an exact usecase for it as of now. Please let us know an instance where this could be useful.
  5. Using StorageDoubleMap with {(collectionId, nftId) -> data} could be interesting. Or even a "triple" StorageMap, like pallet-nfts does here:
    That way users would be able to "mint" more than one NftId: 1. As each Collection would have its own first NFT (1).
    But in our case we have separate collection data as well which we are handling in Collections storage map. and in the NFTData we are mapping the collection_id to it like below. Even now users would be able to mint more than one Nft.

pub struct NFTData<AccountId, NFTOffchainDataLimit>
where
AccountId: Clone + PartialEq + Debug,
NFTOffchainDataLimit: Get,
{
/// NFT owner
pub owner: AccountId,
/// NFT creator
pub creator: AccountId,
/// NFT offchain_data
pub offchain_data: U8BoundedVec,
/// Collection ID
pub collection_id: Option,
/// Royalty
pub royalty: Permill,
/// NFT state
pub state: NFTState,
}

  1. Recommends moving all ensure!(nft, ...) duplicated blocks (checks) to a generic function, customized for each case.
    This could be done if there are many duplicated checks with same errors in multiple extrinsics. But most of the error we have defined are specific to each extrinsic even though some checks are duplicated.
  2. ensure_root() could be replaced by a more democratic Origin (like a Council) .
    This is our end goal but right now we are good with _ensure_root()_
  3. Calling an extrinsic from another is not a good practice. We'd recommend creating a simple Pallet function for that.
    DONE! Here is the [PR](https://github.com/capsule-corp-ternoa/ternoa-pallets/pull/72) for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant