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

Add TransferHooks, Budget fix and upgrade to Polkadot v0.9.33 #256

Closed
wants to merge 11 commits into from

Conversation

HashWarlock
Copy link
Contributor

@HashWarlock HashWarlock commented Dec 19, 2022

Description

TransferHooks implementation

For implementations downstream of the RMRK pallet there needs to be an ability to add custom logic in the case that there are multiple pallets using the RMRK NFT pallet, but want to keep the business logic mutually exclusive. This implementation allows for a trait called TransferHooks to be implemented with 2 functions pre_check and post_transfer that default to true when not implemented.

Budget trait fix

There are 2 problems with the current Budget trait:

NFTs are currently allowed to mint and send NFTs to a parent NFT that can meet up to the threshold of NestingBudget::get() + 1. This allows for NFTs queries like lookup_root_owner to return errors like TooManyRecursions. Having this error is okay, but the NFTs should be prevented from being minted to or sent to a parent NFT if the Budget threshold is met.

Currently the code will do a calculation on functions like burn_nft and this will perform an arithmetic subtraction at the end of the function, but this should be added as a field in the Value type & implement a function from Budget to get the budget that has been consumed and budget that is left.

Targets

  • Upgrade to polkadot-v0.9.33
  • implement TransferHooks trait with pre_check and post_transfer functions
  • Update integration tests
  • Fix minting/sending NFTs that exceed the threshold of NestingBudget::get() value.
  • Add fields names budget_consumed and budget_left` to track & through errors is an overflow is detected.
  • Add functions in Budget trait to get the 2 new fields.
  • Fix benchmarking errors from Traits being renamed

Comment on lines +12 to +13
fn budget_left_value(&self) -> u32 {
self.get_budget_left_value()
Copy link
Contributor

Choose a reason for hiding this comment

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

A better name for this may be budget_remaining_value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen cases for both namings. We currently use it in production for Phala, but I can make the change if we want to use remaining vs left

Copy link
Contributor

Choose a reason for hiding this comment

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

If we already use it in production for Phala it is probably better to keep it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilionic can weigh-in on this. The change isn't too major, so I think it should be okay if there is a preference and we will make updates through runtime upgrade in the future.

@HashWarlock
Copy link
Contributor Author

HashWarlock commented Dec 28, 2022

I'm gonna create another PR to update to polkadot-v0.9.34 with the benchmarking changes. We can close this one afterwards.

@HashWarlock
Copy link
Contributor Author

Closing in favor of #259

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.

None yet

3 participants