rmrk-substrate icon indicating copy to clipboard operation
rmrk-substrate copied to clipboard

Improve nesting

Open Szegoo opened this issue 3 years ago • 5 comments

Closes: #223

Szegoo avatar Oct 23 '22 17:10 Szegoo

Just realized that this may be a bit early for a review(that is why I deleted the previous message), will ping you when it will be ready :)

Szegoo avatar Oct 23 '22 18:10 Szegoo

@ilionic This is ready for review, other than the burn_nft I couldn't find any other functions that have recursion. Are these the only ones?

Szegoo avatar Oct 24 '22 16:10 Szegoo

Thanks @Szegoo. That's correct, recursive calls are only related to burning and root owner lookup. Also please check unit tests:

error[E0046]: not all trait items implemented, missing: `NestingBudget`
   --> pallets/rmrk-market/src/mock.rs:107:1
    |
107 | impl pallet_rmrk_core::Config for Test {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `NestingBudget` in implementation

ilionic avatar Oct 24 '22 20:10 ilionic

Do we prevent this scenario when sending/minting NFTs to NFTs? If we are able to get a Budget from lookup_root_owner, could we prevent this scenario from happening?

Nested-Logic-Q

HashWarlock avatar Oct 25 '22 22:10 HashWarlock

If we are able to get a Budget from lookup_root_owner, could we prevent this scenario from happening?

Not sure I understand this totally correctly but did you mean that we should somehow be able to get the Budget that is needed to look up the owner?

Edit: Maybe the functions that deal with budget should make it possible(but optional) to define the budget. For example:

fn property_set(
	sender: T::AccountId,
	collection_id: CollectionId,
        maybe_nft_id: Option<NftId>,
	key: KeyLimitOf<T>,
	value: ValueLimitOf<T>,
        budget: Option<&dyn Budget> // <- we add this to the functions that deal with budget.
) -> DispatchResult {

If the function is called with None we simply use the NestingBudget. When we add benchmarks the weight could be calculated based on the budget provided.

This would make it possible to look up the owner for any asset(of course it will cost more if the nft is deeply nested)

Edit: AFAIU Doing nft_mint_directly_to_nft will fail if the nft that we are trying to mint up on is too deeply nested. Sorry if I didn't understand you correctly. @HashWarlock

Szegoo avatar Oct 26 '22 13:10 Szegoo