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

Destroy standard #182

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Destroy standard #182

wants to merge 12 commits into from

Conversation

superboyiii
Copy link
Member

No description provided.

nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
@superboyiii
Copy link
Member Author

@ixje Done.

Copy link
Contributor

@ixje ixje left a comment

Choose a reason for hiding this comment

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

Just 2 suggestions on wording.

nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
superboyiii and others added 2 commits September 12, 2024 16:59
Co-authored-by: ixje <git@erikvandenbrink.com>
Co-authored-by: ixje <git@erikvandenbrink.com>
@superboyiii
Copy link
Member Author

@ixje Can I add your name and email on Author?

@ixje
Copy link
Contributor

ixje commented Sep 12, 2024

No need. You authored it, I just gave some feedback.

Copy link
Contributor

@ixje ixje left a comment

Choose a reason for hiding this comment

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

👍

nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Show resolved Hide resolved
nep-X1.mediawiki Show resolved Hide resolved
nep-X1.mediawiki Outdated Show resolved Hide resolved
nep-X1.mediawiki Show resolved Hide resolved
nep-X1.mediawiki Show resolved Hide resolved
shargon and others added 5 commits September 13, 2024 01:11
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
Co-authored-by: Anna Shaleva <shaleva.ann@gmail.com>
@superboyiii
Copy link
Member Author

@AnnaShaleva All done.


==Motivation==

Contracts that are not relevant anymore may need to be destroyed. Neo N3 has provided contract destroy mechanism but it hasn't been summarized in a standard. This standard will show details.
Copy link
Member

Choose a reason for hiding this comment

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

There should be an incentive.

Copy link

Choose a reason for hiding this comment

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

Yep. Partial deploy cost refund, perhaps. But should we reward for freeing up all that state, too?

Copy link
Member

Choose a reason for hiding this comment

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

I think that some minimal GAS refund would be interesting

Copy link
Contributor

@roman-khimov roman-khimov Sep 30, 2024

Choose a reason for hiding this comment

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

It's a different topic, this NEP is about some standard interface provided by contracts. It stays the same irrespective of refunding.


<code>destroy</code> method can have multi parameters for any specific purpose since it's an operation only for contract owner.

This function SHOULD check whether the <code>signer</code> address equals the <code>owner</code> hash of contract to avoid security risks. This function SHOULD use the SYSCALL <code>Neo.Runtime.CheckWitness</code> to verify the <code>signer</code>. Details has been explained in [NEP-30](https://github.com/superboyiii/proposals/blob/upgrade-standard/nep-30.mediawiki).
Copy link
Member

Choose a reason for hiding this comment

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

This function? The same as I asked in the other PR of update.

</pre>
This method MUST invoke the <code>destroy</code> method of <code>ContractManagement</code> contract when the contract is destroyed.

<code>destroy</code> method can have multi parameters for any specific purpose since it's an operation only for contract owner.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid this and have a specific (parameterless) interface just like for update. Suppose I want to create a NEP-DESTROY-compatible CLI command, it'd be harder to do this if one contract has parameters and the other does not.

Copy link
Member

Choose a reason for hiding this comment

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

Should we strictly require zero parameters in the standard, or it is just a "SHOULD" requirement?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the practical value of parameters here? I can only imagine moving tokens from contract address before deletion (which might require some address), but likely they'll be sent to owner or this can be handled in a different manner by contract. Conditional (based on parameters) destroy? Not likely to be useful, owner can do anything already, so if he decides to destroy he will do this. So I'd go with MUST here.


This function SHOULD check whether the <code>signer</code> address equals the <code>owner</code> hash of contract to avoid security risks. This function SHOULD use the SYSCALL <code>Neo.Runtime.CheckWitness</code> to verify the <code>signer</code>. Details has been explained in [NEP-30](https://github.com/superboyiii/proposals/blob/upgrade-standard/nep-30.mediawiki).

This function is not allowed to call another contract when executing <code>destroy</code> method of native <code>ContractManagement</code> contract since <code>RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like details of ContractManagement operation, maybe this can be signified.

This function SHOULD check whether the <code>signer</code> address equals the <code>owner</code> hash of contract to avoid security risks. This function SHOULD use the SYSCALL <code>Neo.Runtime.CheckWitness</code> to verify the <code>signer</code>. Details has been explained in [NEP-30](https://github.com/superboyiii/proposals/blob/upgrade-standard/nep-30.mediawiki).

This function is not allowed to call another contract when executing <code>destroy</code> method of native <code>ContractManagement</code> contract since <code>RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify</code>

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be some recommendation on immediate exit after ContractManagement call. While technically we can still execute a lot of code afterwards, it's somewhat in the grey zone (see discussion in neo-project/neo#3290 (comment) as well), so I'd recommend to do any other actions before calling ContractManagement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants