docs(protocol): enhance bridge documentation (#13106)

Co-authored-by: Daniel Wang <[email protected]>
Co-authored-by: David <[email protected]>

packages/protocol/contracts/L1/TaikoData.sol

@@ -75,7 +75,8 @@ library TaikoData {
 
     // This struct takes 9 slots.
     struct State {
-        // block id => block hash (some blocks' hashes won't be persisted)
+        // block id => block hash (some blocks' hashes won't be persisted,
+        // only the latest one if verified in a batch)
         mapping(uint256 => bytes32) l2Hashes;
         // block id => ProposedBlock
         mapping(uint256 => ProposedBlock) proposedBlocks;

packages/protocol/contracts/L1/libs/LibVerifying.sol

@@ -10,7 +10,10 @@ import "../../common/AddressResolver.sol";
 import "../TkoToken.sol";
 import "./LibUtils.sol";
 
-/// @author dantaik <[email protected]>
+/**
+ * LibVerifying.
+ * @author dantaik <[email protected]>
+ */
 library LibVerifying {
     using SafeCastUpgradeable for uint256;
     using LibUtils for TaikoData.State;
@@ -107,8 +110,10 @@ library LibVerifying {
             if (latestL2Height > state.latestVerifiedHeight) {
                 state.latestVerifiedHeight = latestL2Height;
 
-                // Note that not all L2 hashes are stored on L1, only the last
-                // verified one in a batch.
+                // Note: Not all L2 hashes are stored on L1, only the last
+                // verified one in a batch. This is sufficient because the last
+                // verified hash is the only one needed checking the existence
+                // of a cross-chain message with a merkle proof.
                 state.l2Hashes[
                     latestL2Height % config.blockHashHistory
                 ] = latestL2Hash;

packages/protocol/contracts/L2/TaikoL2.sol

@@ -16,16 +16,18 @@ import "../libs/LibInvalidTxList.sol";
 import "../libs/LibSharedConfig.sol";
 import "../libs/LibTxDecoder.sol";
 
-/// @author dantaik <[email protected]>
+/**
+ * @author dantaik <[email protected]>
+ */
 contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
     using LibTxDecoder for bytes;
 
     /**********************
      * State Variables    *
      **********************/
 
-    mapping(uint256 => bytes32) private l2Hashes;
-    mapping(uint256 => bytes32) private l1Hashes;
+    mapping(uint256 => bytes32) private _l2Hashes;
+    mapping(uint256 => bytes32) private _l1Hashes;
     bytes32 public publicInputHash;
     uint256 public latestSyncedL1Height;
 
@@ -63,12 +65,13 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
      * External Functions *
      **********************/
 
-    /** Persist the latest L1 block height and hash to L2 for cross-layer
-     *        bridging. This function will also check certain block-level global
-     *        variables because they are not part of the Trie structure.
+    /**
+     * Persist the latest L1 block height and hash to L2 for cross-layer
+     * message verification (eg. bridging). This function will also check
+     * certain block-level global variables because they are not part of the
+     * Trie structure.
      *
-     *        Note that this transaction shall be the first transaction in every
-     *        L2 block.
+     * Note: This transaction shall be the first transaction in every L2 block.
      *
      * @param l1Height The latest L1 block height when this block was proposed.
      * @param l1Hash The latest L1 block hash when this block was proposed.
@@ -80,7 +83,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
         }
 
         latestSyncedL1Height = l1Height;
-        l1Hashes[l1Height] = l1Hash;
+        _l1Hashes[l1Height] = l1Hash;
         emit HeaderSynced(block.number, l1Height, l1Hash);
     }
 
@@ -135,11 +138,11 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
     function getSyncedHeader(
         uint256 number
     ) public view override returns (bytes32) {
-        return l1Hashes[number];
+        return _l1Hashes[number];
     }
 
     function getLatestSyncedHeader() public view override returns (bytes32) {
-        return l1Hashes[latestSyncedL1Height];
+        return _l1Hashes[latestSyncedL1Height];
     }
 
     function getBlockHash(uint256 number) public view returns (bytes32) {
@@ -148,7 +151,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
         } else if (number < block.number && number >= block.number - 256) {
             return blockhash(number);
         } else {
-            return l2Hashes[number];
+            return _l2Hashes[number];
         }
     }
 
@@ -189,7 +192,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync {
             ancestors: ancestors
         });
 
-        l2Hashes[parentHeight] = parentHash;
+        _l2Hashes[parentHeight] = parentHash;
     }
 
     function _hashPublicInputs(

packages/protocol/contracts/bridge/Bridge.sol

@@ -18,9 +18,8 @@ import "./libs/LibBridgeStatus.sol";
 /**
  * Bridge contract which is deployed on both L1 and L2. Mostly a thin wrapper
  * which calls the library implementations. See _IBridge_ for more details.
- *
- * @author dantaik <[email protected]>
  * @dev The code hash for the same address on L1 and L2 may be different.
+ * @author dantaik <[email protected]>
  */
 contract Bridge is EssentialContract, IBridge {
     using LibBridgeData for Message;
@@ -29,7 +28,7 @@ contract Bridge is EssentialContract, IBridge {
      * State Variables   *
      *********************/
 
-    LibBridgeData.State private state; // 50 slots reserved
+    LibBridgeData.State private _state; // 50 slots reserved
     uint256[50] private __gap;
 
     /*********************
@@ -47,8 +46,10 @@ contract Bridge is EssentialContract, IBridge {
      * External Functions*
      *********************/
 
-    /// Allow Bridge to receive ETH from EtherVault.
-    receive() external payable {}
+    /// Allow Bridge to receive ETH from the TokenVault or EtherVault.
+    receive() external payable {
+        // TODO(dave,PR#13110): require the sender is the TokenVault or EtherVault
+    }
 
     /// @dev Initializer to be called after being deployed behind a proxy.
     function init(address _addressManager) external initializer {
@@ -60,7 +61,7 @@ contract Bridge is EssentialContract, IBridge {
     ) external payable nonReentrant returns (bytes32 msgHash) {
         return
             LibBridgeSend.sendMessage({
-                state: state,
+                state: _state,
                 resolver: AddressResolver(this),
                 message: message
             });
@@ -72,7 +73,7 @@ contract Bridge is EssentialContract, IBridge {
     ) external nonReentrant {
         return
             LibBridgeRelease.releaseEther({
-                state: state,
+                state: _state,
                 resolver: AddressResolver(this),
                 message: message,
                 proof: proof
@@ -85,7 +86,7 @@ contract Bridge is EssentialContract, IBridge {
     ) external nonReentrant {
         return
             LibBridgeProcess.processMessage({
-                state: state,
+                state: _state,
                 resolver: AddressResolver(this),
                 message: message,
                 proof: proof
@@ -98,7 +99,7 @@ contract Bridge is EssentialContract, IBridge {
     ) external nonReentrant {
         return
             LibBridgeRetry.retryMessage({
-                state: state,
+                state: _state,
                 resolver: AddressResolver(this),
                 message: message,
                 isLastAttempt: isLastAttempt
@@ -148,7 +149,7 @@ contract Bridge is EssentialContract, IBridge {
     }
 
     function context() public view returns (Context memory) {
-        return state.ctx;
+        return _state.ctx;
     }
 
     function isDestChainEnabled(

packages/protocol/contracts/bridge/EtherVault.sol

@@ -14,7 +14,10 @@ import "../common/EssentialContract.sol";
 import "../libs/LibAddress.sol";
 
 /**
- * Vault that holds Ether.
+ * EtherVault is a special vault contract that:
+ * - Is initialized with 2^128 Ether.
+ * - Allows the contract owner to authorize addresses.
+ * - Allows authorized addresses to send/release Ether.
  * @author dantaik <[email protected]>
  */
 contract EtherVault is EssentialContract {
@@ -24,7 +27,7 @@ contract EtherVault is EssentialContract {
      * State Variables   *
      *********************/
 
-    mapping(address => bool) private authorizedAddrs;
+    mapping(address => bool) private _authorizedAddrs;
     uint256[49] private __gap;
 
     /*********************
@@ -67,20 +70,20 @@ contract EtherVault is EssentialContract {
     /**
      * Transfer Ether from EtherVault to the sender, checking that the sender
      * is authorized.
-     * @param amount Amount of ether to send.
+     * @param amount Amount of Ether to send.
      */
     function releaseEther(uint256 amount) public onlyAuthorized nonReentrant {
         msg.sender.sendEther(amount);
         emit EtherReleased(msg.sender, amount);
     }
 
     /**
-     * Transfer Ether from EtherVault to an desinated address, checking that the
+     * Transfer Ether from EtherVault to a designated address, checking that the
      * sender is authorized.
-     * @param recipient Address to receive Ether
+     * @param recipient Address to receive Ether.
      * @param amount Amount of ether to send.
      */
-    function releaseEtherTo(
+    function releaseEther(
         address recipient,
         uint256 amount
     ) public onlyAuthorized nonReentrant {
@@ -96,10 +99,10 @@ contract EtherVault is EssentialContract {
      */
     function authorize(address addr, bool authorized) public onlyOwner {
         require(
-            addr != address(0) && authorizedAddrs[addr] != authorized,
+            addr != address(0) && _authorizedAddrs[addr] != authorized,
             "EV:param"
         );
-        authorizedAddrs[addr] = authorized;
+        _authorizedAddrs[addr] = authorized;
         emit Authorized(addr, authorized);
     }
 
@@ -108,6 +111,6 @@ contract EtherVault is EssentialContract {
      * @param addr Address to get the authorized status of.
      */
     function isAuthorized(address addr) public view returns (bool) {
-        return authorizedAddrs[addr];
+        return _authorizedAddrs[addr];
     }
 }

packages/protocol/contracts/bridge/IBridge.sol

@@ -8,23 +8,37 @@ pragma solidity ^0.8.9;
 
 /**
  * Bridge interface.
- * @dev Cross-chain Ether is held by Bridges, not TokenVaults.
+ * @dev Ether is held by Bridges on L1 and by the EtherVault on L2,
+ * not TokenVaults.
  * @author dantaik <[email protected]>
  */
 interface IBridge {
     struct Message {
-        uint256 id; // auto filled
-        address sender; // auto filled
-        uint256 srcChainId; // auto filled
+        // Message ID.
+        uint256 id;
+        // Message sender address (auto filled).
+        address sender;
+        // Source chain ID (auto filled).
+        uint256 srcChainId;
+        // Destination chain ID (auto filled).
         uint256 destChainId;
+        // Owner address of the bridged asset.
         address owner;
-        address to; // target address on destChain
-        address refundAddress; // if address(0), refunds to owner
-        uint256 depositValue; // value to be deposited at "to" address
-        uint256 callValue; // value to be called on destChain
-        uint256 processingFee; // processing fee sender is willing to pay
+        // Destination address.
+        address to;
+        // Alternate address to send any refund. If blank, defaults to owner.
+        address refundAddress;
+        // Amount to bridge.
+        uint256 depositValue;
+        // callValue to invoke on the destination chain, for ERC20 transfers.
+        uint256 callValue;
+        // Processing fee for the relayer. Zero if owner will process themself.
+        uint256 processingFee;
+        // gasLimit to invoke on the destination chain, for ERC20 transfers.
         uint256 gasLimit;
-        bytes data; // calldata
+        // callData to invoke on the destination chain, for ERC20 transfers.
+        bytes data;
+        // Optional memo.
         string memo;
     }
 

packages/protocol/contracts/bridge/TokenVault.sol

@@ -19,7 +19,8 @@ import "./IBridge.sol";
  * This vault holds all ERC20 tokens (but not Ether) that users have deposited.
  * It also manages the mapping between canonical ERC20 tokens and their bridged
  * tokens.
- *
+ * @dev Ether is held by Bridges on L1 and by the EtherVault on L2,
+ * not TokenVaults.
  * @author dantaik <[email protected]>
  */
 contract TokenVault is EssentialContract {
@@ -95,6 +96,7 @@ contract TokenVault is EssentialContract {
         address token,
         uint256 amount
     );
+
     event ERC20Received(
         bytes32 indexed msgHash,
         address indexed from,
@@ -113,14 +115,14 @@ contract TokenVault is EssentialContract {
     }
 
     /**
-     * Transfers Ether to this vault and sends a message to the destination
-     * chain so the user can receive Ether.
-     *
-     * @dev Ether is held by Bridges on L1 and by the EtherVault on L2,
-     *      not TokenVaults.
-     * @param destChainId The destination chain ID where the `to` address lives.
-     * @param to The destination address.
-     * @param processingFee @custom:see Bridge
+     * Receives Ether and constructs a Bridge message. Sends the Ether and
+     * message along to the Bridge.
+     * @param destChainId @custom:see IBridge.Message.
+     * @param to @custom:see IBridge.Message.
+     * @param gasLimit @custom:see IBridge.Message.
+     * @param processingFee @custom:see IBridge.Message
+     * @param refundAddress @custom:see IBridge.Message
+     * @param memo @custom:see IBridge.Message
      */
     function sendEther(
         uint256 destChainId,
@@ -141,17 +143,15 @@ contract TokenVault is EssentialContract {
         message.destChainId = destChainId;
         message.owner = msg.sender;
         message.to = to;
-
         message.gasLimit = gasLimit;
         message.processingFee = processingFee;
         message.depositValue = msg.value - processingFee;
         message.refundAddress = refundAddress;
         message.memo = memo;
 
+        // prevent future PRs from changing the callValue when it must be zero
         require(message.callValue == 0, "V:callValue");
 
-        // Ether are held by the Bridge on L1 and by the EtherVault on L2, not
-        // the TokenVault
         bytes32 msgHash = IBridge(resolve("bridge", false)).sendMessage{
             value: msg.value
         }(message);

packages/protocol/contracts/bridge/libs/LibBridgeData.sol

@@ -13,8 +13,7 @@ import "../../libs/LibMath.sol";
 import "../IBridge.sol";
 
 /**
- * Stores message data for the bridge.
- *
+ * Stores message metadata on the Bridge.
  * @author dantaik <[email protected]>
  */
 library LibBridgeData {
@@ -40,8 +39,8 @@ library LibBridgeData {
     event DestChainEnabled(uint256 indexed chainId, bool enabled);
 
     /**
-     * @dev Hashes messages and returns the hash signed with
-     * "TAIKO_BRIDGE_MESSAGE" for verification.
+     * @return msgHash The keccak256 hash of the message encoded with
+     * "TAIKO_BRIDGE_MESSAGE".
      */
     function hashMessage(
         IBridge.Message memory message