Update proposal mapping (#184)

- Updates proposal mappings to use hash of proposal data as a key - Combines nonce and source chain ID into a single key
ChainSafe · Jun 19, 2020 · 6a07eef · 6a07eef
1 parent 52a6765
commit 6a07eef
Show file tree

Hide file tree

Showing 15 changed files with 95 additions and 59 deletions.
diff --git a/contracts/Bridge.sol b/contracts/Bridge.sol
@@ -40,12 +40,12 @@ contract Bridge is Pausable, AccessControl {
     mapping(uint8 => uint64) public _depositCounts;
     // resourceID => handler address
     mapping(bytes32 => address) public _resourceIDToHandlerAddress;
-    // destinationChainID => depositNonce => bytes
-    mapping(uint8 => mapping(uint64 => bytes)) public _depositRecords;
-    // destinationChainID => depositNonce => Proposal
-    mapping(uint8 => mapping(uint64 => Proposal)) public _proposals;
-    // destinationChainID => depositNonce => relayerAddress => bool
-    mapping(uint8 => mapping(uint64 => mapping(address => bool))) public _hasVotedOnProposal;
+    // depositNonce => destinationChainID => bytes
+    mapping(uint64 => mapping(uint8 => bytes)) public _depositRecords;
+    // destinationChainID + depositNonce => dataHash => Proposal
+    mapping(uint72 => mapping(bytes32 => Proposal)) public _proposals;
+    // destinationChainID + depositNonce => dataHash => relayerAddress => bool
+    mapping(uint72 => mapping(bytes32 => mapping(address => bool))) public _hasVotedOnProposal;
 
     event RelayerThresholdChanged(uint indexed newThreshold);
     event RelayerAdded(address indexed relayer);
@@ -261,14 +261,16 @@ contract Bridge is Pausable, AccessControl {
         @notice Returns a proposal.
         @param originChainID Chain ID deposit originated from.
         @param depositNonce ID of proposal generated by proposal's origin Bridge contract.
+        @param dataHash Hash of data to be provided when deposit proposal is executed.
         @return Proposal which consists of:
         - _dataHash Hash of data to be provided when deposit proposal is executed.
         - _yesVotes Number of votes in favor of proposal.
         - _noVotes Number of votes against proposal.
         - _status Current status of proposal.
      */
-    function getProposal(uint8 originChainID, uint64 depositNonce) external view returns (Proposal memory) {
-        return _proposals[originChainID][depositNonce];
+    function getProposal(uint8 originChainID, uint64 depositNonce, bytes32 dataHash) external view returns (Proposal memory) {
+        uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(originChainID);
+        return _proposals[nonceAndID][dataHash];
     }
 
     /**
@@ -313,7 +315,7 @@ contract Bridge is Pausable, AccessControl {
         require(handler != address(0), "resourceID not mapped to handler address");
 
         uint64 depositNonce = ++_depositCounts[destinationChainID];
-        _depositRecords[destinationChainID][depositNonce] = data;
+        _depositRecords[depositNonce][destinationChainID] = data;
 
         IDepositExecute depositHandler = IDepositExecute(handler);
         depositHandler.deposit(resourceID, destinationChainID, depositNonce, msg.sender, data);
@@ -335,15 +337,17 @@ contract Bridge is Pausable, AccessControl {
         {_relayerThreshold}.
      */
     function voteProposal(uint8 chainID, uint64 depositNonce, bytes32 resourceID, bytes32 dataHash) external onlyRelayers whenNotPaused {
-        Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];
+
+        uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(chainID);
+        Proposal storage proposal = _proposals[nonceAndID][dataHash];
 
         require(_resourceIDToHandlerAddress[resourceID] != address(0), "no handler for resourceID");
         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");
+        require(!_hasVotedOnProposal[nonceAndID][dataHash][msg.sender], "relayer has already voted on proposal");
 
         if (uint(proposal._status) == 0) {
             ++_totalProposals;
-            _proposals[chainID][depositNonce] = Proposal({
+            _proposals[nonceAndID][dataHash] = Proposal({
                 _resourceID: resourceID,
                 _dataHash: dataHash,
                 _yesVotes: new address[](1),
@@ -364,11 +368,12 @@ contract Bridge is Pausable, AccessControl {
                 require(dataHash == proposal._dataHash, "datahash mismatch");
                 proposal._yesVotes.push(msg.sender);
 
+
             }
 
         }
         if (proposal._status != ProposalStatus.Cancelled) {
-            _hasVotedOnProposal[chainID][depositNonce][msg.sender] = true;
+            _hasVotedOnProposal[nonceAndID][dataHash][msg.sender] = true;
             emit ProposalVote(chainID, _chainID, depositNonce, resourceID, proposal._status);
 
             // If _depositThreshold is set to 1, then auto finalize
@@ -381,8 +386,19 @@ contract Bridge is Pausable, AccessControl {
 
     }
 
-    function cancelProposal(uint8 chainID, uint64 depositNonce) public onlyAdminOrRelayer {
-        Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];
+    /**
+        @notice Executes a deposit proposal that is considered passed using a specified handler contract.
+        @notice Only callable by relayers when Bridge is not paused.
+        @param chainID ID of chain deposit originated from.
+        @param depositNonce ID of deposited generated by origin Bridge contract.
+        @param dataHash Hash of data originally provided when deposit was made.
+        @notice Proposal must be past expiry threshold.
+        @notice Emits {ProposalCancelled} event.
+     */
+    function cancelProposal(uint8 chainID, uint64 depositNonce, bytes32 dataHash) public onlyAdminOrRelayer {
+        uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(chainID);
+        Proposal storage proposal = _proposals[nonceAndID][dataHash];
+
         require(proposal._status != ProposalStatus.Cancelled, "Proposal already cancelled");
         require((block.number).sub(proposal._proposedBlock) > _expiry, "Proposal does not meet expiry threshold");
 
@@ -395,20 +411,22 @@ contract Bridge is Pausable, AccessControl {
         @notice Executes a deposit proposal that is considered passed using a specified handler contract.
         @notice Only callable by relayers when Bridge is not paused.
         @param chainID ID of chain deposit originated from.
+        @param resourceID ResourceID to be used when making deposits.
         @param depositNonce ID of deposited generated by origin Bridge contract.
         @param data Data originally provided when deposit was made.
         @notice Proposal must have Passed status.
         @notice Hash of {data} must equal proposal's {dataHash}.
         @notice Emits {ProposalExecuted} event.
      */
-    function executeProposal(uint8 chainID, uint64 depositNonce, bytes calldata data) external onlyRelayers whenNotPaused {
-        Proposal storage proposal = _proposals[uint8(chainID)][depositNonce];
-        address handler = _resourceIDToHandlerAddress[proposal._resourceID];
+    function executeProposal(uint8 chainID, uint64 depositNonce, bytes calldata data, bytes32 resourceID) external onlyRelayers whenNotPaused {
+        address handler = _resourceIDToHandlerAddress[resourceID];
+        uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(chainID);
+        bytes32 dataHash = keccak256(abi.encodePacked(handler, data));
+        Proposal storage proposal = _proposals[nonceAndID][dataHash];
 
         require(proposal._status != ProposalStatus.Inactive, "proposal is not active");
         require(proposal._status == ProposalStatus.Passed, "proposal was not passed or has already been transferred");
-        require(keccak256(abi.encodePacked(handler, data)) == proposal._dataHash,
-            "provided data does not match proposal's data hash");
+        require(dataHash == proposal._dataHash, "provided data does not match proposal's data hash");
 
         proposal._status = ProposalStatus.Transferred;
 
@@ -430,4 +448,4 @@ contract Bridge is Pausable, AccessControl {
         }
     }
 
-}
+}
diff --git a/test/contractBridge/cancelDepositProposal.js b/test/contractBridge/cancelDepositProposal.js
@@ -90,7 +90,7 @@ contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts)
         };
 
         const depositProposal = await BridgeInstance.getProposal(
-            originChainID, expectedDepositNonce);
+            originChainID, expectedDepositNonce, depositDataHash);
 
         assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
     });
@@ -114,7 +114,7 @@ contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts)
             _status: '4' // Cancelled
         };
 
-        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
+        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce, depositDataHash);
         assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
         await TruffleAssert.reverts(vote(relayer3Address), "proposal has already been passed, transferred, or cancelled.")
 
@@ -135,8 +135,8 @@ contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts)
             _status: '4' // Cancelled
         };
 
