-
Notifications
You must be signed in to change notification settings - Fork 6
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
Daanvdplas
merged 9 commits into
daan/fix-read_state_error_handling
from
daan/feat-asset_management
Aug 6, 2024
Merged
feat: asset management #151
Daanvdplas
merged 9 commits into
daan/fix-read_state_error_handling
from
daan/feat-asset_management
Aug 6, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Daanvdplas
force-pushed
the
daan/feat-asset_management
branch
from
July 31, 2024 15:36
f9ffe65
to
772514c
Compare
chungquantin
approved these changes
Aug 2, 2024
peterwht
reviewed
Aug 2, 2024
There was a problem hiding this 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:
- The integration test contract is only testing
create
and asset exists, is this on purpose? - The new formatting would have been nice to do in a different PR. Made this one noisier than it needed to be.
Daanvdplas
merged commit Aug 6, 2024
5b78e4c
into
daan/fix-read_state_error_handling
6 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the functionality for asset management:
Additional changes:
DEPRECATED
to old examples and automatic formatting was applied.utils.rs
.The dispatchables required to destroy an asset but not added to the api are
destroy_accounts
,destroy_approvals
andfinish_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