H-04: Update ERC2771Forwarder executeBatch docs removing atomicity terminology

.changeset/dull-games-fly.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+Update `ERC2771Forwarder` `executeBatch` docs removing atomicity terminology

contracts/metatx/ERC2771Forwarder.sol

@@ -76,9 +76,9 @@ contract ERC2771Forwarder is EIP712, Nonces {
     event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success);
 
     /**
-     * @dev One of the calls in an atomic batch failed.
+     * @dev A request in the batch failed and no `refundReceiver` was set to handle the leftover value.
      */
-    error ERC2771ForwarderFailureInAtomicBatch();
+    error ERC2771ForwarderNoRefundReceiver();
 
     /**
      * @dev The request `from` doesn't match with the recovered `signer`.
@@ -143,40 +143,34 @@ contract ERC2771Forwarder is EIP712, Nonces {
     }
 
     /**
-     * @dev Batch version of {execute} with optional refunding and atomic execution.
+     * @dev Batch version of {execute} with optional refunding.
      *
      * In case a batch contains at least one invalid request (see {verify}), the
      * request will be skipped and the `refundReceiver` parameter will receive back the
      * unused requested value at the end of the execution. This is done to prevent reverting
      * the entire batch when a request is invalid or has already been submitted.
      *
-     * If the `refundReceiver` is the `address(0)`, this function will revert when at least
-     * one of the requests was not valid instead of skipping it. This could be useful if
-     * a batch is required to get executed atomically (at least at the top-level). For example,
-     * refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids
-     * including reverted transactions.
+     * If the `refundReceiver` is `address(0)`, the function will instead revert
+     * when any request is invalid or when a valid request's forwarded call fails while
+     * carrying value (since there is no receiver to refund the leftover ETH to).
      *
      * Requirements:
      *
      * - The sum of the requests' values should be equal to the provided `msg.value`.
      * - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address.
-     *
-     * NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for
-     * the first-level forwarded calls. In case a forwarded request calls to a contract with another
-     * subcall, the second-level call may revert without the top-level call reverting.
      */
     function executeBatch(
         ForwardRequestData[] calldata requests,
         address payable refundReceiver
     ) public payable virtual {
-        bool atomic = refundReceiver == address(0);
+        bool requireValidRequests = refundReceiver == address(0);
 
         uint256 requestsValue;
         uint256 refundValue;
 
         for (uint256 i; i < requests.length; ++i) {
             requestsValue += requests[i].value;
-            bool success = _execute(requests[i], atomic);
+            bool success = _execute(requests[i], requireValidRequests);
             if (!success) {
                 refundValue += requests[i].value;
             }
@@ -189,9 +183,9 @@ contract ERC2771Forwarder is EIP712, Nonces {
         }
 
         // Some requests with value were invalid (possibly due to frontrunning).
-        // To avoid leaving ETH in the contract this value is refunded.
+        // To avoid leaving ETH in the contract, this value is refunded.
         if (refundValue != 0) {
-            if (atomic) revert ERC2771ForwarderFailureInAtomicBatch();
+            if (requireValidRequests) revert ERC2771ForwarderNoRefundReceiver();
 
             // We know refundReceiver != address(0) && requestsValue == msg.value
             // meaning we can ensure refundValue is not taken from the original contract's balance

test/metatx/ERC2771Forwarder.test.js

@@ -228,18 +228,73 @@ describe('ERC2771Forwarder', function () {
         }
       });
 
-      it('atomic batch with reverting request reverts the whole batch', async function () {
-        // Add extra reverting request
-        await this.forgeRequest(
-          { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') },
-          this.accounts[requestCount],
-        ).then(extraRequest => this.requests.push(extraRequest));
-        // recompute total value with the extra request
-        this.value = requestsValue(this.requests);
+      it('refunds value to refund receiver when a valid request fails execution', async function () {
+        const initialRefundReceiverBalance = await ethers.provider.getBalance(this.refundReceiver);
+        const initialNonce = await this.forwarder.nonces(this.requests[idx].from);
+
+        // Replace one request with a valid-but-reverting one (signature/nonce/deadline all fine)
+        this.requests[idx] = await this.forgeRequest(
+          {
+            value: this.requests[idx].value,
+            data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason'),
+          },
+          this.accounts[idx],
+        );
 
-        await expect(
-          this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }),
-        ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderFailureInAtomicBatch');
+        const events = await this.forwarder
+          .executeBatch(this.requests, this.refundReceiver, { value: this.value })
+          .then(tx => tx.wait())
+          .then(receipt =>
+            receipt.logs.filter(
+              log => log?.fragment?.type == 'event' && log?.fragment?.name == 'ExecutedForwardRequest',
+            ),
+          );
+
+        // All requests emit the event — the reverting one has success == false
+        expect(events).to.have.lengthOf(this.requests.length);
+        expect(events[idx].args.success).to.be.false;
+
+        // Unlike a tampered request, the nonce is consumed since the request was valid
+        expect(await this.forwarder.nonces(this.requests[idx].from)).to.equal(initialNonce + 1n);
+
+        // The value is refunded to refundReceiver
+        expect(await ethers.provider.getBalance(this.refundReceiver)).to.equal(
+          initialRefundReceiverBalance + this.requests[idx].value,
+        );
+      });
+
+      describe('when the refund receiver is the zero address', function () {
+        beforeEach(function () {
+          this.refundReceiver = ethers.ZeroAddress;
+        });
+
+        it('does not revert when a failing request carries no value', async function () {
+          // Zero-value revert: nothing to refund, batch can proceed
+          await this.forgeRequest(
+            { value: 0n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') },
+            this.accounts[requestCount],
+          ).then(extraRequest => this.requests.push(extraRequest));
+          this.value = requestsValue(this.requests);
+
+          const receipt = this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value });
+
+          // The reverting request emits success == false; the others still execute normally
+          await expect(receipt)
+            .to.emit(this.forwarder, 'ExecutedForwardRequest')
+            .withArgs(this.requests.at(-1).from, this.requests.at(-1).nonce, false);
+        });
+
+        it('reverts when a failing request carries value', async function () {
+          await this.forgeRequest(
+            { value: 10n, data: this.receiver.interface.encodeFunctionData('mockFunctionRevertsNoReason') },
+            this.accounts[requestCount],
+          ).then(extraRequest => this.requests.push(extraRequest));
+          this.value = requestsValue(this.requests);
+
+          await expect(
+            this.forwarder.executeBatch(this.requests, ethers.ZeroAddress, { value: this.value }),
+          ).to.be.revertedWithCustomError(this.forwarder, 'ERC2771ForwarderNoRefundReceiver');
+        });
       });
     });