Skip to content
This repository has been archived by the owner on May 9, 2024. It is now read-only.

Commit

Permalink
Fix Proposals cannot be cancelled (5.6 – major) (#156)
Browse files Browse the repository at this point in the history
* proposals can now be cancelled, added in proposal cancelled event

* fix build issues

* update bridge constuctors in all tests

* fix existing bridge test

* added stests for cancellation

* fix cli

* await in async

* helpers

* spelling errors

* remove silent

* update test, doesn't pass locally but no issue connecting to ganache

* wait did this work:

* typo

* works now, fixed provider

* added additional tests for relayer and admin cancel functions

* added admin cli command to cancel proposal

* internal function to cancel proposal

* use adminOrRelayer modifier

* fix indentation and comment
  • Loading branch information
steviezhang authored May 28, 2020
1 parent 3da7646 commit 5fbc2be
Show file tree
Hide file tree
Showing 33 changed files with 287 additions and 53 deletions.
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ jobs:
env: CI=true
script:
- make install-deps
- SILENT=true make start-ganache
- make test
# - name: "Run Truffle tests (Geth)"
# install:
Expand Down
16 changes: 16 additions & 0 deletions cli/cmd/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,26 @@ const setBurnCmd = new Command("set-burn")

})

const cancelProposalCmd = new Command("cancel-proposal")
.description("cancel a proposal past the expiry threshold")
.option('--bridge <address>', 'Custom bridge address', constants.BRIDGE_ADDRESS)
.option('--chainID <int>', 'chainID of proposal to cancel', 0)
.option('--depositNonce <int>', 'depositNonce of proposal to cancel', 0)
.action(async function (args) {
await setupParentArgs(args, args.parent.parent)

const bridgeInstance = new ethers.Contract(args.bridge, constants.ContractABIs.Bridge.abi, args.wallet);
await bridgeInstance.adminCancelProposal(args.chainID, args.depositNonce);
console.log(`[BRIDGE] Successfully set proposal with chainID ${args.chainID}
and depositNonce ${args.depositNonce} status to 'Cancelled`);

})

const bridgeCmd = new Command("bridge")

bridgeCmd.addCommand(registerResourceCmd)
bridgeCmd.addCommand(registerGenericResourceCmd)
bridgeCmd.addCommand(setBurnCmd)
bridgeCmd.addCommand(cancelProposalCmd)

