Skip to content

Commit

Permalink
updated error handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@aalavandhan
aalavandhan committed Apr 5, 2021
1 parent 5923aff commit 542d176
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 203 deletions.
74 changes: 9 additions & 65 deletions contracts/Orchestrator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -15,26 +14,20 @@ interface IUFragmentsPolicy {
* actions with external consumers.
*/
contract Orchestrator is Ownable {
using BytesLib for bytes;
using StringUtils for uint16;

// Reference to the Ampleforth Policy
address public policy;

struct Transaction {
bool enabled;
// A failed Transaction marked critical will also cause rebase to fail.
bool critical;
address destination;
bytes data;
}

// Stable ordering is not guaranteed.
Transaction[] public transactions;

// events
event TransactionFailed(uint16 index);

/**
* @param policy_ Address of the UFragments policy.
*/
Expand All @@ -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
Expand All @@ -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));
}
}
}
Expand All @@ -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}));
}

/**
Expand Down Expand Up @@ -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.
*/
Expand All @@ -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));
}
}
83 changes: 0 additions & 83 deletions contracts/_external/BytesLib.sol
View file

This file was deleted.

69 changes: 14 additions & 55 deletions test/unit/Orchestrator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})

Expand Down Expand Up @@ -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()
})

Expand Down Expand Up @@ -261,60 +261,27 @@ 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,
23456,
)
await orchestrator
.connect(deployer)
.addTransaction(true, mockDownstream.address, updateTwoArgsEncoded.data)
.addTransaction(mockDownstream.address, updateTwoArgsEncoded.data)

let exp
try {
Expand All @@ -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',
)
})

Expand All @@ -340,27 +307,27 @@ 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,
33,
)
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 {
Expand All @@ -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',
)
})

Expand All @@ -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
})

Expand All @@ -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
})
})
Expand Down

0 comments on commit 542d176

Please sign in to comment.