ampleforth-contracts icon indicating copy to clipboard operation
ampleforth-contracts copied to clipboard

Orchestrator V2

Open aalavandhan opened this issue 4 years ago • 3 comments

Contributors can pick up of the issues listed here (all are good first issues, great to get started):

  • [x] The Orchestrator was created to disregard failed transactions. However it was patched to revert if one of the listed transactions fail (to prevent a gas underprice attack). Update the inline code documentation to reflect the patched change].

  • [x] Replace externalCall with .call

  • [ ] The orchestrator imports the entire UFragmentsPolicy.sol. The typical way to address this is to just import the relevant interface, instead of importing the full contract. This helps reduce the deployed contract size.

  • [ ] Use the revert message to indicate the index of the external call transaction which failed

  • [ ] Use non upgradable version of Ownable in The Orchestrator contract.

  • [ ] The current Orchestrator contract uses require(msg.sender == tx.origin) to prevent contracts from calling rebase. But tx.origin may be deprecated in the future. Implement one of these alternate mechanisms described here.

aalavandhan avatar Jan 28 '21 01:01 aalavandhan

@nithinkrishna Can you explain a little more about Replace externalCall with .call? I see that externalCall function already has a .call function wrapped inside of the inline assembly.

tedwu13 avatar Feb 18 '21 06:02 tedwu13

@tedwu13 With solidity 0.4.24 we had to use assembly to make external calls. With later versions you can use address.call(data) to make an external function call.

The idea is to get rid of the externalCall method we've defined and just use the native solidity method.

https://docs.soliditylang.org/en/v0.7.0/control-structures.html?highlight=call#external-function-calls

# something like?
Transaction storage t = transactions[i];
(bool result, bytes memory reason) = t.destination.call(t.data);

aalavandhan avatar Feb 18 '21 18:02 aalavandhan

@nithinkrishna I think tx.origin is here to stay in order to protect backwards compatibility - see below. https://github.com/ethereum/solidity/issues/683

baribari1 avatar Mar 30 '21 18:03 baribari1