diff --git a/contracts/Orchestrator.sol b/contracts/Orchestrator.sol index 3f30cb6..fff0064 100644 --- a/contracts/Orchestrator.sol +++ b/contracts/Orchestrator.sol @@ -2,7 +2,6 @@ pragma solidity 0.8.2; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; -import {BytesLib} from "./_external/BytesLib.sol"; import {StringUtils} from "./_external/StringUtils.sol"; interface IUFragmentsPolicy { @@ -15,7 +14,6 @@ interface IUFragmentsPolicy { * actions with external consumers. */ contract Orchestrator is Ownable { - using BytesLib for bytes; using StringUtils for uint16; // Reference to the Ampleforth Policy @@ -23,8 +21,6 @@ contract Orchestrator is Ownable { struct Transaction { bool enabled; - // A failed Transaction marked critical will also cause rebase to fail. - bool critical; address destination; bytes data; } @@ -32,9 +28,6 @@ contract Orchestrator is Ownable { // Stable ordering is not guaranteed. Transaction[] public transactions; - // events - event TransactionFailed(uint16 index); - /** * @param policy_ Address of the UFragments policy. */ @@ -47,8 +40,6 @@ contract Orchestrator is Ownable { * The Orchestrator calls rebase on the policy and notifies downstream applications. * Contracts are guarded from calling, to avoid flash loan attacks on liquidity * providers. - * If a transaction marked 'critical' in the transaction list fails, - * Orchestrator will stop execution and revert. */ function rebase() external { require(msg.sender == tx.origin); // solhint-disable-line avoid-tx-origin @@ -58,16 +49,11 @@ contract Orchestrator is Ownable { for (uint16 i = 0; i < transactions.length; i++) { Transaction storage t = transactions[i]; if (t.enabled) { - (bool success, bytes memory reason) = t.destination.call(t.data); - - // Critical transaction failed, revert with message - if (!success && t.critical) { - revert(buildRevertReason(i, reason)); - } + (bool success, ) = t.destination.call(t.data); - // Non-Critical transaction failed, log error and continue + // Transaction failed, revert with message if (!success) { - emit TransactionFailed(i); + revert(buildRevertReason(i)); } } } @@ -78,14 +64,8 @@ contract Orchestrator is Ownable { * @param destination Address of contract destination * @param data Transaction data payload */ - function addTransaction( - bool critical, - address destination, - bytes memory data - ) external onlyOwner { - transactions.push( - Transaction({enabled: true, critical: critical, destination: destination, data: data}) - ); + function addTransaction(address destination, bytes memory data) external onlyOwner { + transactions.push(Transaction({enabled: true, destination: destination, data: data})); } /** @@ -114,18 +94,6 @@ contract Orchestrator is Ownable { transactions[index].enabled = enabled; } - /** - * @param index Index of transaction. Transaction ordering may have changed since adding. - * @param critical True for critical, false for non-critical. - */ - function setTransactionCritical(uint16 index, bool critical) external onlyOwner { - require( - index < transactions.length, - "Orchestrator: index must be in range of stored tx list" - ); - transactions[index].critical = critical; - } - /** * @return Number of transactions, both enabled and disabled, in transactions list. */ @@ -135,39 +103,15 @@ contract Orchestrator is Ownable { /** * @param txIndex The index of the failing transaction in the transaction array. - * @param reason The revert reason in bytes. - * @return Number of transactions, both enabled and disabled, in transactions list. + * @return The revert reason. */ - function buildRevertReason(uint16 txIndex, bytes memory reason) - internal - pure - returns (string memory) - { + function buildRevertReason(uint16 txIndex) internal pure returns (string memory) { return string( abi.encodePacked( - "Orchestrator: critical index:{job} reverted with:{reason}:", - txIndex.uintToBytes(), - "|", - revertReasonToString(reason) + "Orchestrator: transaction:{index} reverted:", + txIndex.uintToBytes() ) ); } - - /** - * @dev github.com/authereum/contracts/account/BaseAccount.sol#L132 - * @param reason The revert reason in bytes. - * @return The revert reason as a string. - */ - function revertReasonToString(bytes memory reason) internal pure returns (string memory) { - // If the reason length is less than 68, then the transaction failed - // silently (without a revert message) - if (reason.length < 68) return "Transaction reverted silently"; - - // Remove the selector which is the first 4 bytes - bytes memory revertData = reason.slice(4, reason.length - 4); - - // All that remains is the revert string - return abi.decode(revertData, (string)); - } } diff --git a/contracts/_external/BytesLib.sol b/contracts/_external/BytesLib.sol deleted file mode 100644 index 542bc9c..0000000 --- a/contracts/_external/BytesLib.sol +++ /dev/null @@ -1,83 +0,0 @@ -// SPDX-License-Identifier: Unlicensed -pragma solidity ^0.8.0; - -// https://github.com/GNSPS/solidity-bytes-utils/blob/master/contracts/BytesLib.sol -/* - * @title Solidity Bytes Arrays Utils - * @author Gonçalo Sá - * - * @dev Bytes tightly packed arrays utility library for ethereum contracts written in Solidity. - * The library lets you concatenate, slice and type cast bytes arrays both in memory - * and storage. - */ -library BytesLib { - function slice( - bytes memory _bytes, - uint256 _start, - uint256 _length - ) - internal - pure - returns (bytes memory) - { - require(_length + 31 >= _length, "slice_overflow"); - require(_start + _length >= _start, "slice_overflow"); - require(_bytes.length >= _start + _length, "slice_outOfBounds"); - - bytes memory tempBytes; - - assembly { - switch iszero(_length) - case 0 { - // Get a location of some free memory and store it in tempBytes as - // Solidity does for memory variables. - tempBytes := mload(0x40) - - // The first word of the slice result is potentially a partial - // word read from the original array. To read it, we calculate - // the length of that partial word and start copying that many - // bytes into the array. The first word we copy will start with - // data we don't care about, but the last `lengthmod` bytes will - // land at the beginning of the contents of the new array. When - // we're done copying, we overwrite the full first word with - // the actual length of the slice. - let lengthmod := and(_length, 31) - - // The multiplication in the next line is necessary - // because when slicing multiples of 32 bytes (lengthmod == 0) - // the following copy loop was copying the origin's length - // and then ending prematurely not copying everything it should. - let mc := add(add(tempBytes, lengthmod), mul(0x20, iszero(lengthmod))) - let end := add(mc, _length) - - for { - // The multiplication in the next line has the same exact purpose - // as the one above. - let cc := add(add(add(_bytes, lengthmod), mul(0x20, iszero(lengthmod))), _start) - } lt(mc, end) { - mc := add(mc, 0x20) - cc := add(cc, 0x20) - } { - mstore(mc, mload(cc)) - } - - mstore(tempBytes, _length) - - //update free-memory pointer - //allocating the array padded to 32 bytes like the compiler does now - mstore(0x40, and(add(mc, 31), not(31))) - } - //if we want a zero-length slice let's just return a zero-length array - default { - tempBytes := mload(0x40) - //zero out the 32 bytes slice we are about to return - //we need to do it because Solidity does not garbage collect - mstore(tempBytes, 0) - - mstore(0x40, add(tempBytes, 0x20)) - } - } - - return tempBytes; - } -} diff --git a/test/unit/Orchestrator.ts b/test/unit/Orchestrator.ts index 93ed9f0..6c1fff9 100644 --- a/test/unit/Orchestrator.ts +++ b/test/unit/Orchestrator.ts @@ -102,7 +102,7 @@ describe('Orchestrator', function () { ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateOneArgEncoded.data) + .addTransaction(mockDownstream.address, updateOneArgEncoded.data) r = orchestrator.connect(deployer).rebase() }) @@ -139,7 +139,7 @@ describe('Orchestrator', function () { ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateTwoArgsEncoded.data) + .addTransaction(mockDownstream.address, updateTwoArgsEncoded.data) r = orchestrator.connect(deployer).rebase() }) @@ -261,52 +261,19 @@ describe('Orchestrator', function () { }) }) - describe('when a non-critical transaction reverts', async function () { - before('adding 2 transactions', async function () { - expect(await orchestrator.transactionsSize()).to.eq(0) - - const updateOneArgEncoded = await mockDownstream.populateTransaction.updateOneArg( - 999, - ) - await orchestrator - .connect(deployer) - .addTransaction(true, mockDownstream.address, updateOneArgEncoded.data) - - const revertsEncoded = await mockDownstream.populateTransaction.reverts() - await orchestrator - .connect(deployer) - .addTransaction(false, mockDownstream.address, revertsEncoded.data) - - await expect(orchestrator.connect(deployer).rebase()) - // .not.to.be.reverted - .to.emit(orchestrator, 'TransactionFailed') - .withArgs(1) - }) - - it('should have 2 transactions', async function () { - expect(await orchestrator.transactionsSize()).to.eq(2) - }) - - after('removing 2 transactions', async function () { - await orchestrator.connect(deployer).removeTransaction(1) - await orchestrator.connect(deployer).removeTransaction(0) - expect(await orchestrator.transactionsSize()).to.eq(0) - }) - }) - - describe('when a critical transaction reverts with message', async function () { + describe('when a transaction reverts with message', async function () { before('adding 3 transactions', async function () { const updateOneArgEncoded = await mockDownstream.populateTransaction.updateOneArg( 123, ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateOneArgEncoded.data) + .addTransaction(mockDownstream.address, updateOneArgEncoded.data) const revertsEncoded = await mockDownstream.populateTransaction.reverts() await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, revertsEncoded.data) + .addTransaction(mockDownstream.address, revertsEncoded.data) const updateTwoArgsEncoded = await mockDownstream.populateTransaction.updateTwoArgs( 12345, @@ -314,7 +281,7 @@ describe('Orchestrator', function () { ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateTwoArgsEncoded.data) + .addTransaction(mockDownstream.address, updateTwoArgsEncoded.data) let exp try { @@ -324,7 +291,7 @@ describe('Orchestrator', function () { } expect(!exp).to.be.false expect(exp.message.replace(/\0/g, '')).to.eq( - 'VM Exception while processing transaction: revert Orchestrator: critical index:{job} reverted with:{reason}:1|reverted', + 'VM Exception while processing transaction: revert Orchestrator: transaction:{index} reverted:1', ) }) @@ -340,14 +307,14 @@ describe('Orchestrator', function () { }) }) - describe('when a critical transaction reverts without message', async function () { + describe('when a transaction reverts without message', async function () { before('adding 3 transactions', async function () { const updateOneArgEncoded = await mockDownstream.populateTransaction.updateOneArg( 555, ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateOneArgEncoded.data) + .addTransaction(mockDownstream.address, updateOneArgEncoded.data) const updateTwoArgsEncoded = await mockDownstream.populateTransaction.updateTwoArgs( 72, @@ -355,12 +322,12 @@ describe('Orchestrator', function () { ) await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, updateTwoArgsEncoded.data) + .addTransaction(mockDownstream.address, updateTwoArgsEncoded.data) const revertsEncoded = await mockDownstream.populateTransaction.revertsWithoutMessage() await orchestrator .connect(deployer) - .addTransaction(true, mockDownstream.address, revertsEncoded.data) + .addTransaction(mockDownstream.address, revertsEncoded.data) let exp try { @@ -370,7 +337,7 @@ describe('Orchestrator', function () { } expect(!exp).to.be.false expect(exp.message.replace(/\0/g, '')).to.eq( - 'VM Exception while processing transaction: revert Orchestrator: critical index:{job} reverted with:{reason}:2|Transaction reverted silently', + 'VM Exception while processing transaction: revert Orchestrator: transaction:{index} reverted:2', ) }) @@ -393,11 +360,7 @@ describe('Orchestrator', function () { await expect( orchestrator .connect(deployer) - .addTransaction( - true, - mockDownstream.address, - updateNoArgEncoded.data, - ), + .addTransaction(mockDownstream.address, updateNoArgEncoded.data), ).to.not.be.reverted }) @@ -406,11 +369,7 @@ describe('Orchestrator', function () { await expect( orchestrator .connect(user) - .addTransaction( - true, - mockDownstream.address, - updateNoArgEncoded.data, - ), + .addTransaction(mockDownstream.address, updateNoArgEncoded.data), ).to.be.reverted }) })