Skip to content

Conversation

@SupremaLex
Copy link
Contributor

Potential implementation of removal of resource feature

@SupremaLex
Copy link
Contributor Author

@ilionic can you review this pr?

@ilionic ilionic requested review from HashWarlock and bmacer March 24, 2022 18:14
ilionic
ilionic approved these changes Mar 29, 2022
Copy link
Contributor

@HashWarlock HashWarlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use some rework. I do not think there should be a need for PendingResourceRemovals. Instead there could be an added bool property to the ResourceInfo trait named pending_removal which is false by default.

if an account requests to remove_resource and the sender is not the root_owner then you can do:

Resources::<T>::try_mutate_exists(
		(collection_id, nft_id, resource_id.clone()),
		|resource| -> DispatchResult {
			if let Some(res) = resource {
				res.pending_removal = true;
			}
			Ok(())
		},
	)?;

remove from Resource if it is the root_owner that calls remove_resource

Then for accept_removal you can check if the pending_removal property is set to true in the Resources StorageNMap and remove the resource_id.

@HashWarlock
Copy link
Contributor

HashWarlock commented Mar 30, 2022

Also, based on the RMRK 2.0.0 spec ACCEPT if a resource is accepted in the past it is not possible to remove the resource in the future. The accept resource history is something that would need to be tracked in this case.

Removal of resources

It is not possible to remove resources accepted in the past. This prevents art rug-pulls.

@SupremaLex
Copy link
Contributor Author

Regarding the RMRK spec, Bruno said that resource removal wouldn't be added to the spec, but it is ok to add this feature to EVM&Substrate versions.

@SupremaLex SupremaLex requested a review from HashWarlock March 31, 2022 11:30
Copy link
Contributor

@HashWarlock HashWarlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question about if the Resource ID key would be removed from the StorageNMap. Then small suggestion for checking if a Resource StorageNMap contains_key() for the error checks. Other than that, I think it is good.

@SupremaLex SupremaLex requested a review from HashWarlock April 7, 2022 14:38
Copy link
Contributor

@HashWarlock HashWarlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my view.

@HashWarlock HashWarlock merged commit 27f9e27 into rmrk-team:main Apr 7, 2022
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.

3 participants