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-Y: add _deploy NEP #179

Merged
merged 6 commits into from
Aug 30, 2024
Merged

Conversation

roman-khimov
Copy link
Contributor

NEP-22 can reference it. This mostly follows the implementation, there is nothing really new here.

NEP-22 can reference it. This mostly follows the implementation, there is
nothing really new here.

Signed-off-by: Roman Khimov <[email protected]>
@roman-khimov
Copy link
Contributor Author

Refs. #154.

Jim8y
Jim8y previously approved these changes Jul 25, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Jul 25, 2024

See, man, here is the problem, you dont have an NEP number,,,,, and we have to approve it without the number,,, then when its acctepted, you will need to update it with a number,,,, then all of us have to reapprove it again...... you know how hard it is to collect review and approves.

Signed-off-by: Roman Khimov <[email protected]>
Jim8y
Jim8y previously approved these changes Jul 26, 2024
shargon
shargon previously approved these changes Aug 1, 2024
nep-29.mediawiki Outdated Show resolved Hide resolved
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

LGTM.

nep-Y.mediawiki Outdated Show resolved Hide resolved
Comment on lines +26 to +42
<pre>
{
"name": "_deploy",
"safe": false,
"parameters": [
{
"name": "data",
"type": "Any"
},
{
"name": "update",
"type": "Boolean"
}
],
"returntype": "Void"
}
</pre>
Copy link
Member

Choose a reason for hiding this comment

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

{
  "name": "_deploy",
  "safe": false,
  "parameters": [
    {
      "name": "data",
      "type": "Any"
    },
    {
      "name": "update",
      "type": "Boolean"
    }
  ],
  "returntype": "Void"
}

Copy link
Member

Choose a reason for hiding this comment

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

syntax highlighting would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nep-29.mediawiki Outdated

==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 can invoke a special <code>_deploy</code> method if it's implemented by the contract.
Copy link
Member

Choose a reason for hiding this comment

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

Neo N3 has a native ContractManagement contract that is used for contract deployment (deploy) and update (update) methods. After settling with the new NEF and Manifest, both of them can invoke a special _deploy method (if it's implemented by the contract).

Copy link
Member

Choose a reason for hiding this comment

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

@roman-khimov, consider this new sentence.

One question.
"both of them can invoke a special¨, you mean, if the developer implements the _deploy method, it is still optional to call it from Deploy/Update,right?
It is not automatically and compulsory, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

consider this new sentence.

It was phrased like that in one of the earlier drafts, but I wanted to avoid "deployment (deploy)/update (update)". Pure stylistic, I like the way it's in the PR, but I can change it as well if there is a wider agreement that your version is better.

if the developer implements the _deploy method

Then it will always be called. Maybe s/can/will/ makes sense here, a bit stronger causality (if implemented -> will invoke).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Now we need more approvals.

AnnaShaleva
AnnaShaleva previously approved these changes Aug 12, 2024
Use more strict wording to clarify how it works.

Signed-off-by: Roman Khimov <[email protected]>
AnnaShaleva
AnnaShaleva previously approved these changes Aug 12, 2024
Jim8y
Jim8y previously approved these changes Aug 13, 2024
shargon
shargon previously approved these changes Aug 13, 2024
@igormcoelho
Copy link

My only recommendation here is to include an example of of the data on _deploy, something that could demonstrate its usefulness, and if possible, add a C# reference to the method implementation as well. Minor comments, author can decide to follow them or not. For me it's accepted.

@roman-khimov roman-khimov dismissed stale reviews from shargon, Jim8y, and AnnaShaleva via f318350 August 15, 2024 14:11
@roman-khimov
Copy link
Contributor Author

an example of of the data on _deploy

This better be in implementations, some of them do use it.

C# reference to the method

Added! It's not used often, so it takes some time to find any implementation.

nep-29.mediawiki Outdated
}
</pre>

This method will be automatically executed by ContractManagement contract when a contract is first deployed or updated. At this point new contract state is already saved (and it can be called from other code), but <code>Deploy</code> or <code>Update</code> notification is not yet emitted by ContractManagement contract.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This method will be automatically executed by ContractManagement contract when a contract is first deployed or updated. At this point new contract state is already saved (and it can be called from other code), but <code>Deploy</code> or <code>Update</code> notification is not yet emitted by ContractManagement contract.
This method will be automatically executed by <code>ContractManagement</code> contract when a contract is first deployed or updated. At this point new contract state is already saved (and it can be called from other code), but <code>Deploy</code> or <code>Update</code> notification is not yet emitted by <code>ContractManagement</code> contract.

wrap ContractManagement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

nep-29.mediawiki Outdated

<code>_deploy</code> parameters are made to be generic enough to cover all contract state management cases, it is possible to get additional per-network or per-instance contract data for initialization and updates can be easily differentiated from deployments via <code>update</code> parameter.

Notice that if there are any notifications emitted during <code>_deploy</code> execution they will precede <code>Deploy</code> or <code>Update</code> notifications from ContractManagement. This behavior is historic, it mostly means that we're considering contract to be completely ready after succesful <code>_deploy</code> invocation (it can throw an exception or abort execution as well).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Notice that if there are any notifications emitted during <code>_deploy</code> execution they will precede <code>Deploy</code> or <code>Update</code> notifications from ContractManagement. This behavior is historic, it mostly means that we're considering contract to be completely ready after succesful <code>_deploy</code> invocation (it can throw an exception or abort execution as well).
Notice that if there are any notifications emitted during <code>_deploy</code> execution they will precede <code>Deploy</code> or <code>Update</code> notifications from <code>ContractManagement</code>. This behavior is historic, it mostly means that we're considering contract to be completely ready after succesful <code>_deploy</code> invocation (it can throw an exception or abort execution as well).

wrap ContractManagement

nep-29.mediawiki Outdated Show resolved Hide resolved
nep-29.mediawiki Outdated
==Implementation==

* https:/nspcc-dev/neofs-contract/blob/99fb86c35a48ed12017423aa4fee13f7d07fa4c0/contracts/netmap/contract.go#L79
* https:/neo-project/non-native-contracts/blob/8d72b92e5e5705d763232bcc24784ced0fb8fc87/src/NameService/NameService.cs#L439
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* https:/neo-project/non-native-contracts/blob/8d72b92e5e5705d763232bcc24784ced0fb8fc87/src/NameService/NameService.cs#L439
* C# .NET - https:/neo-project/non-native-contracts/blob/8d72b92e5e5705d763232bcc24784ced0fb8fc87/src/NameService/NameService.cs#L439

And reoder them alphabetically.

Signed-off-by: Roman Khimov <[email protected]>
Sometimes it's <code></code>, sometimes it's not, make it uniform.

Signed-off-by: Roman Khimov <[email protected]>
nep-29.mediawiki Show resolved Hide resolved
@superboyiii
Copy link
Member

Seems good to merge.

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