Merge pull request #25 from Phala-Network/debug

EVM rollup error handling and debug improvements

evm/README.md

@@ -11,3 +11,17 @@ REPORT_GAS=true npx hardhat test
 npx hardhat node
 npx hardhat run scripts/deploy.ts
 ```
+
+## Gas metering
+
+```shell
+REPORT_GAS=1 npx hardhat test ./test/PhatRollupAnchor.ts
+REPORT_GAS=1 npx hardhat test ./test/SampleOracle.ts
+```
+
+## Coverage
+
+``shell
+npx hardhat coverage
+# miniserve ./coverage
+```

evm/contracts/MetaTransaction.sol

@@ -19,6 +19,9 @@ contract MetaTxReceiver is EIP712, Context {
 
     bytes32 private constant _TYPEHASH =
         keccak256("ForwardRequest(address from,uint256 nonce,bytes data)");
+
+    error NonceTooLow(uint256 actual, uint256 currentNonce);
+    error MetaTxSignatureNotMatch();
 
     mapping(address => uint256) private _nonces;
 
@@ -35,7 +38,9 @@ contract MetaTxReceiver is EIP712, Context {
     }
 
     function metaTxPrepareWithNonce(address from, bytes calldata data, uint256 nonce) public view returns (ForwardRequest memory, bytes32) {
-        require(nonce >= _nonces[from], "nonce too low");
+        if (nonce < _nonces[from]) {
+            revert NonceTooLow(nonce, _nonces[from]);
+        }
         ForwardRequest memory req = ForwardRequest(from, nonce, data);
         bytes32 hash = _hashTypedDataV4(
             keccak256(abi.encode(_TYPEHASH, from, nonce, keccak256(data)))
@@ -56,7 +61,9 @@ contract MetaTxReceiver is EIP712, Context {
         ForwardRequest calldata req,
         bytes calldata signature
     ) {
-        require(metaTxVerify(req, signature), "MetaTxReceiver: signature does not match request");
+        if (!metaTxVerify(req, signature)) {
+            revert MetaTxSignatureNotMatch();
+        }
         _nonces[req.from] = req.nonce + 1;
         _;
     }

evm/contracts/PhatRollupAnchor.sol

@@ -62,9 +62,19 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
     // Only submission from attestor is allowed.
     bytes32 public constant ATTESTOR_ROLE = keccak256("ATTESTOR_ROLE");
 
+    event MetaTxDecoded();
     event MessageQueued(uint256 idx, bytes data);
     event MessageProcessedTo(uint256);
 
+    error BadAttestor();
+    error BadCondLen(uint kenLen, uint valueLen);
+    error BadUpdateLen(uint kenLen, uint valueLen);
+    error CondNotMet(bytes cond, uint32 expected, uint32 actual);
+    error CannotDecodeAction(uint8 actionId);
+    error UnsupportedAction(uint8 actionId);
+    error Internal_toUint32Strict_outOfBounds(bytes data);
+    error InvalidPopTarget(uint256 targetIdx, uint256 tailIdx);
+
     uint8 constant ACTION_REPLY = 0;
     uint8 constant ACTION_SET_QUEUE_HEAD = 1;
     uint8 constant ACTION_GRANT_ATTESTOR = 10;
@@ -87,23 +97,35 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
         bytes[] calldata actions
     ) public returns (bool) {
         // Allow meta tx to call itself
-        require(msg.sender == address(this) || hasRole(ATTESTOR_ROLE, msg.sender), "bad attestor");
+        if (msg.sender != address(this) && !hasRole(ATTESTOR_ROLE, msg.sender)) {
+            revert BadAttestor();
+        }
         return _rollupU256CondEqInternal(condKeys, condValues, updateKeys, updateValues, actions);
     }
 
+    /// Triggers a rollup transaction similar to `rollupU256CondEq` but with meta-tx.
+    ///
+    /// Note to error handling. Most of the errors are propagated to the transaction error.
+    /// However in case of out-of-gas, the error will not be propagated. It results in a bare
+    /// "reverted" in etherscan. It's hard to debug, but you will find the gas is 100% used like
+    /// [this tx](https://mumbai.polygonscan.com/tx/0x0abe643ada209ec31a0a6da4fab546b7071e1cf265f3b4681b9bede209c400c9).
     function metaTxRollupU256CondEq(
         ForwardRequest calldata req,
         bytes calldata signature
     ) public useMetaTx(req, signature) returns (bool) {
-        require(hasRole(ATTESTOR_ROLE, req.from), "bad attestor");
+        if (!hasRole(ATTESTOR_ROLE, req.from)) {
+            revert BadAttestor();            
+        }
         (
             bytes[] memory condKeys,
             bytes[] memory condValues,
             bytes[] memory updateKeys,
             bytes[] memory updateValues,
             bytes[] memory actions
-            ) = abi.decode(req.data, (bytes[], bytes[], bytes[], bytes[], bytes[]));
-        // Self-call to move memory bytes to calldata
+        ) = abi.decode(req.data, (bytes[], bytes[], bytes[], bytes[], bytes[]));
+        emit MetaTxDecoded();
+        // Self-call to move memory bytes to calldata. Check "error handling" notes in docstring
+        // to learn more.
         return this.rollupU256CondEq(condKeys, condValues, updateKeys, updateValues, actions);
     }
 
@@ -114,15 +136,19 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
         bytes[] calldata updateValues,
         bytes[] calldata actions
     ) internal nonReentrant() returns (bool) {
-        require(condKeys.length == condValues.length, "bad cond len");
-        require(updateKeys.length == updateValues.length, "bad update len");
+        if (condKeys.length != condValues.length) {
+            revert BadCondLen(condKeys.length, condValues.length);
+        }
+        if (updateKeys.length != updateValues.length) {
+            revert BadUpdateLen(updateKeys.length, updateValues.length);
+        }
 
         // check cond
         for (uint i = 0; i < condKeys.length; i++) {
             uint32 value = toUint32Strict(kvStore[condKeys[i]]);
             uint32 expected = toUint32Strict(condValues[i]);
             if (value != expected) {
-                revert("cond not met");
+                revert CondNotMet(condKeys[i], expected, value);
             }
         }
 
@@ -144,19 +170,25 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
         if (actionType == ACTION_REPLY) {
             _onMessageReceived(action[1:]);
         } else if (actionType == ACTION_SET_QUEUE_HEAD) {
-            require(action.length >= 1 + 32, "ACTION_SET_QUEUE_HEAD cannot decode");
+            if (action.length < 1 + 32) {
+                revert CannotDecodeAction(ACTION_SET_QUEUE_HEAD);
+            }
             uint32 targetIdx = abi.decode(action[1:], (uint32));
             _popTo(targetIdx);
         } else if (actionType == ACTION_GRANT_ATTESTOR) {
-            require(action.length >= 1 + 20, "ACTION_GRANT_ATTESTOR cannot decode");
+            if (action.length < 1 + 20) {
+                revert CannotDecodeAction(ACTION_GRANT_ATTESTOR);
+            }
             address attestor = abi.decode(action[1:], (address));
             _grantRole(ATTESTOR_ROLE, attestor);
         } else if (actionType == ACTION_REVOKE_ATTESTOR) {
-            require(action.length >= 1 + 20, "ACTION_REVOKE_ATTESTOR cannot decode");
+            if (action.length < 1 + 20) {
+                revert CannotDecodeAction(ACTION_REVOKE_ATTESTOR);
+            }
             address attestor = abi.decode(action[1:], (address));
             _revokeRole(ATTESTOR_ROLE, attestor);
         } else {
-            revert("unsupported action");
+            revert UnsupportedAction(actionType);
         }
     }
 
