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

feat: asset management #151

Merged

Conversation

Daanvdplas
Copy link
Collaborator

@Daanvdplas Daanvdplas commented Jul 31, 2024

Implements the functionality for asset management:

  • create
  • start_destroy
  • set_metadata
  • clear_metadata
  • integration test for creating an asset in the constructor

Additional changes:

  • added DEPRECATED to old examples and automatic formatting was applied.
  • refactored the integration tests helper functions in a separate file utils.rs.

The dispatchables required to destroy an asset but not added to the api are destroy_accounts, destroy_approvals and finish_destroy. They have not been included in the api because they do not have to be signed by the owner of the asset and cause complexities in the implementation*. Asset owners (in this case via a contract) are required to complete the steps of destroying an asset via pallet assets in stead of the contract (as it is normally also the case). Only starting the destroying process of an asset has to be initiated by the contract if it is the owner of the asset. If developers want this implemented in the api, we can always do this. Yet now due to the complexity and because destroying assets doesn't happen often we leave it as is. This has to be documented, an issue is created #152.

*It requires a config type RemoveItemsLimit (same as pallet assets) which has to be configured to a smaller amount than in pallet assets. RemoveItemsLimit is the max. # of accounts and approvals that can be removed in a single tx. Due to the call being made by a contract this has to be configured to a smaller amount in the api::fungibles pallet.

Contract examples showing how to to use the create(), whether in the constructor or not (or not at all, instantiating a contract which wraps around an existing asset), have been included in #108.

Closes #92

Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

Nicely done. Cool to see how clean it was to add in the new functionality.

Two comments:

  1. The integration test contract is only testing create and asset exists, is this on purpose?
  2. The new formatting would have been nice to do in a different PR. Made this one noisier than it needed to be.

@Daanvdplas Daanvdplas merged commit 5b78e4c into daan/fix-read_state_error_handling Aug 6, 2024
6 checks passed
@Daanvdplas Daanvdplas deleted the daan/feat-asset_management branch August 6, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat (pop api): implement asset management interfaces local fungibles
3 participants