Send contract deploy notification before calling _deploy?

[ixje]

I've been meaning to ask this for a long time. For this piece of code

neo/src/Neo/SmartContract/Native/ContractManagement.cs

Lines 98 to 103 in a7db45c

	private async ContractTask OnDeploy(ApplicationEngine engine, ContractState contract, StackItem data, bool update)
	{
	ContractMethodDescriptor md = contract.Manifest.Abi.GetMethod("_deploy", 2);
	if (md is not null)
	await engine.CallFromNativeContract(Hash, contract.Hash, md.Name, data, update);
	engine.SendNotification(Hash, update ? "Update" : "Deploy", new VM.Types.Array(engine.ReferenceCounter) { contract.Hash.ToArray() });

Is there a specific reason why we are sending the Deploy notification after calling _deploy? If not, can we swap it around?

The current order has some side effects on the application logs. For example for the T5 transaction 0x208ba62b6d52410ed2b8fe36a2cc6f5072442d50e2b0967c7bd4c7a1f4e0fd7a the notifications list is as follows

"notifications": [
                    {
                        "contract": "0xb0868db5bc45836be80e7af20ff6cad192f0206a",
                        "eventname": "Transfer",
                        "state": {
                            "type": "Array",
                            "value": [
                                {
                                    "type": "Any"
                                },
                                {
                                    "type": "ByteString",
                                    "value": "BLh4wtjZB+zx1xiZD5A4Z8WSoRM="
                                },
                                {
                                    "type": "Integer",
                                    "value": "100000000"
                                }
                            ]
                        }
                    },
                    {
                        "contract": "0xfffdc93764dbaddd97c48f252a53ea4643faa3fd",
                        "eventname": "Deploy",
                        "state": {
                            "type": "Array",
                            "value": [
                                {
                                    "type": "ByteString",
                                    "value": "aiDwktHK9g/yeg7oa4NFvLWNhrA="
                                }
                            ]
                        }
                    }

The second notification is the deployment of contract 0xb0868db5bc45836be80e7af20ff6cad192f0206a, whereas the first notification is a transfer for the just deployed contract mentioned. In other words the order of notifications to me looks wrong because you can't transfer if the contract is not yet deployed (note: storing of the contract to the storage snapshot is done before we reach the code snippet above). Having this in chronological order would make it easier to consume the applicationlog as the hash in the contract field of a notification will always have an existing contract if parsed in order (because it will have had a deploy event prior to it).

[cschuchardt88]

The contract is deployed. This event is emitted when your _deploy is called. It can't call that method until the contract is deployed. After the contract is deployed it calls _deploy method of your contract. Thats why Transfer happens 1st. Its if you were to emit the event in your _deploy function in your contract. Order would be the same.

[ixje]

if you were to emit the event in your _deploy function in your contract. Order would be the same.

That's not what I'm suggesting. I'm suggesting to put

neo/src/Neo/SmartContract/Native/ContractManagement.cs

Line 103 in a7db45c

engine.SendNotification(Hash, update ? "Update" : "Deploy", new VM.Types.Array(engine.ReferenceCounter) { contract.Hash.ToArray() });

at the beginning of the function (before the _deploy call) instead of at the end.

[roman-khimov]

_deploy can ABORT and it can THROW, so we can say that the contract is not yet deployed until it's done. At the same time ContractManagement will fail the transaction if it throws an exception, so it'd be a FAULTed transaction with an event if we're to change the order (ABORT can also happen after ContactManagement invocation). And state (as in ContractManagement state) changes are made before the event emission, so in some ways it can be considered as deployed at this stage. Like, for example, we emit NEP-17 Transfer before calling onNEP17Payment.

At the same time, this behavior existed for quite some time already, some other code can depend on the order we have now.

[cschuchardt88]

On TestNet T5 i don't even get Transfer event. Is this normal? What node you accessing?

[ixje]

_deploy can ABORT and it can THROW, so we can say that the contract is not yet deployed until it's done. At the same time ContractManagement will fail the transaction if it throws an exception, so it'd be a FAULTed transaction with an event if we're to change the order (ABORT can also happen after ContactManagement invocation).

I can't tell whether you think this is a problem or not. The C# node does not store notifications for transactions that failed so the order there doesn't matter. One could even have a contract deploy that succeeds but some other contract call that happens after the deploy (in the same script) that fails.

At the same time, this behavior existed for quite some time already, some other code can depend on the order we have now.

I can't come up with an example what code would depend on order, got an example? When parsing notifications for Transfer events you can't tell where the contract Deploy notification will be. It's going to be somewhere between i+1 and max notificationSize.

[ixje]

On TestNet T5 i don't even get Transfer event. Is this normal? What node you accessing?

That is not normal. http://seed1t5.neo.org:20332

[roman-khimov]

I can't tell whether you think this is a problem or not.

At first I thought that THROW and exception handling might be a problem and the proposal should be rejected right away, but turns out that's not the case, so I've just tried to think about other potential consequences. At the moment I don't see any clear problems other than potential incompatibility.

It's just a conceptual question, when we consider contract to be deployed. If we're to compare it to NEP-17 then ContractManagement state change is sufficient. _deploy code can successfully call GetContract for itself or invoke some method via System.Contract.Call, for example, just like onNEP17Payment can see updated balances.

The C# node does not store notifications for transactions that failed

Meh. Hi, nspcc-dev/neo-go#3189. And no, I don't think C# node behavior is the best one wrt this. But it's a bit different story.

I can't come up with an example what code would depend on order, got an example?

Me neither, but some lazily coded thing that expects its deployment event in events[1] for your case can theoretically exist. At the same time this behavior can be easily controlled with HF, at least keeping old events as is.

[ixje]

Me neither, but some lazily coded thing that expects its deployment event in events[1] for your case can theoretically exist. At the same time this behavior can be easily controlled with HF, at least keeping old events as is.

Ah yes, so simple I didn't even consider that 😅
Mmh, updating this with a Hardfork would kind of beat the purpose (at least for me). As I'd still need to keep the code that deals with the old situation.

[cschuchardt88]

What you should do in the meantime is emit your own deploy event in _deploy method in your contract. I know it may sound kind of dumb. Plus the event can be looked up by your contract hash; instead of searching through ContractManagement hash. There would be 1000s of deploy events on ContractManagement. However I'm unsure if the Transfer event will still be 1st.

[dusmart]

In other words the order of notifications to me looks wrong because you can't transfer if the contract is not yet deployed (note: storing of the contract to the storage snapshot is done before we reach the code snippet above).

I'm not stand with you on this point. Actually you can calculate an contract address you will deploy to and transfer NEP-17 to the contract address first. Then deploy the contract successfully.