@@ -168,7 +200,9 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
         if (_bytes.length == 0) {
             return 0;
         }
-        require(_bytes.length == 32, "toUint32Strict_outOfBounds");
+        if (_bytes.length != 32) {
+            revert Internal_toUint32Strict_outOfBounds(_bytes);
+        }
         uint32 v = abi.decode(_bytes, (uint32));
         return v;
     }
@@ -189,7 +223,9 @@ abstract contract PhatRollupAnchor is ReentrancyGuard, MetaTxReceiver, AccessCon
 
     function _popTo(uint32 targetIdx) internal {
         uint32 curTail = queueGetUint(KEY_TAIL);
-        require(targetIdx <= curTail, "invalid pop target");
+        if (targetIdx > curTail) {
+            revert InvalidPopTarget(targetIdx, curTail);
+        }
         for (uint32 i = queueGetUint(KEY_HEAD); i < targetIdx; i++) {
             queueRemoveItem(i);
         }

evm/test/PhatRollupAnchor.ts

@@ -43,7 +43,7 @@ describe("RollupAnchor", function () {
           // actions
           ['0x00DEADBEEF'],
         )
-      ).to.be.revertedWith('bad attestor');
+      ).to.be.revertedWithCustomError(target, 'BadAttestor');
     });
 
     it("Should not allow invalid input arrays", async function () {
@@ -58,7 +58,7 @@ describe("RollupAnchor", function () {
           // actions
           ['0x00DEADBEEF'],
         )
-      ).to.be.revertedWith('bad cond len');
+      ).to.be.revertedWithCustomError(target, 'BadCondLen');
 
       await expect(
         target.connect(attestor).rollupU256CondEq(
@@ -69,7 +69,7 @@ describe("RollupAnchor", function () {
           // actions
           ['0x00DEADBEEF'],
         )
-      ).to.be.revertedWith('bad update len');
+      ).to.be.revertedWithCustomError(target, 'BadUpdateLen');
     });
 
     it("Should forward actions", async function () {
@@ -123,7 +123,10 @@ describe("RollupAnchor", function () {
           // actions
           ['0x00DEADBEEF'],
         )
-      ).to.be.revertedWith('cond not met');
+      ).to.be
+        .revertedWithCustomError(target, 'CondNotMet')
+        // We want to ensure 0x01 to match 0, but the value is 1.
+        .withArgs('0x01', 0, 1);
     });
   });
 
@@ -223,6 +226,26 @@ describe("RollupAnchor", function () {
         // start
         expect(await target.queueGetUint(hex('_head'))).to.be.equals(1);
     })
+
+    it("Can propogate internal call error", async function () {
+      const { target, attestor } = await loadFixture(deployFixture);
+      // Rollup by a meta-tx
+      const [metaTxData, metaTxSig] = await metaTx([
+        ['0x00'],
+        [encodeUint32(0)],
+        ['0x00'],
+        [encodeUint32(1)],
+        [
+            ethers.utils.hexConcat(['0x00', encodeUint32(0), '0xDEADBEEF']),
+            ethers.utils.hexConcat(['0x01', encodeUint32(1)]),
+        ],
+      ], attestor, 0, target.address);
+      // Send meta-tx
+      const rollupTx = target
+        .connect(attestor)
+        .metaTxRollupU256CondEq(metaTxData, metaTxSig);
+      await expect(rollupTx).to.be.revertedWithCustomError(target, 'InvalidPopTarget');
+    })
   });
 });