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 contract that handles destruction of resources with an optional callback interface #31

Closed
wants to merge 4 commits into from

Conversation

austinkline
Copy link
Contributor

No description provided.

Copy link

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Great work! I'd love to get something like this that we can deploy to the service account or another core flow account that so it can effectively be a core contract or something like that. What do you think about making this as a PR to flow-core-contracts? I could do it for you if that would be easier. I could make a FLIP about adding it to the service account

@austinkline
Copy link
Contributor Author

austinkline commented Jan 22, 2024

Great work! I'd love to get something like this that we can deploy to the service account or another core flow account that so it can effectively be a core contract or something like that. What do you think about making this as a PR to flow-core-contracts? I could do it for you if that would be easier. I could make a FLIP about adding it to the service account

Not opposed to this living elsewhere, is there a way for us to structure this such that it takes the FungibleToken burn method as well? We had some work to do there to get event emissions when a non-0 vault was burned anyway. Is this a sufficient to get us there?

I'll make the PR, good chance to contribute to flow-core-contracts finally

Copy link

@FelipeRibeiroLabs FelipeRibeiroLabs left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you @austinkline

@austinkline
Copy link
Contributor Author

Made some new adjustments to handle Array and Dictionary types since those are resources but wouldn't handle their respective callbacks appropriately. I'm not sure if all the key types are covered or not, but this is a good starting point

@turbolent
Copy link
Contributor

I'm a bit worried to call this "safe", as it still allows arbitrary code to be run, which could be re-entrant. This was one of the reasons why custom resource destructors where removed.

cadence/contracts/Burner.cdc Outdated Show resolved Hide resolved
cadence/contracts/Burner.cdc Outdated Show resolved Hide resolved
cadence/transactions/burner/create_and_destroy_dict.cdc Outdated Show resolved Hide resolved
@austinkline
Copy link
Contributor Author

I'm a bit worried to call this "safe", as it still allows arbitrary code to be run, which could be re-entrant. This was one of the reasons why custom resource destructors where removed.

I'll rename the test contract, but @bluesign and I discussed names last night and settled on:

  • Burner (contract)
  • Burnable (interface)
  • burn (global method)

Any concerns with that?

@austinkline austinkline changed the title add contract that handles safe destruction of resources add contract that handles destruction of resources with an optional callback interface Jan 23, 2024
@austinkline
Copy link
Contributor Author

@joshuahannan made a PR to the core contracts repo here:
onflow/flow-core-contracts#407

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.

5 participants