Replace custom external call with native solidity .call #204
Conversation
contracts/Orchestrator.sol
Outdated
| @@ -47,7 +47,7 @@ contract Orchestrator is Ownable { | |||
| for (uint256 i = 0; i < transactions.length; i++) { | |||
| Transaction storage t = transactions[i]; | |||
| if (t.enabled) { | |||
| bool result = externalCall(t.destination, t.data); | |||
| (bool result, bytes memory reason) = t.destination.call(t.data); | |||
| if (!result) { | |||
| emit TransactionFailed(t.destination, i, t.data); | |||
There was a problem hiding this comment.
With the new flow, if one of the transactions reverts rebase reverts.
The TransactionFailed log message will not be emitted when transaction reverts, it can be removed.
There was a problem hiding this comment.
If I remove the log message, should I also remove the TransactionFailed event or do we listen to that event anywhere else? https://github.com/ampleforth/uFragments/blob/07e229b2c3dd23b1b55ed871207196179fdfb5e6/contracts/Orchestrator.sol#L19
There was a problem hiding this comment.
I think we can remove it. The contract doesn't really use it so there's not need for it.
Services do listen to this, however the next orchestrator version will be a fresh contract deployment so it shouldn't break anything :)
There was a problem hiding this comment.
awesome will remove it.
contracts/Orchestrator.sol
Outdated
| @@ -47,7 +47,7 @@ contract Orchestrator is Ownable { | |||
| for (uint256 i = 0; i < transactions.length; i++) { | |||
| Transaction storage t = transactions[i]; | |||
| if (t.enabled) { | |||
| bool result = externalCall(t.destination, t.data); | |||
| (bool result, bytes memory reason) = t.destination.call(t.data); | |||
There was a problem hiding this comment.
Seems like we're not using reason, so maybe not reference it?
(bool result, ) = t.destination.call(t.data);
There was a problem hiding this comment.
yeah good call I'll remove it
|
@tedwu13 looks mostly good. Left a couple of comments 👍 |
hardhat.config.ts
Outdated
| @@ -3,26 +3,26 @@ import { HardhatUserConfig } from 'hardhat/config' | |||
| import '@nomiclabs/hardhat-ethers' | |||
| import '@nomiclabs/hardhat-waffle' | |||
| import '@openzeppelin/hardhat-upgrades' | |||
| import "@nomiclabs/hardhat-etherscan"; | |||
There was a problem hiding this comment.
Hmm. Can you leave the changes in this file out to keep the diff clean?
Seems like they've been added by the linter
tedwu13
force-pushed
the
tedwu-change-external-call-to-call
branch
from
d7a03bf to
8d1e556
Compare
Overview
Replacing custom externalCall function with the native solidity .call. With older versions of solidity (0.4.24) inline assembly is used to make external function calls. Now solidity supports external function calls. Removing the externalCall function because it is not being used in the repo. Let me know if I should keep the externalCall function.
Based on the documentation, .call returns a boolean and bytes of memory.
<address>.call(bytes memory) returns (bool, bytes memory)Changes
Reviewers
@nithinkrishna