[EVM] Contract deploy change return value

[devbugging]

Closes: #5505

❗️ API Breaking Change

The field is optional

        access(all)
        let deployedContract: EVMAddress?

Change in the EVM COA deploy function return value, previously it returned a byte array containing the newly deployed contract address, now it returns the result type that is consistent with run and call functions. The address is stored as result data.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.55%. Comparing base (5922cda) to head (0a0b559).
Report is 354 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5606       +/-   ##
===========================================
+ Coverage   55.65%   77.55%   +21.89%     
===========================================
  Files        1037        6     -1031     
  Lines      101377      392   -100985     
===========================================
- Hits        56424      304    -56120     
+ Misses      40613       71    -40542     
+ Partials     4340       17     -4323     

Flag	Coverage Δ
unittests	77.55% <ø> (+21.89%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ramtinms]

I think we also need to update the tests in the handler and ideally an integration test in the evm_test

[turbolent]

Nice! Please also amend the FLIP accordingly

[devbugging]

Nice! Please also amend the FLIP accordingly

Good point, updated in onflow/flips@e0122e0

[m-Peter]

I have one concern about making this change here. Right now, a contract deployment results in the following evm.TransactionExecuted event payload:

evm.TransactionExecuted(
    blockHeight: 3,
    blockHash: "0xc9a60a5503928c637fbed960b8224e8786f147d1e9a4b6245527f50df0e7fbad",
    transactionHash: "0xe414f90fea2aebd75e8c0b3b6a4a0c9928e86c16ea724343d884f40bfe2c4c6b",
    transaction: "f9015880808301e8488080b901086060604052341561000f57600080fd5b60eb8061001d6000396000f300606060405260043610603f576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff168063c6888fa1146044575b600080fd5b3415604e57600080fd5b606260048080359060200190919050506078565b6040518082815260200191505060405180910390f35b60007f24abdb5865df5079dcc5ac590ff6f01d5c16edbc5fab4e195d9febd1114503da600783026040518082815260200191505060405180910390a16007820290509190505600a165627a7a7230582040383f19d9f65246752244189b02f56e8d0980ed44e7a56c0b200458caad20bb002982052fa09c05a7389284dc02b356ec7dee8a023c5efd3a9d844fa3c481882684b0640866a057e96d0a71a857ed509bb2b7333e78b2408574b8cc7f51238f25c58812662653",
    failed: false,
    vmError: "",
    transactionType: 0,
    gasConsumed: 103856,
    deployedContractAddress: "0x99466ED2E37B892A2Ee3E9CD55a98b68f5735db2",
    returnedValue: "606060405260043610603f576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff168063c6888fa1146044575b600080fd5b3415604e57600080fd5b606260048080359060200190919050506078565b6040518082815260200191505060405180910390f35b60007f24abdb5865df5079dcc5ac590ff6f01d5c16edbc5fab4e195d9febd1114503da600783026040518082815260200191505060405180910390a16007820290509190505600a165627a7a7230582040383f19d9f65246752244189b02f56e8d0980ed44e7a56c0b200458caad20bb0029",
    logs: ""
)

Where returnedValue holds the executed byte-code, basically what should be returned by eth_getCode for the deployed contract address.

With res.ReturnedValue = to[:], we will basically return the deployed contract address for both deployedContractAddress & returnedValue.

This results in less data being available in the emitted event. For example, the Gateway can use the current event payload and index the executed byte-code for each address.

[devbugging]

That is true and I would say the only way to resolve this would to add the deployed address value to the EVM.Result type, the other way is to revert to overwrite the ReturnedValue in the contract handler no in emulator. cc @ramtinms

[devbugging]

@m-Peter I added deployed contract address as the new field on the result type

[turbolent]

Left a question in the FLIP: onflow/flips#225 (comment)

[turbolent]

@sideninja Thanks for the update of the FLIP! Maybe an accident, but onflow/flips@e0122e0 also reverted all the Cadence 1.0 changes (access modifiers with entitlements, view annotations, etc.). Could you please revert those?

[sisyphusSmiling]

I'm a bit late to the party here - what is this value for CadenceOwnedAccount.call - I'd imagine a 0 array since the field isn't optional?

[devbugging]

I will make it optional, it's a good suggestion

[sisyphusSmiling]

Perhaps more preference than anything, I feel like this field should be converted in init and stored as EVMAddress since that's what it ultimately is and will be used as in Cadence.

I'm also wondering if this field should be optional in the event deployment fails assuming failure doesn't revert. If this value is a 0 array on deployment failure, that could be a potential footgun if a dev doesn't check result.status == EVM.Status.successful and just blindly assigns the deployedContractAddress. If it's optional, operations would at least revert on forced optional failure.

[devbugging]

very good point!

[devbugging]

The deployed contract field on Result type was changed to be an optional as per @sisyphusSmiling comment.
cc @ramtinms

[sisyphusSmiling]

Result looks good, thanks for the updates!