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

Allow deploy native contracts in any block (only once) #2056

Closed
wants to merge 17 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Nov 11, 2020

Close #2055

@erikzhang
Copy link
Member

I prefer to use System.Contract.Create.

And how do onPersist() and postPersist() work?

@Qiao-Jin
Copy link
Contributor

I think logic of onPersistScript and postPersistScript should also be changed.
Besides, do we need some checks, i.e. CheckCommittee for Neo_Native_Deploy?

@shargon
Copy link
Member Author

shargon commented Nov 12, 2020

I prefer to use System.Contract.Create.

Remove Neo_Native_Deploy and generate a native NEFFile?
On persist logic can be changed, but it's easy to do a conditional. if (height > X) onPersist=...

@erikzhang
Copy link
Member

Remove Neo_Native_Deploy and generate a native NEFFile?

Yes. But let's wait for #2044.

@Tommo-L Tommo-L mentioned this pull request Nov 19, 2020
44 tasks
@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 25, 2020

Ping

We need to merge this pr in preview4.

@shargon
Copy link
Member Author

shargon commented Nov 25, 2020

@Tommo-L We need #2044 I will move the code to onPersist

@shargon shargon changed the title Allow deploy native contracts in all blocks (only once) Allow deploy native contracts in any block (only once) Nov 30, 2020
@shargon
Copy link
Member Author

shargon commented Dec 1, 2020

Ready to review again

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 1, 2020

Remove Neo_Native_Deploy and generate a native NEFFile?

Yes. But let's wait for #2044.

@shargon miss nef file?

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 1, 2020

Remove Neo_Native_Deploy and generate a native NEFFile?

If so, we should add Update/Destroy methods in native contract.

@shargon
Copy link
Member Author

shargon commented Dec 1, 2020

OnPersist has no Tx, and the hash it's related to the sender, so we can't use Contract.Create like this...

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 2, 2020

OnPersist has no Tx, and the hash it's related to the sender, so we can't use Contract.Create like this...

I think we don't need to deploy it in OnPersis. If we use Contract.Create, just execute the tx.script is enough.

@shargon
Copy link
Member Author

shargon commented Dec 2, 2020

@Tommo-L Then we need GAS in the deploy transaction, and the sender has no gas....

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 2, 2020

@Tommo-L Then we need GAS in the deploy transaction, and the sender has no gas....

No need to charge for the txs in genesis block.

@erikzhang
Copy link
Member

If so, we should add Update/Destroy methods in native contract.

Why do we need Update/Destroy?

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 2, 2020

If so, we should add Update/Destroy methods in native contract.

Why do we need Update/Destroy?

Just like a normal contract upgrade, we don't have to worry about the impact of the state, due to the native contract upgrade.

@erikzhang
Copy link
Member

But the code is not in the script. It is in the native code.

@@ -6,24 +6,24 @@ namespace Neo.SmartContract
{
partial class ApplicationEngine
{
public static readonly InteropDescriptor Neo_Native_Deploy = Register("Neo.Native.Deploy", nameof(DeployNativeContracts), 0, CallFlags.AllowModifyStates, false);
public static readonly InteropDescriptor Neo_Native_Deploy = Register("Neo.Native.Deploy", nameof(DeployNativeContract), 0, CallFlags.AllowModifyStates, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need Neo.Native.Update ? How do we update native contract?

Copy link
Member Author

@shargon shargon Dec 3, 2020

Choose a reason for hiding this comment

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

I think that we should do a condition inside the method, and change the method hash in order to include the arguments, in this way we can have method overloads (or just use other name).

@roman-khimov
Copy link
Contributor

I think this can be simplified by using the following model:

  • every native contract has an onPersist method
  • onPersistScript invokes all of them (iterating over NativeContract.Contracts, in their natural ID-based order)
  • in onPersist handler native contract can check for PersistingBlock.Index == 0 (or any other block number where this contract should be activated) and deploy itself if needed

This:

  • allows to avoid if block.Index ... logic in Persist (think of new contracts added and new branches there), moving it to specific contracts
  • allows for contract to actually destroy itself if needed at certain height
  • follows OnPersist semantics for native contracts #1913
  • allows for Contract to manage contracts #2000 (contract-managing contract will be the first one to execute in genesis onPersist, so it can deploy itself using internal methods and make it possible for other contracts to use its public methods afterwards)

@erikzhang
Copy link
Member

Please check #2119

@erikzhang erikzhang deleted the change-native-deploy branch January 26, 2021 04:00
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.

Change Native.Deploy
5 participants