Improvements for channel protocols (paritytech#631)

ethereum/.envrc-example

@@ -5,10 +5,12 @@ export ETHERSCAN_API_KEY=
 
 # Basic Channel
 export BASIC_CHANNEL_PRINCIPAL=0x0000000000000000000000000000000000000042
+export BASIC_CHANNEL_SOURCE_ID=0
 
 # Incentivized Channel
 # Default fee is 1 SnowDOT (12 decimal places)
 export INCENTIVIZED_CHANNEL_FEE=1000000000000000000
+export INCENTIVIZED_CHANNEL_SOURCE_ID=1
 
-# ParachainClient needs to be constructed with our parachain ID
+# ParachainClient
 export PARACHAIN_ID=1000

ethereum/README.md

@@ -26,10 +26,50 @@ direnv allow
 
 ## Testing
 
-Run tests on the hardhat network:
+Run tests:
 
 ```bash
-yarn test
+npx hardhat test
+```
+
+### Updating fixture data for unit tests
+
+We use logging artifacts from the relayer in E2E stack as a source of compliant fixture data.
+
+BEEFY commitment & proofs extracted from `../test/beefy-relay.log`.
+* test/fixtures/beefy-relay-basic.json
+* test/fixtures/beefy-relay-incentivized.json
+
+Message bundles & proofs extracted from `../test/parachain-relay.log`.
+* test/fixtures/parachain-relay-basic.json
+* test/fixtures/parachain-relay-incentivized.json
+
+First, change to the E2E test directory at `../test`.
+
+1. Run the following tests to initiate bridge activity and produce logging data
+```bash
+yarn test --grep 'should transfer DOT from Substrate to Ethereum \(basic channel\)'
+yarn test --grep 'should transfer ETH from Ethereum to Substrate \(incentivized channel\)'
+yarn test --grep 'should transfer DOT from Substrate to Ethereum \(incentivized channel\)'
+```
+
+Steps for updating the fixture data for the basic channel:
+1. Grep for `Sent transaction BasicInboundChannel.submit` in `parachain-relay.json`
+2. Copy that log line into `./test/fixtures/parachain-relay-basic.json`
+3. In that file, take the `beefyBlock` field (a block hash) and use polkadot.js explorer to find the corresponding block number.
+4. Run the following (substituting `$BLOCKNUMBER`) and paste the output into `./test/fixtures/beefy-relay-basic.json`:
+```bash
+jq -s --argjson blocknumber $BLOCKNUMBER '.[] | select( .message | contains("Sent SubmitFinal transaction")) | select( .params.commitment.blockNumber | contains($blocknumber))' beefy-relay.log
+```
+5: NOTE: if the produced `./test/fixtures/beefy-relay-basic.json` doesn't contain a `params.leaf` field, repeat all the steps again.
+
+Steps for updating the fixture data for the basic channel:
+1. Grep for `Sent transaction IncentivizedInboundChannel.submit` in `parachain-relay.json`
+2. Copy that log line into `./test/fixtures/parachain-relay-incentivized.json`
+3. In that file, take the `beefyBlock` field (a hash) and use polkadot.js explorer to find the corresponding block number.
+4. Run the following (substituting `$BLOCKNUMBER`) and paste the output into `./test/fixtures/beefy-relay-incentivized.json`:
+```bash
+jq -s --argjson blocknumber $BLOCKNUMBER '.[] | select( .message | contains("Sent SubmitFinal transaction")) | select( .params.commitment.blockNumber | contains($blocknumber))' beefy-relay.log
 ```
 
 ## Deployment

ethereum/contracts/BasicInboundChannel.sol

@@ -7,11 +7,14 @@ contract BasicInboundChannel {
     uint256 public constant MAX_GAS_PER_MESSAGE = 100000;
     uint256 public constant GAS_BUFFER = 60000;
 
+    uint8 public immutable sourceChannelID;
+
     uint64 public nonce;
 
     ParachainClient public parachainClient;
 
     struct MessageBundle {
+        uint8 sourceChannelID;
         uint64 nonce;
         Message[] messages;
     }
@@ -24,39 +27,33 @@ contract BasicInboundChannel {
 
     event MessageDispatched(uint64 id, bool result);
 
-    constructor(ParachainClient client) {
+    constructor(uint8 _sourceChannelID, ParachainClient _parachainClient) {
         nonce = 0;
-        parachainClient = client;
+        sourceChannelID = _sourceChannelID;
+        parachainClient = _parachainClient;
     }
 
-    function submit(MessageBundle calldata bundle, ParachainClient.Proof calldata proof) external {
+    function submit(MessageBundle calldata bundle,  bytes calldata proof) external {
         bytes32 commitment = keccak256(abi.encode(bundle));
 
         require(parachainClient.verifyCommitment(commitment, proof), "Invalid proof");
-
-        // Require there is enough gas to play all messages
+        require(bundle.sourceChannelID == sourceChannelID, "Invalid source channel");
+        require(bundle.nonce == nonce + 1, "Invalid nonce");
         require(
             gasleft() >= (bundle.messages.length * MAX_GAS_PER_MESSAGE) + GAS_BUFFER,
             "insufficient gas for delivery of all messages"
         );
-
-        processMessages(bundle);
+        nonce++;
+        dispatch(bundle);
     }
 
-    function processMessages(MessageBundle calldata bundle) internal {
-        require(bundle.nonce == nonce + 1, "invalid nonce");
-
+    function dispatch(MessageBundle calldata bundle) internal {
         for (uint256 i = 0; i < bundle.messages.length; i++) {
             Message calldata message = bundle.messages[i];
-
-            // Deliver the message to the target
             (bool success, ) = message.target.call{ value: 0, gas: MAX_GAS_PER_MESSAGE }(
                 message.payload
             );
-
             emit MessageDispatched(message.id, success);
         }
-
-        nonce++;
     }
 }

ethereum/contracts/DOTApp.sol

@@ -5,11 +5,11 @@ import "@openzeppelin/contracts/access/AccessControl.sol";
 import "./WrappedToken.sol";
 import "./ScaleCodec.sol";
 import "./OutboundChannel.sol";
-import "./FeeSource.sol";
+import "./FeeController.sol";
 
 enum ChannelId {Basic, Incentivized}
 
-contract DOTApp is FeeSource, AccessControl {
+contract DOTApp is FeeController, AccessControl {
     using ScaleCodec for uint256;
 
     mapping(ChannelId => Channel) public channels;
@@ -90,7 +90,7 @@ contract DOTApp is FeeSource, AccessControl {
     }
 
     // Incentivized channel calls this to charge (burn) fees
-    function burnFee(address feePayer, uint256 _amount) external override onlyRole(FEE_BURNER_ROLE) {
+    function handleFee(address feePayer, uint256 _amount) external override onlyRole(FEE_BURNER_ROLE) {
         token.burn(feePayer, _amount, "");
     }
 

ethereum/contracts/ETHApp.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.9;
 
 import "@openzeppelin/contracts/access/AccessControl.sol";
 import "@openzeppelin/contracts/utils/math/SafeCast.sol";
-import "./RewardSource.sol";
+import "./RewardController.sol";
 import "./ScaleCodec.sol";
 import "./OutboundChannel.sol";
 
@@ -12,7 +12,7 @@ enum ChannelId {
     Incentivized
 }
 
-contract ETHApp is RewardSource, AccessControl {
+contract ETHApp is RewardController, AccessControl {
     using ScaleCodec for uint128;
     using ScaleCodec for uint32;
     using SafeCast for uint256;
@@ -151,13 +151,16 @@ contract ETHApp is RewardSource, AccessControl {
             );
     }
 
-    function reward(address payable _recipient, uint128 _amount)
+    // NOTE: should never revert or the bridge will be broken
+    function handleReward(address payable _relayer, uint128 _amount)
         external
         override
         onlyRole(REWARD_ROLE)
     {
-        (bool success, ) = _recipient.call{value: _amount}("");
-        require(success, "Unable to send Ether");
+        (bool success,) = _relayer.call{value: _amount}("");
+        if (success) {
+            emit Rewarded(_relayer, _amount);
+        }
     }
 
     function upgrade(

ethereum/contracts/FeeController.sol

@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.9;
+
+// Something that can burn a fee from a feepayer account.
+interface FeeController {
+    // NOTE: Should never revert or the bridge will be broken
+    function handleFee(address feePayer, uint256 _amount) external;
+}

ethereum/contracts/IncentivizedInboundChannel.sol

@@ -3,20 +3,22 @@ pragma solidity ^0.8.9;
 
 import "@openzeppelin/contracts/access/AccessControl.sol";
 import "./ParachainClient.sol";
-import "./RewardSource.sol";
+import "./RewardController.sol";
 
 contract IncentivizedInboundChannel is AccessControl {
+    uint8 public immutable sourceChannelID;
     uint64 public nonce;
 
     struct MessageBundle {
+        uint8 sourceChannelID;
         uint64 nonce;
+        uint128 fee;
         Message[] messages;
     }
 
     struct Message {
         uint64 id;
         address target;
-        uint128 fee;
         bytes payload;
     }
 
@@ -28,64 +30,52 @@ contract IncentivizedInboundChannel is AccessControl {
     // Governance contracts will administer using this role.
     bytes32 public constant CONFIG_UPDATE_ROLE = keccak256("CONFIG_UPDATE_ROLE");
 
-    RewardSource private rewardSource;
+    RewardController private rewardController;
 
     ParachainClient public parachainClient;
 
-    constructor(ParachainClient client) {
+    constructor(uint8 _sourceChannelID, ParachainClient _parachainClient) {
         _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-        parachainClient = client;
         nonce = 0;
+        sourceChannelID = _sourceChannelID;
+        parachainClient = _parachainClient;
     }
 
     // Once-off post-construction call to set initial configuration.
-    function initialize(address _configUpdater, address _rewardSource)
+    function initialize(address _configUpdater, address _rewardController)
         external
         onlyRole(DEFAULT_ADMIN_ROLE)
     {
         // Set initial configuration
         grantRole(CONFIG_UPDATE_ROLE, _configUpdater);
-        rewardSource = RewardSource(_rewardSource);
+        rewardController = RewardController(_rewardController);
 
         // drop admin privileges
         renounceRole(DEFAULT_ADMIN_ROLE, msg.sender);
     }
 
-    function submit(MessageBundle calldata bundle, ParachainClient.Proof calldata proof) external {
-        // Proof
-        // 1. Compute our parachain's message `commitment` by ABI encoding and hashing the `_messages`
+    function submit(MessageBundle calldata bundle, bytes calldata proof) external {
         bytes32 commitment = keccak256(abi.encode(bundle));
 
         require(parachainClient.verifyCommitment(commitment, proof), "Invalid proof");
-
-        // Require there is enough gas to play all messages
+        require(bundle.sourceChannelID == sourceChannelID, "Invalid source channel");
+        require(bundle.nonce == nonce + 1, "Invalid nonce");
         require(
             gasleft() >= (bundle.messages.length * MAX_GAS_PER_MESSAGE) + GAS_BUFFER,
             "insufficient gas for delivery of all messages"
         );
-
-        processMessages(payable(msg.sender), bundle);
+        nonce++;
+        dispatch(bundle);
+        rewardController.handleReward(payable(msg.sender), bundle.fee);
     }
 
-    function processMessages(address payable _relayer, MessageBundle calldata bundle) internal {
-        require(bundle.nonce == nonce + 1, "invalid nonce");
-
-        uint128 _rewardAmount = 0;
+    function dispatch(MessageBundle calldata bundle) internal {
         for (uint256 i = 0; i < bundle.messages.length; i++) {
             Message calldata message = bundle.messages[i];
-
-            // Deliver the message to the target
-            // Delivery will have fixed maximum gas allowed for the target app
             (bool success, ) = message.target.call{ value: 0, gas: MAX_GAS_PER_MESSAGE }(
                 message.payload
             );
-
-            _rewardAmount = _rewardAmount + message.fee;
             emit MessageDispatched(message.id, success);
         }
-
-        // reward the relayer
-        rewardSource.reward(_relayer, _rewardAmount);
-        nonce++;
     }
 }

ethereum/contracts/IncentivizedOutboundChannel.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.9;
 import "@openzeppelin/contracts/access/AccessControl.sol";
 import "./OutboundChannel.sol";
 import "./ChannelAccess.sol";
-import "./FeeSource.sol";
+import "./FeeController.sol";
 
 // IncentivizedOutboundChannel is a channel that sends ordered messages with an increasing nonce. It will have
 // incentivization too.
@@ -17,7 +17,7 @@ contract IncentivizedOutboundChannel is OutboundChannel, ChannelAccess, AccessCo
     uint64 public nonce;
 
     uint256 public fee;
-    FeeSource public feeSource;
+    FeeController public feeController;
 
     event Message(
         address source,
@@ -38,12 +38,12 @@ contract IncentivizedOutboundChannel is OutboundChannel, ChannelAccess, AccessCo
     // Once-off post-construction call to set initial configuration.
     function initialize(
         address _configUpdater,
-        address _feeSource,
+        address _feeController,
         address[] memory defaultOperators
     )
     external onlyRole(DEFAULT_ADMIN_ROLE) {
         // Set initial configuration
-        feeSource = FeeSource(_feeSource);
+        feeController = FeeController(_feeController);
         grantRole(CONFIG_UPDATE_ROLE, _configUpdater);
         for (uint i = 0; i < defaultOperators.length; i++) {
             _authorizeDefaultOperator(defaultOperators[i]);
@@ -74,7 +74,7 @@ contract IncentivizedOutboundChannel is OutboundChannel, ChannelAccess, AccessCo
      */
     function submit(address feePayer, bytes calldata payload) external override {
         require(isOperatorFor(msg.sender, feePayer), "Caller is not an operator for fee payer");
-        feeSource.burnFee(feePayer, fee);
+        feeController.handleFee(feePayer, fee);
         nonce = nonce + 1;
         emit Message(msg.sender, nonce, fee, payload);
     }

ethereum/contracts/MaliciousDOTApp.sol

@@ -5,7 +5,7 @@ import "@openzeppelin/contracts/access/AccessControl.sol";
 import "./WrappedToken.sol";
 import "./ScaleCodec.sol";
 import "./OutboundChannel.sol";
-import "./FeeSource.sol";
+import "./FeeController.sol";
 
 enum ChannelId {
     Basic,
@@ -15,7 +15,7 @@ enum ChannelId {
 // MaliciousDOTApp is similar to DOTApp, but contains an infinite loop in the mint function, which will consume all the
 // gas of the message. MaliciousDOTApp is used in a test which verifies that a message running out of gas will not
 // prevent execution of other messages
-contract MaliciousDOTApp is FeeSource, AccessControl {
+contract MaliciousDOTApp is FeeController, AccessControl {
     using ScaleCodec for uint256;
 
     mapping(ChannelId => Channel) public channels;
@@ -85,7 +85,7 @@ contract MaliciousDOTApp is FeeSource, AccessControl {
     }
 
     // Incentivized channel calls this to charge (burn) fees
-    function burnFee(address feePayer, uint256 _amount) external override onlyRole(FEE_BURNER_ROLE) {
+    function handleFee(address feePayer, uint256 _amount) external override onlyRole(FEE_BURNER_ROLE) {
         token.burn(feePayer, _amount, "");
     }