-        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce))
-        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
+        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce, depositDataHash))
+        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce, depositDataHash);
         assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
         await TruffleAssert.reverts(vote(relayer4Address), "proposal has already been passed, transferred, or cancelled.")
 
@@ -156,8 +156,8 @@ contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts)
             _status: '4' // Cancelled
         };
 
-        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce))
-        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce);
+        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce, depositDataHash))
+        const depositProposal = await BridgeInstance.getProposal(originChainID, expectedDepositNonce, depositDataHash);
         assert.deepInclude(Object.assign({}, depositProposal), expectedDepositProposal);
         await TruffleAssert.reverts(vote(relayer2Address), "proposal has already been passed, transferred, or cancelled.")
 
@@ -177,8 +177,8 @@ contract('Bridge - [voteProposal with relayerThreshold == 3]', async (accounts)
             _status: '4' // Cancelled
         };
 
-        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce))
-        await TruffleAssert.reverts(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce), "Proposal already cancelled")
+        await TruffleAssert.passes(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce, depositDataHash))
+        await TruffleAssert.reverts(BridgeInstance.cancelProposal(originChainID, expectedDepositNonce, depositDataHash), "Proposal already cancelled")
 
     });
 

diff --git a/test/contractBridge/createDepositProposal.js b/test/contractBridge/createDepositProposal.js
@@ -94,7 +94,7 @@ contract('Bridge - [create a deposit proposal (voteProposal) with relayerThresho
 
     it("getProposal should be called successfully", async () => {
         await TruffleAssert.passes(BridgeInstance.getProposal(
-            destinationChainID, expectedDepositNonce
+            destinationChainID, expectedDepositNonce, dataHash
         ));
     });
 
@@ -116,7 +116,7 @@ contract('Bridge - [create a deposit proposal (voteProposal) with relayerThresho
         );
 
         const depositProposal = await BridgeInstance.getProposal(
-            destinationChainID, expectedDepositNonce);
+            destinationChainID, expectedDepositNonce, dataHash);
         Helpers.assertObjectsMatch(expectedDepositProposal, Object.assign({}, depositProposal));
     });
 
