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

NEP 22: Contract Update Standard #154

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

Conversation

superboyiii
Copy link
Member

Close: #135

nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
ixje
ixje previously approved these changes Aug 31, 2022
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.

LGTM

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

It'd be nice to have this, I'd change our examples to use this standard interface.

nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
nep-xx.mediawiki Outdated Show resolved Hide resolved
roman-khimov
roman-khimov previously approved these changes Sep 14, 2022
nep-xx.mediawiki Outdated Show resolved Hide resolved
@erikzhang
Copy link
Member

Add _initialize?

@roman-khimov
Copy link
Contributor

_initialize is something different to me, not directly related to deploy/update. Maybe it's in some other document (but I wasn't able to find it on https://docs.neo.org/) or other NEP.

@superboyiii
Copy link
Member Author

@roman-khimov @erikzhang Added. Please review again.

@roman-khimov
Copy link
Contributor

NEP-22:

provide systems with a generalized interaction mechanism for smart contract initial deploy, update and destroy.

I don't see _initialize being a part of this NEP.

@erikzhang
Copy link
Member

I don't see _initialize being a part of this NEP.

I'm not sure if it should be part of this PR, but obviously it should be included in a document.

Also, don't forget the verify method.

@superboyiii
Copy link
Member Author

I don't see _initialize being a part of this NEP.

I'm not sure if it should be part of this PR, but obviously it should be included in a document.

Also, don't forget the verify method.

Added.

@superboyiii
Copy link
Member Author

@erikzhang @roman-khimov Is it OK?

@roman-khimov
Copy link
Contributor

It's a method soup now to me. I'd rather have separate NEPs for _initialize, _deploy and verify (three NEPs, seems to be perfectly fine to me, they are different unrelated things) and one for update/destroy. I want to be able to specify "NEP-XYZ" in SupportedStandards and expect all the tooling around to understand that this contract has standardized update/destroy method pair that can be interacted with. "NEP-22" doesn't give this meaning right now.

@ixje
Copy link
Contributor

ixje commented May 8, 2023

_initialize is something different to me, not directly related to deploy/update. Maybe it's in some other document (but I wasn't able to find it on https://docs.neo.org/) or other NEP.

I can kind of see the relationship. update can call _deploy which in turn can call _initialize again. Ideally there would be a flow diagram included to make it clear. verify on the other hand seems totally unrelated and worthy of a separate document if we're splitting anyway

@roman-khimov
Copy link
Contributor

I can kind of see the relationship

Is it related? Probably so. Can be it be used without all the other things? Sure it is. Can all the other things in this proposal be used without _initialize? Sure they are. We can have as many standards as we need and they can be related, but different standards for different things. NEP-XYZ in SupportedStandards should have some specific meaning. And SupportedStandards can have as many NEP-X, NEP-Y, NEP-Z as needed.

@EdgeDLT
Copy link

EdgeDLT commented May 8, 2023

And SupportedStandards can have as many NEP-X, NEP-Y, NEP-Z as needed.

I agree in principle with the idea of having independent standards. But I suspect there are many others like myself who also have a mental block at the future thought of these long lists of numbers with no meaning. Maybe we could package them together under a standard group, but at that point, is it that much different from what we have now?

@Jim8y
Copy link
Contributor

Jim8y commented Jun 6, 2024

#154 (comment)

I can't imagine how to use this NEP in its current form. It needs a split.

@roman-khimov this one can be the most general description of base methods, as for specific method that should have a seperate NEP, we can create another NEPs for them, like Verify, no need to complete them in one NEP.

@superboyiii
Copy link
Member Author

#154 (comment)

I can't imagine how to use this NEP in its current form. It needs a split.

It's a guideline proposal for someone doesn't know where to start. It doesn't mean that you must implement all of these. In fact, you can even don't apply any of these, but it can remind these people some important points when starting to write neo smart contract.

@roman-khimov
Copy link
Contributor

Let's get back to #135 that started all of this for a moment. What people really wanted there is a convention for methods that contract authors can use to have some common way to update/destroy contracts. In this sense it's somewhat similar to NEP-11/17/24. And this is only about two methods: update and destroy, so that I wouldn't want to do

func obnovlyashka(data Array)

