From bd5fa2fe3aeaa5803bb561adc7776a64695ed627 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Mon, 6 Feb 2023 21:42:29 -0500 Subject: [PATCH 01/19] docs(protocol): enhance bridge documentation --- packages/protocol/contracts/L1/TaikoData.sol | 3 +- .../contracts/L1/libs/LibVerifying.sol | 11 ++++-- packages/protocol/contracts/L2/TaikoL2.sol | 12 ++++--- packages/protocol/contracts/bridge/Bridge.sol | 9 ++--- .../protocol/contracts/bridge/EtherVault.sol | 14 +++++--- .../protocol/contracts/bridge/IBridge.sol | 34 +++++++++++++------ .../protocol/contracts/bridge/TokenVault.sol | 24 ++++++------- .../contracts/bridge/libs/LibBridgeData.sol | 8 ++--- .../bridge/libs/LibBridgeProcess.sol | 29 ++++++++++------ .../bridge/libs/LibBridgeRelease.sol | 8 +++++ .../contracts/bridge/libs/LibBridgeRetry.sol | 17 +++++----- .../contracts/bridge/libs/LibBridgeSend.sol | 9 +++-- .../protocol/contracts/common/IHeaderSync.sol | 3 +- .../contracts/signal/SignalService.sol | 15 ++++---- 14 files changed, 123 insertions(+), 73 deletions(-) diff --git a/packages/protocol/contracts/L1/TaikoData.sol b/packages/protocol/contracts/L1/TaikoData.sol index a95b0edaca5..b074c92ad99 100644 --- a/packages/protocol/contracts/L1/TaikoData.sol +++ b/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; diff --git a/packages/protocol/contracts/L1/libs/LibVerifying.sol b/packages/protocol/contracts/L1/libs/LibVerifying.sol index a844c8c4350..8ad9bfdc4ee 100644 --- a/packages/protocol/contracts/L1/libs/LibVerifying.sol +++ b/packages/protocol/contracts/L1/libs/LibVerifying.sol @@ -10,7 +10,10 @@ import "../../common/AddressResolver.sol"; import "../TkoToken.sol"; import "./LibUtils.sol"; -/// @author dantaik +/** + * LibVerifying. + * @author dantaik + */ 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; diff --git a/packages/protocol/contracts/L2/TaikoL2.sol b/packages/protocol/contracts/L2/TaikoL2.sol index a98acc6dbe4..37fa87fc566 100644 --- a/packages/protocol/contracts/L2/TaikoL2.sol +++ b/packages/protocol/contracts/L2/TaikoL2.sol @@ -24,6 +24,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync { * State Variables * **********************/ + // TODO(dave): rename with underscore convention for private vars. mapping(uint256 => bytes32) private l2Hashes; mapping(uint256 => bytes32) private l1Hashes; bytes32 public publicInputHash; @@ -63,12 +64,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. diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 72670a77fb9..0ab5a298e8f 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/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 * @dev The code hash for the same address on L1 and L2 may be different. + * @author dantaik */ contract Bridge is EssentialContract, IBridge { using LibBridgeData for Message; @@ -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): 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 { diff --git a/packages/protocol/contracts/bridge/EtherVault.sol b/packages/protocol/contracts/bridge/EtherVault.sol index b698c96746b..4a710937cd4 100644 --- a/packages/protocol/contracts/bridge/EtherVault.sol +++ b/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 */ contract EtherVault is EssentialContract { @@ -24,6 +27,7 @@ contract EtherVault is EssentialContract { * State Variables * *********************/ + // TODO(dave): renamed `authorizedAddrs` to `_authorizedAddresses` mapping(address => bool) private authorizedAddrs; uint256[49] private __gap; @@ -67,7 +71,7 @@ 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); @@ -75,9 +79,10 @@ contract EtherVault is EssentialContract { } /** - * Transfer Ether from EtherVault to an desinated address, checking that the + * TODO(dave): update name to `releaseEther` + * 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( @@ -104,6 +109,7 @@ contract EtherVault is EssentialContract { } /** + * TODO(dave): remove helper, can directly use `authorizedAddrs[addr]`. * Get the authorized status of an address. * @param addr Address to get the authorized status of. */ diff --git a/packages/protocol/contracts/bridge/IBridge.sol b/packages/protocol/contracts/bridge/IBridge.sol index e5907474131..6de8b0e578b 100644 --- a/packages/protocol/contracts/bridge/IBridge.sol +++ b/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 */ 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; } diff --git a/packages/protocol/contracts/bridge/TokenVault.sol b/packages/protocol/contracts/bridge/TokenVault.sol index f047845984c..c7156db9c0c 100644 --- a/packages/protocol/contracts/bridge/TokenVault.sol +++ b/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 */ 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); diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeData.sol b/packages/protocol/contracts/bridge/libs/LibBridgeData.sol index dcaa0df9ba3..6985501d2f9 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeData.sol +++ b/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 */ library LibBridgeData { @@ -40,12 +39,13 @@ 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 ) internal pure returns (bytes32) { + // TODO(dave): can we use abi.encodePacked here? return keccak256(abi.encode("TAIKO_BRIDGE_MESSAGE", message)); } } diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol b/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol index 7feeea1bc92..58470841769 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol @@ -14,7 +14,6 @@ import "./LibBridgeStatus.sol"; /** * Process bridge messages on the destination chain. - * * @title LibBridgeProcess * @author dantaik */ @@ -31,7 +30,6 @@ library LibBridgeProcess { * then takes custody of the ether from the EtherVault and attempts to * invoke the messageCall, changing the message's status accordingly. * Finally, it refunds the processing fee if needed. - * * @param state The bridge state. * @param resolver The address resolver. * @param message The message to process. @@ -43,15 +41,15 @@ library LibBridgeProcess { IBridge.Message calldata message, bytes calldata proof ) external { + // If the gas limit is set to zero, only the owner can process the message. if (message.gasLimit == 0) { require(msg.sender == message.owner, "B:forbidden"); } - // The message's destination chain must be the current chain. require(message.destChainId == block.chainid, "B:destChainId"); - // The status of the message must be "NEW"; RETRIABLE is handled in - // LibBridgeRetry.sol + // The message status must be "NEW"; "RETRIABLE" is handled in + // LibBridgeRetry.sol. bytes32 msgHash = message.hashMessage(); require( LibBridgeStatus.getMessageStatus(msgHash) == @@ -76,30 +74,36 @@ library LibBridgeProcess { "B:notReceived" ); - // We retrieve the necessary ether from EtherVault + // We retrieve the necessary ether from EtherVault if receiving on + // Taiko, otherwise it is already available in this Bridge. address ethVault = resolver.resolve("ether_vault", true); if (ethVault != address(0)) { EtherVault(payable(ethVault)).releaseEther( message.depositValue + message.callValue + message.processingFee ); } - // We deposit Ether first before the message call in case the call - // will actually consume the Ether. + // We send the Ether before the message call in case the call will + // actually consume Ether. message.owner.sendEther(message.depositValue); LibBridgeStatus.MessageStatus status; uint256 refundAmount; + // TODO(dave): can we just catch this earlier (in sendMessage) and throw an error? + // if the user is sending to the bridge or zero-address, just process as DONE + // and refund the owner if (message.to == address(this) || message.to == address(0)) { // For these two special addresses, the call will not be actually // invoked but will be marked DONE. The callValue will be refunded. status = LibBridgeStatus.MessageStatus.DONE; refundAmount = message.callValue; } else { + // use the specified message gas limit if not called by the owner uint256 gasLimit = msg.sender == message.owner ? gasleft() : message.gasLimit; + // this will call receiveERC20 on the tokenVault, sending the tokens to the user bool success = LibBridgeInvoke.invokeMessageCall({ state: state, message: message, @@ -117,17 +121,20 @@ library LibBridgeProcess { } } + // Mark the status as DONE or RETRIABLE. LibBridgeStatus.updateMessageStatus(msgHash, status); address refundAddress = message.refundAddress == address(0) ? message.owner : message.refundAddress; + // if sender is the refundAddress if (msg.sender == refundAddress) { - refundAddress.sendEther(refundAmount + message.processingFee); + refundAddress.sendEther(message.processingFee + refundAmount); } else { - // First attempt relayer gets the processingFee - // message.owner has to eat the cost. + // if sender is another address (eg. the relayer) + // First attempt relayer is rewarded the processingFee + // message.owner has to eat the cost msg.sender.sendEther(message.processingFee); refundAddress.sendEther(refundAmount); } diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol b/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol index 9c0bce7a486..e5108a05665 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol @@ -18,6 +18,11 @@ library LibBridgeRelease { event EtherReleased(bytes32 indexed msgHash, address to, uint256 amount); + /** + * Release Ether to the message owner, only if the Taiko Bridge state says: + * - Ether for this message has not been released before. + * - The message is in a failed state. + */ function releaseEther( LibBridgeData.State storage state, AddressResolver resolver, @@ -45,12 +50,15 @@ library LibBridgeRelease { if (releaseAmount > 0) { address ethVault = resolver.resolve("ether_vault", true); + // if on Taiko if (ethVault != address(0)) { + // TODO(dave): rename to `releaseEther` EtherVault(payable(ethVault)).releaseEtherTo( message.owner, releaseAmount ); } else { + // if on Ethereum (bool success, ) = message.owner.call{value: releaseAmount}(""); require(success, "B:transfer"); } diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol b/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol index 878e5d3503e..4d288856072 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol @@ -13,7 +13,6 @@ import "./LibBridgeStatus.sol"; /** * Retry bridge messages. - * * @title LibBridgeRetry * @author dantaik */ @@ -23,17 +22,18 @@ library LibBridgeRetry { using LibBridgeData for LibBridgeData.State; /** - * Retry a bridge message on the destination chain. This function can be - * called by any address, including `message.owner`. It can only be called - * on messages marked "RETRIABLE". It attempts to reinvoke the messageCall. - * If reinvoking fails and `isLastAttempt` is set to true, then the message - * is marked "DONE" and cannot be retried. - * + * Retries to invoke the messageCall, the owner has already been sent Ether. + * - This function can be called by any address, including `message.owner`. + * - Can only be called on messages marked "RETRIABLE". + * - It attempts to reinvoke the messageCall. + * - If it succeeds, the message is marked as "DONE". + * - If it fails and `isLastAttempt` is set to true, the message is marked + * as "FAILED" and cannot be retried. * @param state The bridge state. * @param resolver The address resolver. * @param message The message to retry. * @param isLastAttempt Specifies if this is the last attempt to retry the - * message. + * message. */ function retryMessage( LibBridgeData.State storage state, @@ -61,6 +61,7 @@ library LibBridgeRetry { // successful invocation if ( + // TODO(dave): do we want to use gasleft() here? couldn't it consume beyond the gas limit? LibBridgeInvoke.invokeMessageCall({ state: state, message: message, diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeSend.sol b/packages/protocol/contracts/bridge/libs/LibBridgeSend.sol index 93bd6a6cd9d..40374192a37 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeSend.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeSend.sol @@ -20,7 +20,9 @@ library LibBridgeSend { using LibBridgeData for IBridge.Message; /** - * Initiate a bridge request. + * Send a message to the Bridge with the details of the request. The Bridge + * takes custody of the funds, unless the source chain is Taiko, in which + * the funds are sent to and managed by the EtherVault. * * @param message Specifies the `depositValue`, `callValue`, * and `processingFee`. These must sum to `msg.value`. It also specifies the @@ -49,8 +51,9 @@ library LibBridgeSend { message.processingFee; require(expectedAmount == msg.value, "B:value"); - // For each message, expectedAmount is sent to ethVault to be handled. - // Processing will retrieve these funds directly from ethVault. + // If on Taiko, send the expectedAmount to the EtherVault. Otherwise, + // store it here on the Bridge. Processing will release Ether from the + // EtherVault or the Bridge on the destination chain. address ethVault = resolver.resolve("ether_vault", true); if (ethVault != address(0)) { ethVault.sendEther(expectedAmount); diff --git a/packages/protocol/contracts/common/IHeaderSync.sol b/packages/protocol/contracts/common/IHeaderSync.sol index 755f5b9effc..586d27046c4 100644 --- a/packages/protocol/contracts/common/IHeaderSync.sol +++ b/packages/protocol/contracts/common/IHeaderSync.sol @@ -7,8 +7,9 @@ pragma solidity ^0.8.9; /** + * Interface implemented by both the TaikoL1 and TaikoL2 contracts. It exposes + * the methods needed to access the block hashes of the other chain. * @author dantaik - * @notice Interface to set and get an address for a name. */ interface IHeaderSync { event HeaderSynced( diff --git a/packages/protocol/contracts/signal/SignalService.sol b/packages/protocol/contracts/signal/SignalService.sol index 7fd7ee0ac20..61920fe0fe9 100644 --- a/packages/protocol/contracts/signal/SignalService.sol +++ b/packages/protocol/contracts/signal/SignalService.sol @@ -60,17 +60,13 @@ contract SignalService is ISignalService, EssentialContract { require(signal != 0, "B:signal"); SignalProof memory sp = abi.decode(proof, (SignalProof)); + // Resolve the TaikoL1 or TaikoL2 contract if on Ethereum or Taiko. bytes32 syncedHeaderHash = IHeaderSync(resolve("taiko", false)) .getSyncedHeader(sp.header.height); - if ( - syncedHeaderHash == 0 || - syncedHeaderHash != sp.header.hashBlockHeader() - ) { - return false; - } - return + syncedHeaderHash != 0 && + syncedHeaderHash == sp.header.hashBlockHeader() && LibTrieProof.verify({ stateRoot: sp.header.stateRoot, addr: resolve(srcChainId, "signal_service", false), @@ -80,6 +76,11 @@ contract SignalService is ISignalService, EssentialContract { }); } + /** + * @param app The srcAddress of the app (eg. the Bridge). + * @param signal The signal to store. + * @return signalSlot The storage key for the signal on the signal service. + */ function getSignalSlot( address app, bytes32 signal From 2ebec03de5d8b20a9df3d910fb0868013268993d Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:35:00 -0500 Subject: [PATCH 02/19] style --- packages/protocol/contracts/bridge/IBridge.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/protocol/contracts/bridge/IBridge.sol b/packages/protocol/contracts/bridge/IBridge.sol index 6de8b0e578b..0c34174770f 100644 --- a/packages/protocol/contracts/bridge/IBridge.sol +++ b/packages/protocol/contracts/bridge/IBridge.sol @@ -16,15 +16,15 @@ interface IBridge { struct Message { // Message ID. uint256 id; - /// Message sender address (auto filled). + // Message sender address (auto filled). address sender; - /// Source chain ID (auto filled). + // Source chain ID (auto filled). uint256 srcChainId; - /// Destination chain ID (auto filled). + // Destination chain ID (auto filled). uint256 destChainId; - /// Owner address of the bridged asset. + // Owner address of the bridged asset. address owner; - /// Destination address. + // Destination address. address to; // Alternate address to send any refund. If blank, defaults to owner. address refundAddress; From 822cc0fc5aff37121b66f54d43eece3fcafb1675 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:37:08 -0500 Subject: [PATCH 03/19] add autodocs --- .../contract-documentation/L2/TaikoL2.md | 8 +++--- .../contract-documentation/bridge/Bridge.md | 2 +- .../bridge/EtherVault.md | 20 +++++++++----- .../contract-documentation/bridge/IBridge.md | 3 ++- .../bridge/TokenVault.md | 26 +++++++++---------- .../common/IHeaderSync.md | 3 ++- .../signal/SignalService.md | 13 ++++++++++ 7 files changed, 48 insertions(+), 27 deletions(-) diff --git a/packages/website/pages/docs/reference/contract-documentation/L2/TaikoL2.md b/packages/website/pages/docs/reference/contract-documentation/L2/TaikoL2.md index 2f315b55a87..c852b7544fd 100644 --- a/packages/website/pages/docs/reference/contract-documentation/L2/TaikoL2.md +++ b/packages/website/pages/docs/reference/contract-documentation/L2/TaikoL2.md @@ -35,11 +35,11 @@ function anchor(uint256 l1Height, bytes32 l1Hash) external ``` 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. +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. #### Parameters diff --git a/packages/website/pages/docs/reference/contract-documentation/bridge/Bridge.md b/packages/website/pages/docs/reference/contract-documentation/bridge/Bridge.md index a46dfddfaaa..cfbffdae89d 100644 --- a/packages/website/pages/docs/reference/contract-documentation/bridge/Bridge.md +++ b/packages/website/pages/docs/reference/contract-documentation/bridge/Bridge.md @@ -27,7 +27,7 @@ event DestChainEnabled(uint256 chainId, bool enabled) receive() external payable ``` -Allow Bridge to receive ETH from EtherVault. +Allow Bridge to receive ETH from the TokenVault or EtherVault. ### init diff --git a/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md b/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md index f6e0635c294..4778e67ecdb 100644 --- a/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md +++ b/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md @@ -4,7 +4,11 @@ title: EtherVault ## EtherVault -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. ### Authorized @@ -49,7 +53,7 @@ is authorized. | Name | Type | Description | | ------ | ------- | ------------------------ | -| amount | uint256 | Amount of ether to send. | +| amount | uint256 | Amount of Ether to send. | ### releaseEtherTo @@ -57,15 +61,16 @@ is authorized. function releaseEtherTo(address recipient, uint256 amount) public ``` -Transfer Ether from EtherVault to an desinated address, checking that the +TODO(dave): update name to `releaseEther` +Transfer Ether from EtherVault to a designated address, checking that the sender is authorized. #### Parameters -| Name | Type | Description | -| --------- | ------- | ------------------------ | -| recipient | address | Address to receive Ether | -| amount | uint256 | Amount of ether to send. | +| Name | Type | Description | +| --------- | ------- | ------------------------- | +| recipient | address | Address to receive Ether. | +| amount | uint256 | Amount of ether to send. | ### authorize @@ -88,6 +93,7 @@ Set the authorized status of an address, only the owner can call this. function isAuthorized(address addr) public view returns (bool) ``` +TODO(dave): remove helper, can directly use `authorizedAddrs[addr]`. Get the authorized status of an address. #### Parameters diff --git a/packages/website/pages/docs/reference/contract-documentation/bridge/IBridge.md b/packages/website/pages/docs/reference/contract-documentation/bridge/IBridge.md index 9211f5fbe07..c159a78bdc4 100644 --- a/packages/website/pages/docs/reference/contract-documentation/bridge/IBridge.md +++ b/packages/website/pages/docs/reference/contract-documentation/bridge/IBridge.md @@ -6,7 +6,8 @@ title: IBridge Bridge interface. -_Cross-chain Ether is held by Bridges, not TokenVaults._ +_Ether is held by Bridges on L1 and by the EtherVault on L2, +not TokenVaults._ ### Message diff --git a/packages/website/pages/docs/reference/contract-documentation/bridge/TokenVault.md b/packages/website/pages/docs/reference/contract-documentation/bridge/TokenVault.md index 22be51518f0..a0b2e9ffdab 100644 --- a/packages/website/pages/docs/reference/contract-documentation/bridge/TokenVault.md +++ b/packages/website/pages/docs/reference/contract-documentation/bridge/TokenVault.md @@ -8,6 +8,9 @@ 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. +_Ether is held by Bridges on L1 and by the EtherVault on L2, +not TokenVaults._ + ### CanonicalERC20 ```solidity @@ -95,22 +98,19 @@ function init(address addressManager) external function sendEther(uint256 destChainId, address to, uint256 gasLimit, uint256 processingFee, address refundAddress, string memo) external payable ``` -Transfers Ether to this vault and sends a message to the destination -chain so the user can receive Ether. - -_Ether is held by Bridges on L1 and by the EtherVault on L2, -not TokenVaults._ +Receives Ether and constructs a Bridge message. Sends the Ether and +message along to the Bridge. #### Parameters -| Name | Type | Description | -| ------------- | ------- | ------------------------------------------------------ | -| destChainId | uint256 | The destination chain ID where the `to` address lives. | -| to | address | The destination address. | -| gasLimit | uint256 | | -| processingFee | uint256 | @custom:see Bridge | -| refundAddress | address | | -| memo | string | | +| Name | Type | Description | +| ------------- | ------- | ---------------------------- | +| destChainId | uint256 | @custom:see IBridge.Message. | +| to | address | @custom:see IBridge.Message. | +| gasLimit | uint256 | @custom:see IBridge.Message. | +| processingFee | uint256 | @custom:see IBridge.Message | +| refundAddress | address | @custom:see IBridge.Message | +| memo | string | @custom:see IBridge.Message | ### sendERC20 diff --git a/packages/website/pages/docs/reference/contract-documentation/common/IHeaderSync.md b/packages/website/pages/docs/reference/contract-documentation/common/IHeaderSync.md index 1c6f711c479..f9bc4ee5f37 100644 --- a/packages/website/pages/docs/reference/contract-documentation/common/IHeaderSync.md +++ b/packages/website/pages/docs/reference/contract-documentation/common/IHeaderSync.md @@ -4,7 +4,8 @@ title: IHeaderSync ## IHeaderSync -Interface to set and get an address for a name. +Interface implemented by both the TaikoL1 and TaikoL2 contracts. It exposes +the methods needed to access the block hashes of the other chain. ### HeaderSynced diff --git a/packages/website/pages/docs/reference/contract-documentation/signal/SignalService.md b/packages/website/pages/docs/reference/contract-documentation/signal/SignalService.md index a5f19bbc0c7..f2d07f762f4 100644 --- a/packages/website/pages/docs/reference/contract-documentation/signal/SignalService.md +++ b/packages/website/pages/docs/reference/contract-documentation/signal/SignalService.md @@ -78,3 +78,16 @@ Check if signal has been received on the destination chain (current). ```solidity function getSignalSlot(address app, bytes32 signal) public pure returns (bytes32) ``` + +#### Parameters + +| Name | Type | Description | +| ------ | ------- | ------------------------------------------- | +| app | address | The srcAddress of the app (eg. the Bridge). | +| signal | bytes32 | The signal to store. | + +#### Return Values + +| Name | Type | Description | +| ---- | ------- | ---------------------------------------------------------------- | +| [0] | bytes32 | signalSlot The storage key for the signal on the signal service. | From 40c3437c4d49903a19a8d993f80718d90564dcde Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:40:29 -0500 Subject: [PATCH 04/19] remove private getter --- .../protocol/contracts/bridge/EtherVault.sol | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/protocol/contracts/bridge/EtherVault.sol b/packages/protocol/contracts/bridge/EtherVault.sol index 4a710937cd4..b7c3cee8489 100644 --- a/packages/protocol/contracts/bridge/EtherVault.sol +++ b/packages/protocol/contracts/bridge/EtherVault.sol @@ -27,8 +27,7 @@ contract EtherVault is EssentialContract { * State Variables * *********************/ - // TODO(dave): renamed `authorizedAddrs` to `_authorizedAddresses` - mapping(address => bool) private authorizedAddrs; + mapping(address => bool) private _isAuthorized; uint256[49] private __gap; /********************* @@ -44,7 +43,7 @@ contract EtherVault is EssentialContract { *********************/ modifier onlyAuthorized() { - require(isAuthorized(msg.sender), "EV:denied"); + require(_isAuthorized[msg.sender], "EV:denied"); _; } @@ -55,7 +54,7 @@ contract EtherVault is EssentialContract { receive() external payable { // EthVault's balance must == 0 OR the sender isAuthorized. require( - address(this).balance == 0 || isAuthorized(msg.sender), + address(this).balance == 0 || _isAuthorized[msg.sender], "EV:denied" ); } @@ -101,19 +100,10 @@ contract EtherVault is EssentialContract { */ function authorize(address addr, bool authorized) public onlyOwner { require( - addr != address(0) && authorizedAddrs[addr] != authorized, + addr != address(0) && _isAuthorized[addr] != authorized, "EV:param" ); - authorizedAddrs[addr] = authorized; + _isAuthorized[addr] = authorized; emit Authorized(addr, authorized); } - - /** - * TODO(dave): remove helper, can directly use `authorizedAddrs[addr]`. - * Get the authorized status of an address. - * @param addr Address to get the authorized status of. - */ - function isAuthorized(address addr) public view returns (bool) { - return authorizedAddrs[addr]; - } } From 4fc4adeea7a2a48922f1129b021d83c372c03aad Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:41:13 -0500 Subject: [PATCH 05/19] overloaded function naming convention --- packages/protocol/contracts/bridge/EtherVault.sol | 3 +-- packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/protocol/contracts/bridge/EtherVault.sol b/packages/protocol/contracts/bridge/EtherVault.sol index b7c3cee8489..1ac51099c5a 100644 --- a/packages/protocol/contracts/bridge/EtherVault.sol +++ b/packages/protocol/contracts/bridge/EtherVault.sol @@ -78,13 +78,12 @@ contract EtherVault is EssentialContract { } /** - * TODO(dave): update name to `releaseEther` * Transfer Ether from EtherVault to a designated address, checking that the * sender is authorized. * @param recipient Address to receive Ether. * @param amount Amount of ether to send. */ - function releaseEtherTo( + function releaseEther( address recipient, uint256 amount ) public onlyAuthorized nonReentrant { diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol b/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol index e5108a05665..c7d7025315a 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeRelease.sol @@ -52,8 +52,7 @@ library LibBridgeRelease { address ethVault = resolver.resolve("ether_vault", true); // if on Taiko if (ethVault != address(0)) { - // TODO(dave): rename to `releaseEther` - EtherVault(payable(ethVault)).releaseEtherTo( + EtherVault(payable(ethVault)).releaseEther( message.owner, releaseAmount ); From df048ee7c9563f39822f5328a5d25a52a3b71c66 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:42:01 -0500 Subject: [PATCH 06/19] remove todo for encodepacked --- packages/protocol/contracts/bridge/libs/LibBridgeData.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeData.sol b/packages/protocol/contracts/bridge/libs/LibBridgeData.sol index 6985501d2f9..c3500e0371a 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeData.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeData.sol @@ -45,7 +45,6 @@ library LibBridgeData { function hashMessage( IBridge.Message memory message ) internal pure returns (bytes32) { - // TODO(dave): can we use abi.encodePacked here? return keccak256(abi.encode("TAIKO_BRIDGE_MESSAGE", message)); } } From 2685f3a0325f611f78bdd2ac2e6078ac55c415ec Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:46:43 -0500 Subject: [PATCH 07/19] resolve gaslimit comment --- packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol b/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol index 4d288856072..58a0a12d562 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeRetry.sol @@ -61,7 +61,8 @@ library LibBridgeRetry { // successful invocation if ( - // TODO(dave): do we want to use gasleft() here? couldn't it consume beyond the gas limit? + // The message.gasLimit only apply for processMessage, if it fails + // then whoever calls retryMessage will use the tx's gasLimit. LibBridgeInvoke.invokeMessageCall({ state: state, message: message, From 7f72418b2c1a31a6ee67874c606cee50f6416c5b Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 08:51:39 -0500 Subject: [PATCH 08/19] private state variables --- packages/protocol/contracts/L2/TaikoL2.sol | 19 ++++++++++--------- packages/protocol/contracts/bridge/Bridge.sol | 12 ++++++------ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/protocol/contracts/L2/TaikoL2.sol b/packages/protocol/contracts/L2/TaikoL2.sol index 37fa87fc566..cdcdb5bcdd0 100644 --- a/packages/protocol/contracts/L2/TaikoL2.sol +++ b/packages/protocol/contracts/L2/TaikoL2.sol @@ -16,7 +16,9 @@ import "../libs/LibInvalidTxList.sol"; import "../libs/LibSharedConfig.sol"; import "../libs/LibTxDecoder.sol"; -/// @author dantaik +/** + * @author dantaik + */ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync { using LibTxDecoder for bytes; @@ -24,9 +26,8 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync { * State Variables * **********************/ - // TODO(dave): rename with underscore convention for private vars. - 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; @@ -82,7 +83,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync { } latestSyncedL1Height = l1Height; - l1Hashes[l1Height] = l1Hash; + _l1Hashes[l1Height] = l1Hash; emit HeaderSynced(block.number, l1Height, l1Hash); } @@ -137,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) { @@ -150,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]; } } @@ -191,7 +192,7 @@ contract TaikoL2 is AddressResolver, ReentrancyGuard, IHeaderSync { ancestors: ancestors }); - l2Hashes[parentHeight] = parentHash; + _l2Hashes[parentHeight] = parentHash; } function _hashPublicInputs( diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 0ab5a298e8f..2de71ec7ea2 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -28,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; /********************* @@ -61,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 }); @@ -73,7 +73,7 @@ contract Bridge is EssentialContract, IBridge { ) external nonReentrant { return LibBridgeRelease.releaseEther({ - state: state, + state: _state, resolver: AddressResolver(this), message: message, proof: proof @@ -86,7 +86,7 @@ contract Bridge is EssentialContract, IBridge { ) external nonReentrant { return LibBridgeProcess.processMessage({ - state: state, + state: _state, resolver: AddressResolver(this), message: message, proof: proof @@ -99,7 +99,7 @@ contract Bridge is EssentialContract, IBridge { ) external nonReentrant { return LibBridgeRetry.retryMessage({ - state: state, + state: _state, resolver: AddressResolver(this), message: message, isLastAttempt: isLastAttempt @@ -149,7 +149,7 @@ contract Bridge is EssentialContract, IBridge { } function context() public view returns (Context memory) { - return state.ctx; + return _state.ctx; } function isDestChainEnabled(uint256 _chainId) public view returns (bool) { From 548e22fdffe12c80d83830fe2924d531be462a1f Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:41:53 -0500 Subject: [PATCH 09/19] update --- .../protocol/contracts/bridge/EtherVault.sol | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/protocol/contracts/bridge/EtherVault.sol b/packages/protocol/contracts/bridge/EtherVault.sol index 1ac51099c5a..b2047107073 100644 --- a/packages/protocol/contracts/bridge/EtherVault.sol +++ b/packages/protocol/contracts/bridge/EtherVault.sol @@ -27,7 +27,7 @@ contract EtherVault is EssentialContract { * State Variables * *********************/ - mapping(address => bool) private _isAuthorized; + mapping(address => bool) private _authorizedAddrs; uint256[49] private __gap; /********************* @@ -43,7 +43,7 @@ contract EtherVault is EssentialContract { *********************/ modifier onlyAuthorized() { - require(_isAuthorized[msg.sender], "EV:denied"); + require(isAuthorized(msg.sender), "EV:denied"); _; } @@ -54,7 +54,7 @@ contract EtherVault is EssentialContract { receive() external payable { // EthVault's balance must == 0 OR the sender isAuthorized. require( - address(this).balance == 0 || _isAuthorized[msg.sender], + address(this).balance == 0 || isAuthorized(msg.sender), "EV:denied" ); } @@ -78,12 +78,13 @@ contract EtherVault is EssentialContract { } /** + * TODO(dave): update name to `releaseEther` * Transfer Ether from EtherVault to a designated address, checking that the * sender is authorized. * @param recipient Address to receive Ether. * @param amount Amount of ether to send. */ - function releaseEther( + function releaseEtherTo( address recipient, uint256 amount ) public onlyAuthorized nonReentrant { @@ -99,10 +100,18 @@ contract EtherVault is EssentialContract { */ function authorize(address addr, bool authorized) public onlyOwner { require( - addr != address(0) && _isAuthorized[addr] != authorized, + addr != address(0) && _authorizedAddrs[addr] != authorized, "EV:param" ); - _isAuthorized[addr] = authorized; + _authorizedAddrs[addr] = authorized; emit Authorized(addr, authorized); } + + /** + * Get the authorized status of an address. + * @param addr Address to get the authorized status of. + */ + function isAuthorized(address addr) public view returns (bool) { + return _authorizedAddrs[addr]; + } } From 56cb190cb889c0012d2fc1b52f23949d8854c984 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:42:38 -0500 Subject: [PATCH 10/19] releaseEtherTo --- packages/protocol/contracts/bridge/EtherVault.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/protocol/contracts/bridge/EtherVault.sol b/packages/protocol/contracts/bridge/EtherVault.sol index b2047107073..d19ade2d592 100644 --- a/packages/protocol/contracts/bridge/EtherVault.sol +++ b/packages/protocol/contracts/bridge/EtherVault.sol @@ -78,13 +78,12 @@ contract EtherVault is EssentialContract { } /** - * TODO(dave): update name to `releaseEther` * Transfer Ether from EtherVault to a designated address, checking that the * sender is authorized. * @param recipient Address to receive Ether. * @param amount Amount of ether to send. */ - function releaseEtherTo( + function releaseEther( address recipient, uint256 amount ) public onlyAuthorized nonReentrant { From 1b55d09fac19bda194a1d4e98e7c626638bf2246 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:43:31 -0500 Subject: [PATCH 11/19] update docs --- packages/protocol/contracts/bridge/Bridge.sol | 2 +- .../reference/contract-documentation/bridge/EtherVault.md | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 2de71ec7ea2..fac15fca981 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -48,7 +48,7 @@ contract Bridge is EssentialContract, IBridge { /// Allow Bridge to receive ETH from the TokenVault or EtherVault. receive() external payable { - // TODO(dave): require the sender is the TokenVault or EtherVault? + // TODO(dave): require the sender is the TokenVault or EtherVault } /// @dev Initializer to be called after being deployed behind a proxy. diff --git a/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md b/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md index 4778e67ecdb..45723cb3d30 100644 --- a/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md +++ b/packages/website/pages/docs/reference/contract-documentation/bridge/EtherVault.md @@ -55,13 +55,12 @@ is authorized. | ------ | ------- | ------------------------ | | amount | uint256 | Amount of Ether to send. | -### releaseEtherTo +### releaseEther ```solidity -function releaseEtherTo(address recipient, uint256 amount) public +function releaseEther(address recipient, uint256 amount) public ``` -TODO(dave): update name to `releaseEther` Transfer Ether from EtherVault to a designated address, checking that the sender is authorized. @@ -93,7 +92,6 @@ Set the authorized status of an address, only the owner can call this. function isAuthorized(address addr) public view returns (bool) ``` -TODO(dave): remove helper, can directly use `authorizedAddrs[addr]`. Get the authorized status of an address. #### Parameters From b3719dd508a779e7a74047cb384d7929d8012ad0 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:48:13 -0500 Subject: [PATCH 12/19] leave pr in todo --- packages/protocol/contracts/bridge/Bridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index fac15fca981..871f65ef49a 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -48,7 +48,7 @@ contract Bridge is EssentialContract, IBridge { /// Allow Bridge to receive ETH from the TokenVault or EtherVault. receive() external payable { - // TODO(dave): require the sender is the TokenVault or EtherVault + // TODO(dave,PR#13110): require the sender is the TokenVault or EtherVault } /// @dev Initializer to be called after being deployed behind a proxy. From 0d1c6cbbeb7cd83a13321bcb2820ec171430901b Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:50:55 -0500 Subject: [PATCH 13/19] remove todo --- packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol b/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol index 58470841769..9b149ba891d 100644 --- a/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol +++ b/packages/protocol/contracts/bridge/libs/LibBridgeProcess.sol @@ -89,7 +89,6 @@ library LibBridgeProcess { LibBridgeStatus.MessageStatus status; uint256 refundAmount; - // TODO(dave): can we just catch this earlier (in sendMessage) and throw an error? // if the user is sending to the bridge or zero-address, just process as DONE // and refund the owner if (message.to == address(this) || message.to == address(0)) { From 8da00cff0f51e89f79e319e7f1947dc3b0d4475c Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:45:40 -0500 Subject: [PATCH 14/19] dirty save --- packages/protocol/contracts/bridge/Bridge.sol | 12 ++ packages/protocol/test/bridge/Bridge.test.ts | 120 +++++++++++++++++- 2 files changed, 129 insertions(+), 3 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 871f65ef49a..ced4919e2ce 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -48,7 +48,19 @@ contract Bridge is EssentialContract, IBridge { /// Allow Bridge to receive ETH from the TokenVault or EtherVault. receive() external payable { +<<<<<<< HEAD // TODO(dave,PR#13110): require the sender is the TokenVault or EtherVault +======= + // If on Ethereum, ensure sender is TokenVault. If on Taiko, ensure + // sender is TokenVault or EtherVault. + require( + address(0) == this.resolve("ether_vault", true) + ? msg.sender == this.resolve("token_vault", false) + : (msg.sender == this.resolve("token_vault", false) || + msg.sender == this.resolve("ether_vault", true)), + "B:receive" + ); +>>>>>>> 6319132b (dirty save) } /// @dev Initializer to be called after being deployed behind a proxy. diff --git a/packages/protocol/test/bridge/Bridge.test.ts b/packages/protocol/test/bridge/Bridge.test.ts index 40b93dd54ca..548bed75103 100644 --- a/packages/protocol/test/bridge/Bridge.test.ts +++ b/packages/protocol/test/bridge/Bridge.test.ts @@ -6,13 +6,14 @@ import { deployBridge, sendMessage } from "../utils/bridge"; import { deploySignalService } from "../utils/signal"; import { Message } from "../utils/message"; -describe("Bridge", function () { +describe("Bridge", () => { let owner: any; let nonOwner: any; let srcChainId: number; let enabledDestChainId: number; let l1Bridge: Bridge; let l1EtherVault: EtherVault; + let addressManager: AddressManager; beforeEach(async () => { [owner, nonOwner] = await ethers.getSigners(); @@ -23,7 +24,7 @@ describe("Bridge", function () { enabledDestChainId = srcChainId + 1; - const addressManager: AddressManager = await ( + addressManager = await ( await ethers.getContractFactory("AddressManager") ).deploy(); await addressManager.init(); @@ -40,9 +41,122 @@ describe("Bridge", function () { `${enabledDestChainId}.bridge`, "0x0000000000000000000000000000000000000001" // dummy address so chain is "enabled" ); + await addressManager.setAddress( + `${srcChainId}.token_vault`, + "0x0000000000000000000000000000000000000002" + ); + await addressManager.setAddress( + `${enabledDestChainId}.token_vault`, + "0x0000000000000000000000000000000000000002" + ); + await addressManager.setAddress( + `${enabledDestChainId}.ether_vault`, + "0x0000000000000000000000000000000000000003" + ); + }); + + describe("receive()", () => { + it("throws when an address other than TokenVault tries to send funds to the bridge on L1", async () => { + const message: Message = { + id: 1, + sender: owner.address, + srcChainId: 1, + destChainId: 5, + owner: ethers.constants.AddressZero, + to: nonOwner.address, + refundAddress: owner.address, + depositValue: 1, + callValue: 1, + processingFee: 1, + gasLimit: 100, + data: ethers.constants.HashZero, + memo: "", + }; + await expect( + owner.sendTransaction({ + to: l1Bridge.address, + value: message.depositValue, + }) + ).to.be.revertedWith("B:receive"); + }); + it("throws when an address other than TokenVault or EtherVault tries to send funds to the bridge on L2", async () => { + const message: Message = { + id: 1, + sender: owner.address, + srcChainId: 1, + destChainId: 5, + owner: ethers.constants.AddressZero, + to: nonOwner.address, + refundAddress: owner.address, + depositValue: 1, + callValue: 1, + processingFee: 1, + gasLimit: 100, + data: ethers.constants.HashZero, + memo: "", + }; + await expect( + owner.sendTransaction({ + to: l1Bridge.address, + value: message.depositValue, + }) + ).to.be.revertedWith("B:receive"); + }); + it("succeeds when the TokenVault sends funds to the bridge on L1", async () => { + const message: Message = { + id: 1, + sender: owner.address, + srcChainId: 1, + destChainId: 5, + owner: ethers.constants.AddressZero, + to: nonOwner.address, + refundAddress: owner.address, + depositValue: 1, + callValue: 1, + processingFee: 1, + gasLimit: 100, + data: ethers.constants.HashZero, + memo: "", + }; + const tokenVaultAddress = await addressManager.getAddress( + `${srcChainId}.token_vault` + ); + await expect( + owner.sendTransaction({ + to: tokenVaultAddress, + value: message.depositValue, + }) + ).to.not.be.reverted; + }); + it("succeeds when the EtherVault sends funds to the bridge on L2", async () => { + const message: Message = { + id: 1, + sender: owner.address, + srcChainId: 1, + destChainId: 5, + owner: ethers.constants.AddressZero, + to: nonOwner.address, + refundAddress: owner.address, + depositValue: 1, + callValue: 1, + processingFee: 1, + gasLimit: 100, + data: ethers.constants.HashZero, + memo: "", + }; + const etherVaultAddress = await addressManager.getAddress( + `${enabledDestChainId}.ether_vault` + ); + await expect( + owner.sendTransaction({ + to: etherVaultAddress, + value: message.depositValue, + }) + ).to.not.be.reverted; + }); }); - describe("sendMessage()", function () { + describe("sendMessage()", () => { it("throws when owner is the zero address", async () => { const message: Message = { id: 1, From d6cbdeb524c9b8941454afc5a0e347b80e0c6117 Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Tue, 7 Feb 2023 09:57:00 -0500 Subject: [PATCH 15/19] dan comments --- packages/protocol/contracts/bridge/Bridge.sol | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index ced4919e2ce..1e6ac3eb913 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -48,19 +48,17 @@ contract Bridge is EssentialContract, IBridge { /// Allow Bridge to receive ETH from the TokenVault or EtherVault. receive() external payable { -<<<<<<< HEAD - // TODO(dave,PR#13110): require the sender is the TokenVault or EtherVault -======= // If on Ethereum, ensure sender is TokenVault. If on Taiko, ensure // sender is TokenVault or EtherVault. - require( - address(0) == this.resolve("ether_vault", true) - ? msg.sender == this.resolve("token_vault", false) - : (msg.sender == this.resolve("token_vault", false) || - msg.sender == this.resolve("ether_vault", true)), - "B:receive" - ); ->>>>>>> 6319132b (dirty save) + if (address(0) == this.resolve("ether_vault", true)) { + require(msg.sender == this.resolve("token_vault", false)); + } else { + // on Taiko + require( + msg.sender == this.resolve("token_vault", false) || + msg.sender == this.resolve("ether_vault", true) + ); + } } /// @dev Initializer to be called after being deployed behind a proxy. From d1d3e97e27ca7b832adb9074b4fffb03e9ddef56 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Wed, 8 Feb 2023 13:22:33 +0800 Subject: [PATCH 16/19] Update Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 1e6ac3eb913..54127cb6ff5 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -50,15 +50,12 @@ contract Bridge is EssentialContract, IBridge { receive() external payable { // If on Ethereum, ensure sender is TokenVault. If on Taiko, ensure // sender is TokenVault or EtherVault. - if (address(0) == this.resolve("ether_vault", true)) { - require(msg.sender == this.resolve("token_vault", false)); - } else { - // on Taiko - require( - msg.sender == this.resolve("token_vault", false) || - msg.sender == this.resolve("ether_vault", true) - ); - } + address etherVault = this.resolve("ether_vault", true); + + require( + msg.sender == etherVault || + msg.sender == this.resolve("token_vault", false) + ); } /// @dev Initializer to be called after being deployed behind a proxy. From 1c5fbd6fdbb3ae3696811b294187ba605ada3e68 Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Wed, 8 Feb 2023 13:22:46 +0800 Subject: [PATCH 17/19] Update Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 54127cb6ff5..6908c1adf5f 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -50,10 +50,8 @@ contract Bridge is EssentialContract, IBridge { receive() external payable { // If on Ethereum, ensure sender is TokenVault. If on Taiko, ensure // sender is TokenVault or EtherVault. - address etherVault = this.resolve("ether_vault", true); - require( - msg.sender == etherVault || + msg.sender == this.resolve("ether_vault", true) || msg.sender == this.resolve("token_vault", false) ); } From e46986458ef7829c2e581416100d67a6a6c5f53d Mon Sep 17 00:00:00 2001 From: Daniel Wang Date: Wed, 8 Feb 2023 13:23:43 +0800 Subject: [PATCH 18/19] Update Bridge.sol --- packages/protocol/contracts/bridge/Bridge.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index 6908c1adf5f..f3de76ea6b0 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/packages/protocol/contracts/bridge/Bridge.sol @@ -48,11 +48,10 @@ contract Bridge is EssentialContract, IBridge { /// Allow Bridge to receive ETH from the TokenVault or EtherVault. receive() external payable { - // If on Ethereum, ensure sender is TokenVault. If on Taiko, ensure - // sender is TokenVault or EtherVault. + // Ensure the sender is either the Ether vault or the token vault. require( - msg.sender == this.resolve("ether_vault", true) || - msg.sender == this.resolve("token_vault", false) + msg.sender == this.resolve("token_vault", false) || + msg.sender == this.resolve("ether_vault", true) ); } From 4164c5cc0e0987f25b438babd4efb5057ed8ec9b Mon Sep 17 00:00:00 2001 From: dave | d1onys1us <13951458+d1onys1us@users.noreply.github.com> Date: Wed, 8 Feb 2023 11:53:19 -0500 Subject: [PATCH 19/19] remove tests --- packages/protocol/test/bridge/Bridge.test.ts | 120 +------------------ 1 file changed, 3 insertions(+), 117 deletions(-) diff --git a/packages/protocol/test/bridge/Bridge.test.ts b/packages/protocol/test/bridge/Bridge.test.ts index 548bed75103..40b93dd54ca 100644 --- a/packages/protocol/test/bridge/Bridge.test.ts +++ b/packages/protocol/test/bridge/Bridge.test.ts @@ -6,14 +6,13 @@ import { deployBridge, sendMessage } from "../utils/bridge"; import { deploySignalService } from "../utils/signal"; import { Message } from "../utils/message"; -describe("Bridge", () => { +describe("Bridge", function () { let owner: any; let nonOwner: any; let srcChainId: number; let enabledDestChainId: number; let l1Bridge: Bridge; let l1EtherVault: EtherVault; - let addressManager: AddressManager; beforeEach(async () => { [owner, nonOwner] = await ethers.getSigners(); @@ -24,7 +23,7 @@ describe("Bridge", () => { enabledDestChainId = srcChainId + 1; - addressManager = await ( + const addressManager: AddressManager = await ( await ethers.getContractFactory("AddressManager") ).deploy(); await addressManager.init(); @@ -41,122 +40,9 @@ describe("Bridge", () => { `${enabledDestChainId}.bridge`, "0x0000000000000000000000000000000000000001" // dummy address so chain is "enabled" ); - await addressManager.setAddress( - `${srcChainId}.token_vault`, - "0x0000000000000000000000000000000000000002" - ); - await addressManager.setAddress( - `${enabledDestChainId}.token_vault`, - "0x0000000000000000000000000000000000000002" - ); - await addressManager.setAddress( - `${enabledDestChainId}.ether_vault`, - "0x0000000000000000000000000000000000000003" - ); - }); - - describe("receive()", () => { - it("throws when an address other than TokenVault tries to send funds to the bridge on L1", async () => { - const message: Message = { - id: 1, - sender: owner.address, - srcChainId: 1, - destChainId: 5, - owner: ethers.constants.AddressZero, - to: nonOwner.address, - refundAddress: owner.address, - depositValue: 1, - callValue: 1, - processingFee: 1, - gasLimit: 100, - data: ethers.constants.HashZero, - memo: "", - }; - await expect( - owner.sendTransaction({ - to: l1Bridge.address, - value: message.depositValue, - }) - ).to.be.revertedWith("B:receive"); - }); - it("throws when an address other than TokenVault or EtherVault tries to send funds to the bridge on L2", async () => { - const message: Message = { - id: 1, - sender: owner.address, - srcChainId: 1, - destChainId: 5, - owner: ethers.constants.AddressZero, - to: nonOwner.address, - refundAddress: owner.address, - depositValue: 1, - callValue: 1, - processingFee: 1, - gasLimit: 100, - data: ethers.constants.HashZero, - memo: "", - }; - await expect( - owner.sendTransaction({ - to: l1Bridge.address, - value: message.depositValue, - }) - ).to.be.revertedWith("B:receive"); - }); - it("succeeds when the TokenVault sends funds to the bridge on L1", async () => { - const message: Message = { - id: 1, - sender: owner.address, - srcChainId: 1, - destChainId: 5, - owner: ethers.constants.AddressZero, - to: nonOwner.address, - refundAddress: owner.address, - depositValue: 1, - callValue: 1, - processingFee: 1, - gasLimit: 100, - data: ethers.constants.HashZero, - memo: "", - }; - const tokenVaultAddress = await addressManager.getAddress( - `${srcChainId}.token_vault` - ); - await expect( - owner.sendTransaction({ - to: tokenVaultAddress, - value: message.depositValue, - }) - ).to.not.be.reverted; - }); - it("succeeds when the EtherVault sends funds to the bridge on L2", async () => { - const message: Message = { - id: 1, - sender: owner.address, - srcChainId: 1, - destChainId: 5, - owner: ethers.constants.AddressZero, - to: nonOwner.address, - refundAddress: owner.address, - depositValue: 1, - callValue: 1, - processingFee: 1, - gasLimit: 100, - data: ethers.constants.HashZero, - memo: "", - }; - const etherVaultAddress = await addressManager.getAddress( - `${enabledDestChainId}.ether_vault` - ); - await expect( - owner.sendTransaction({ - to: etherVaultAddress, - value: message.depositValue, - }) - ).to.not.be.reverted; - }); }); - describe("sendMessage()", () => { + describe("sendMessage()", function () { it("throws when owner is the zero address", async () => { const message: Message = { id: 1,