@@ -129,7 +129,7 @@ contract('Bridge - [create a deposit proposal (voteProposal) with relayerThresho
             { from: originChainRelayerAddress }
         );
         const hasVoted = await BridgeInstance._hasVotedOnProposal.call(
-            destinationChainID, expectedDepositNonce, originChainRelayerAddress);
+            Helpers.nonceAndId(expectedDepositNonce, destinationChainID), dataHash, originChainRelayerAddress);
         assert.isTrue(hasVoted);
     });
 
@@ -252,7 +252,7 @@ contract('Bridge - [create a deposit proposal (voteProposal) with relayerThresho
         );
 
         const depositProposal = await BridgeInstance.getProposal(
-            destinationChainID, expectedDepositNonce);
+            destinationChainID, expectedDepositNonce, dataHash);
         Helpers.assertObjectsMatch(expectedDepositProposal, Object.assign({}, depositProposal));
     });
 
@@ -265,7 +265,7 @@ contract('Bridge - [create a deposit proposal (voteProposal) with relayerThresho
             { from: originChainRelayerAddress }
         );
         const hasVoted = await BridgeInstance._hasVotedOnProposal.call(
-            destinationChainID, expectedDepositNonce, originChainRelayerAddress);
+            Helpers.nonceAndId(expectedDepositNonce, destinationChainID), dataHash, originChainRelayerAddress);
         assert.isTrue(hasVoted);
     });
 

diff --git a/test/contractBridge/depositERC20.js b/test/contractBridge/depositERC20.js
@@ -109,7 +109,7 @@ contract('Bridge - [deposit - ERC20]', async (accounts) => {
             { from: depositerAddress }
         );
 
-        const depositRecord = await BridgeInstance._depositRecords.call(destinationChainID, expectedDepositNonce);
+        const depositRecord = await BridgeInstance._depositRecords.call(expectedDepositNonce, destinationChainID);
         assert.strictEqual(depositRecord, depositData.toLowerCase(), "Stored depositRecord does not match original depositData");
     });
 

diff --git a/test/contractBridge/depositERC721.js b/test/contractBridge/depositERC721.js
@@ -129,7 +129,7 @@ contract('Bridge - [deposit - ERC721]', async (accounts) => {
             { from: depositerAddress }
         );
 
-        const depositRecord = await BridgeInstance._depositRecords.call(destinationChainID, expectedDepositNonce);
+        const depositRecord = await BridgeInstance._depositRecords.call(expectedDepositNonce, destinationChainID);
         assert.strictEqual(depositRecord, depositData.toLowerCase(), "Stored depositRecord does not match original depositData");
     });
 

diff --git a/test/contractBridge/depositGeneric.js b/test/contractBridge/depositGeneric.js
@@ -74,7 +74,7 @@ contract('Bridge - [deposit - Generic]', async () => {
             depositData
         );
 
-        const depositRecord = await BridgeInstance._depositRecords.call(destinationChainID, expectedDepositNonce);
+        const depositRecord = await BridgeInstance._depositRecords.call(expectedDepositNonce, destinationChainID);
         assert.strictEqual(depositRecord, depositData.toLowerCase(), "Stored depositRecord does not match original depositData");
     });
 

diff --git a/test/contractBridge/fee.js b/test/contractBridge/fee.js
@@ -101,17 +101,22 @@ contract('Bridge - [fee]', async (accounts) => {
         // check the balance is 0
         assert.equal(web3.utils.fromWei((await web3.eth.getBalance(BridgeInstance.address)), "ether"), "0");
         await BridgeInstance.deposit(destinationChainID, resourceID, depositData, {value: Ethers.utils.parseEther("1")})
+        assert.equal(web3.utils.fromWei((await web3.eth.getBalance(BridgeInstance.address)), "ether"), "1");
 
+        let b1Before = await web3.eth.getBalance(accounts[1]);
+        let b2Before = await web3.eth.getBalance(accounts[2]);
+
+        let payout = Ethers.utils.parseEther("0.5")
         // Transfer the funds
         TruffleAssert.passes(
             await BridgeInstance.transferFunds(
                 [accounts[1], accounts[2]], 
-                [Ethers.utils.parseEther("0.5"), Ethers.utils.parseEther("0.5")]
+                [payout, payout]
             )
         )
         b1 = await web3.eth.getBalance(accounts[1]);
         b2 = await web3.eth.getBalance(accounts[2]);
-        assert.equal(web3.utils.fromWei(b1), "100.5");
-        assert.equal(web3.utils.fromWei(b2), "100.5");
+        assert.equal(b1, Ethers.utils.bigNumberify(b1Before).add(payout));
+        assert.equal(b2, Ethers.utils.bigNumberify(b2Before).add(payout));
     })
 });