to update my contract (because 3rd-party tools couldn't easily create an update transaction for me in this case, unlike when they deal with a compliant update).

But what we also have here now are:

  • verify, which has nothing to do with update/destroy, it's just a different mechanism
  • _deploy, which is related to update, but is still somewhat different in that it can be omitted completely, it can be implemented in a contract that doesn't have update and, most importantly, it's not something you can call externally yourself, it's a native contract mechanism

At least we need to move verify out. It'd be perfect to have a NEP for it, but it's a separate matter. I would also move _deploy out since it has a broader definition (not just updates, but deployments as well, works irrespective of how your exposed-from-contract update methods are called), then this NEP could just refer to it.

@igormcoelho
Copy link

According to recent discussions, I agree that update and destroy are the most important here to be standardized.

@AnnaShaleva
Copy link
Member

AnnaShaleva commented Jul 18, 2024

Totally agree with Roman, we should adjust this NEP to standardize a single update method. The rest of methods standards should be moved to a separate granular NEPs.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jul 18, 2024

Should update method have a data parameter? Same for destroy. This has been requested from others in the pass to have one added to neo-cli, including neo-express.

@superboyiii superboyiii changed the title Draft NEP 22: Contract Basic Method Guideline NEP 22: Contract Update Standard Sep 11, 2024
@superboyiii
Copy link
Member Author

@roman-khimov @shargon @cschuchardt88 @Jim8y Now I changed it to Contract Update Standard. I will submit destroy standard as well.

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

@shargon @roman-khimov Done.

}
</pre>

This method will will callback <code>Update</code> method in native <code>ContractManagement</code> contract. It will update contract state if it's exectued successfully.
Copy link
Member

Choose a reason for hiding this comment

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

will will

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just MUST call


Updating a smart contract MUST have <code>nefFile</code> or <code>manifest.json</code>. If just update one of them at a time is OK, another SHOULD be set as null. It will be passed to <code>_deploy</code>.

The parameter <code>data</code> can be any type of supported parameters for contract-specific purpose. For example some kinds of json parameters can be put into data for specific check.
Copy link
Member

Choose a reason for hiding this comment

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

"For example some kinds of json parameters can be put into data for specific check."
Some examples here?


The parameter <code>data</code> can be any type of supported parameters for contract-specific purpose. For example some kinds of json parameters can be put into data for specific check.

This function SHOULD check whether the <code>signer</code> address equals the <code>owner</code> hash of contract. 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? Which function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better as this method, since we're talking about update method here. Also, I don't understand what is to check whether the <code>signer</code> address equals the <code>owner</code> hash of contract. Transaction can have a lot of signers. We should care more about owner allowing this action (CheckWitness). And NEP-30 is a different thing in general, most of contracts will not have contract-based owners, they're usually just multisig.


==Motivation==

As the NEO blockchain scales, Smart Contract Update will become increasingly important. We need standize <code>update</code> method in contract to callback native <code>ContractManagement</code> correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/NEO/Neo. s/standize/a standard/? I'm also not sure we need to say anything about callbacks. It's about standard API to update contracts, what it does internally is not motivation probably.


==Specification==

Neo N3 has a native <code>ContractManagement</code> contract that is used for contract deployment and update via <code>deploy</code> and <code>update</code> methods of it. After settling with the new NEF and manifest both of them will invoke a special <code>_deploy</code> method if it's implemented by the contract. More details in [NEP-29](https://github.com/neo-project/proposals/blob/master/nep-29.mediawiki)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a part of specification. We're specifying something different here.

}
</pre>

This method will will callback <code>Update</code> method in native <code>ContractManagement</code> contract. It will update contract state if it's exectued successfully.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just MUST call


This method will will callback <code>Update</code> method in native <code>ContractManagement</code> contract. It will update contract state if it's exectued successfully.

Updating a smart contract MUST have <code>nefFile</code> or <code>manifest.json</code>. If just update one of them at a time is OK, another SHOULD be set as null. It will be passed to <code>_deploy</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe refer to NEP-16 and NEP-15 here. I also don't understand

It will be passed to _deploy.

here. Only data is going to be passed and that's exactly where a reference to NEP-29 is needed.


The parameter <code>data</code> can be any type of supported parameters for contract-specific purpose. For example some kinds of json parameters can be put into data for specific check.

This function SHOULD check whether the <code>signer</code> address equals the <code>owner</code> hash of contract. 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
Contributor

Choose a reason for hiding this comment

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

Maybe better as this method, since we're talking about update method here. Also, I don't understand what is to check whether the <code>signer</code> address equals the <code>owner</code> hash of contract. Transaction can have a lot of signers. We should care more about owner allowing this action (CheckWitness). And NEP-30 is a different thing in general, most of contracts will not have contract-based owners, they're usually just multisig.

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.

Need a proposal for upgrade method in contract to unify the style