module.exports = bridgeCmd
20 changes: 13 additions & 7 deletions cli/cmd/deploy.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const deployCmd = new Command("deploy")
.option('--relayers <value>', 'List of initial relayers', splitCommaList, constants.relayerAddresses)
.option('--relayer-threshold <value>', 'Number of votes required for a proposal to pass', 2)
.option('--fee <ether>', 'Fee to be taken when making a deposit (decimals allowed)', 0)
.option('--expiry <blocks>', 'Numer of blocks after which a proposal is considered cancelled', 100)
.action(async (args, a) => {
await setupParentArgs(args, args.parent)
let startBal = await args.provider.getBalance(args.wallet.address)
Expand All @@ -28,15 +29,18 @@ const deployCmd = new Command("deploy")
const displayLog = (args) => {
console.log(`
================================================================
Url: ${args.url}
Deployer: ${args.wallet.address}
Url: ${args.url}
Deployer: ${args.wallet.address}
Gas Limit: ${ethers.utils.bigNumberify(args.gasLimit)}
Gas Price: ${ethers.utils.bigNumberify(args.gasPrice)}
Chain Id: ${args.chainId}
Threshold: ${args.relayerThreshold}
Relayers: ${args.relayers}
Bridge Fee: ${args.fee}
Deploy Cost: ${ethers.utils.formatEther(args.cost)}
Chain Id: ${args.chainId}
Threshold: ${args.relayerThreshold}
Relayers: ${args.relayers}
Fee: ${args.fee} ETH
Expiry: ${args.expiry}
Cost: ${ethers.utils.formatEther(args.cost)}
=======
Contract Addresses
================================================================
Expand Down Expand Up @@ -68,7 +72,9 @@ async function deployBridgeContract(args) {
args.relayers,
args.relayerThreshold,
ethers.utils.parseEther(args.fee.toString()),
args.expiry,
{ gasPrice: args.gasPrice, gasLimit: args.gasLimit}

);
await contract.deployed();
args.bridgeContract = contract.address
Expand Down
65 changes: 51 additions & 14 deletions contracts/Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,18 @@ contract Bridge is Pausable, AccessControl {
uint256 public _totalRelayers;
uint256 public _totalProposals;
uint256 public _fee;
uint256 public _expiry;

enum Vote {No, Yes}
enum ProposalStatus {Inactive, Active, Passed, Transferred}
enum ProposalStatus {Inactive, Active, Passed, Transferred, Cancelled}

struct Proposal {
bytes32 _resourceID;
bytes32 _dataHash;
address[] _yesVotes;
address[] _noVotes;
ProposalStatus _status;
uint256 _proposedBlock;
}

// destinationChainID => number of deposits
Expand Down Expand Up @@ -60,6 +62,13 @@ contract Bridge is Pausable, AccessControl {
bytes32 resourceID,
bytes32 dataHash
);
event ProposalCancelled(
uint8 indexed originChainID,
uint8 indexed destinationChainID,
uint64 indexed depositNonce,
bytes32 resourceID,
bytes32 dataHash
);
event ProposalVote(
uint8 indexed originChainID,
uint8 indexed destinationChainID,
Expand All @@ -86,6 +95,12 @@ contract Bridge is Pausable, AccessControl {
_;
}

modifier onlyAdminOrRelayer() {
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender) || hasRole(RELAYER_ROLE, msg.sender),
"sender does not have admin role");
_;
}

modifier onlyRelayers() {
require(hasRole(RELAYER_ROLE, msg.sender), "sender does not have relayer role");
_;
Expand All @@ -98,10 +113,11 @@ contract Bridge is Pausable, AccessControl {
@param initialRelayers Addresses that should be initially granted the relayer role.
@param initialRelayerThreshold Number of votes needed for a deposit proposal to be considered passed.
*/
constructor (uint8 chainID, address[] memory initialRelayers, uint initialRelayerThreshold, uint256 fee) public {
constructor (uint8 chainID, address[] memory initialRelayers, uint initialRelayerThreshold, uint256 fee, uint256 expiry) public {
_chainID = chainID;
_relayerThreshold = initialRelayerThreshold;
_fee = fee;
_expiry = expiry;

_setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
_setRoleAdmin(RELAYER_ROLE, DEFAULT_ADMIN_ROLE);
Expand Down Expand Up @@ -320,7 +336,7 @@ contract Bridge is Pausable, AccessControl {
Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];

require(_resourceIDToHandlerAddress[resourceID] != address(0), "no handler for resourceID");
require(uint(proposal._status) <= 1, "proposal has already been passed or transferred");
require(uint(proposal._status) <= 1, "proposal has already been passed, transferred, or cancelled");
require(!_hasVotedOnProposal[chainID][depositNonce][msg.sender], "relayer has already voted on proposal");

if (uint(proposal._status) == 0) {
Expand All @@ -330,25 +346,46 @@ contract Bridge is Pausable, AccessControl {
_dataHash: dataHash,
_yesVotes: new address[](1),
_noVotes: new address[](0),
_status: ProposalStatus.Active
_status: ProposalStatus.Active,
_proposedBlock: block.number
});

proposal._yesVotes[0] = msg.sender;
emit ProposalCreated(chainID, _chainID, depositNonce, resourceID, dataHash);
} else {
require(dataHash == proposal._dataHash, "datahash mismatch");
proposal._yesVotes.push(msg.sender);
}
if ((block.number).sub(proposal._proposedBlock) > _expiry) {
// if the number of blocks that has passed since this proposal was
// submitted exceeds the expiry threshold set, cancel the proposal
proposal._status = ProposalStatus.Cancelled;
emit ProposalCancelled(chainID, _chainID, depositNonce, resourceID, dataHash);
} else {
require(dataHash == proposal._dataHash, "datahash mismatch");
proposal._yesVotes.push(msg.sender);

_hasVotedOnProposal[chainID][depositNonce][msg.sender] = true;
emit ProposalVote(chainID, _chainID, depositNonce, resourceID, proposal._status);
}

// If _depositThreshold is set to 1, then auto finalize
// or if _relayerThreshold has been exceeded
if (_relayerThreshold <= 1 || proposal._yesVotes.length >= _relayerThreshold) {
proposal._status = ProposalStatus.Passed;
emit ProposalFinalized(chainID, _chainID, depositNonce, resourceID);
}
if (proposal._status != ProposalStatus.Cancelled) {
_hasVotedOnProposal[chainID][depositNonce][msg.sender] = true;
emit ProposalVote(chainID, _chainID, depositNonce, resourceID, proposal._status);

// If _depositThreshold is set to 1, then auto finalize
// or if _relayerThreshold has been exceeded
if (_relayerThreshold <= 1 || proposal._yesVotes.length >= _relayerThreshold) {
proposal._status = ProposalStatus.Passed;
emit ProposalFinalized(chainID, _chainID, depositNonce, resourceID);
}
}

}

function cancelProposal(uint8 chainID, uint64 depositNonce) public onlyAdminOrRelayer {
Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];
require((block.number).sub(proposal._proposedBlock) > _expiry, "Proposal does not meet expiry threshold");

proposal._status = ProposalStatus.Cancelled;
emit ProposalCancelled(chainID, _chainID, depositNonce, proposal._resourceID, proposal._dataHash);

}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/contractBridge/admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ contract('Bridge - [admin]', async accounts => {
let BridgeInstance;

beforeEach(async () => {
BridgeInstance = await BridgeContract.new(chainID, initialRelayers, initialRelayerThreshold, 0);
BridgeInstance = await BridgeContract.new(chainID, initialRelayers, initialRelayerThreshold, 0, 100);
ADMIN_ROLE = await BridgeInstance.DEFAULT_ADMIN_ROLE()
});

Expand Down
167 changes: 167 additions & 0 deletions test/contractBridge/cancelDepositProposal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/**
* Copyright 2020 ChainSafe Systems
* SPDX-License-Identifier: LGPL-3.0-only
*/

const TruffleAssert = require('truffle-assertions');
const Ethers = require('ethers');

const Helpers = require('../helpers');

const BridgeContract = artifacts.require("Bridge");
const ERC20MintableContract = artifacts.require("ERC20PresetMinterPauser");
const ERC20HandlerContract = artifacts.require("ERC20Handler");

contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts) => {
const originChainID = 1;
const destinationChainID = 2;
const relayer1Address = accounts[0];
const relayer2Address = accounts[1];
const relayer3Address = accounts[2];
const relayer4Address = accounts[3]
const depositerAddress = accounts[4];
const destinationChainRecipientAddress = accounts[4];
const depositAmount = 10;
const expectedDepositNonce = 1;
const relayerThreshold = 3;

let BridgeInstance;
let DestinationERC20MintableInstance;
let DestinationERC20HandlerInstance;
let depositData = '';
let depositDataHash = '';
let resourceID = '';
let initialResourceIDs;
let initialContractAddresses;
let burnableContractAddresses;

let vote, executeProposal;

beforeEach(async () => {
await Promise.all([
BridgeContract.new(destinationChainID, [
relayer1Address,
relayer2Address,
relayer3Address,
relayer4Address],
relayerThreshold,
0,
10,).then(instance => BridgeInstance = instance),
ERC20MintableContract.new("token", "TOK").then(instance => DestinationERC20MintableInstance = instance)
]);

resourceID = Helpers.createResourceID(DestinationERC20MintableInstance.address, originChainID);
initialResourceIDs = [resourceID];
initialContractAddresses = [DestinationERC20MintableInstance.address];
burnableContractAddresses = [DestinationERC20MintableInstance.address];

DestinationERC20HandlerInstance = await ERC20HandlerContract.new(BridgeInstance.address, initialResourceIDs, initialContractAddresses, burnableContractAddresses);

depositData = Helpers.createERCDepositData(resourceID, depositAmount, 32, destinationChainRecipientAddress);
depositDataHash = Ethers.utils.keccak256(DestinationERC20HandlerInstance.address + depositData.substr(2));

await Promise.all([
DestinationERC20MintableInstance.grantRole(await DestinationERC20MintableInstance.MINTER_ROLE(), DestinationERC20HandlerInstance.address),
BridgeInstance.adminSetHandlerAddress(DestinationERC20HandlerInstance.address, resourceID)
]);

vote = (relayer) => BridgeInstance.voteProposal(originChainID, expectedDepositNonce, resourceID, depositDataHash, {from: relayer});
executeProposal = (relayer) => BridgeInstance.executeProposal(originChainID, expectedDepositNonce, depositData, {from: relayer});
});

it ('[sanity] bridge configured with threshold, relayers, and expiry', async () => {
assert.equal(await BridgeInstance._chainID(), destinationChainID)

assert.equal(await BridgeInstance._relayerThreshold(), relayerThreshold)

assert.equal((await BridgeInstance._totalRelayers()).toString(), '4')

assert.equal(await BridgeInstance._expiry(), 10)
})

it('[sanity] depositProposal should be created with expected values', async () => {
await TruffleAssert.passes(vote(relayer1Address));

const expectedDepositProposal = {
_dataHash: depositDataHash,
_yesVotes: [relayer1Address],
_noVotes: [],
_status: '1' // Active
};

const depositProposal = await BridgeInstance.getProposal(
originChainID, expectedDepositNonce);

assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
});


it("voting on depositProposal after threshold results in cancelled proposal", async () => {


await TruffleAssert.passes(vote(relayer1Address));

for (i=0; i<10; i++) {
await Helpers.advanceBlock();
}

await TruffleAssert.passes(vote(relayer2Address));

const expectedDepositProposal = {
_dataHash: depositDataHash,
_yesVotes: [relayer1Address],
_noVotes: [],
_status: '4' // Cancelled
};

const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
await TruffleAssert.reverts(vote(relayer3Address), "proposal has already been passed, transferred, or cancelled.")

});


it("relayer can cancel proposal after threshold blocks have passed", async () => {
await TruffleAssert.passes(vote(relayer2Address));

for (i=0; i<10; i++) {
await Helpers.advanceBlock();
}

const expectedDepositProposal = {
_dataHash: depositDataHash,
_yesVotes: [relayer2Address],
_noVotes: [],
_status: '4' // Cancelled
};

await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce))
const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
await TruffleAssert.reverts(vote(relayer4Address), "proposal has already been passed, transferred, or cancelled.")

});

it("admin can cancel proposal after threshold blocks have passed", async () => {
await TruffleAssert.passes(vote(relayer3Address));

for (i=0; i<10; i++) {
await Helpers.advanceBlock();
}

const expectedDepositProposal = {
_dataHash: depositDataHash,
_yesVotes: [relayer3Address],
_noVotes: [],
_status: '4' // Cancelled
};

await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce))
const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
await TruffleAssert.reverts(vote(relayer2Address), "proposal has already been passed, transferred, or cancelled.")

});


});
Loading

0 comments on commit 5fbc2be

Please sign in to comment.