Skip to content

Commit

Permalink
Merge pull request #29 from KintoXYZ/error-bubble
Browse files Browse the repository at this point in the history 
Bubble up errors from `UserOperationRevertReason`
  • Loading branch information
@fedealconada
fedealconada authored Jan 5, 2024
2 parents 50904e5 + 271cac0 commit 03ee289
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 51 deletions.
105 changes: 54 additions & 51 deletions test/KintoWallet.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/Signa
import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

import "forge-std/Test.sol";
import {Test, stdError} from "forge-std/Test.sol";
import "forge-std/console.sol";

contract KintoWalletv2 is KintoWallet {
Expand Down Expand Up @@ -109,13 +109,10 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// execute the transaction via the entry point and expect a revert event
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOp),
userOp.sender,
userOp.nonce,
bytes("") // todo: add revert reason
);
emit UserOperationRevertReason(_entryPoint.getUserOpHash(userOp), userOp.sender, userOp.nonce, bytes(""));
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("Address: low-level call with value failed");
}

function test_RevertWhen_OthersCannotUpgrade() public {
Expand Down Expand Up @@ -147,14 +144,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// execute the transaction via the entry point
// @dev handleOps seems to fail silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOp),
userOp.sender,
userOp.nonce,
bytes("") // todo: add revert reason
);
emit UserOperationRevertReason(_entryPoint.getUserOpHash(userOp), userOp.sender, userOp.nonce, bytes(""));

vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW: app not whitelisted");

vm.stopPrank();
}
Expand Down Expand Up @@ -248,12 +242,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// execute the transaction via the entry point
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW: app not whitelisted");
assertEq(counter.count(), 0);
}

Expand Down Expand Up @@ -417,8 +410,23 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
userOps[0] = userOp;

// execute the transaction via the entry point
vm.expectRevert();
// todo: vm.expectRevert(abi.encodeWithSignature("FailedOpWithRevert(uint256,string,bytes)", 0, "AA33 reverted", "TODO"));

// prepare expected error message
uint256 expectedOpIndex = 0; // Adjust as needed
string memory expectedMessage = "AA33 reverted";
bytes memory additionalMessage =
abi.encodePacked("SP: executeBatch must come from same contract or sender wallet");
bytes memory expectedAdditionalData = abi.encodeWithSelector(
bytes4(keccak256("Error(string)")), // Standard error selector
additionalMessage
);

// encode the entire revert reason
bytes memory expectedRevertReason = abi.encodeWithSignature(
"FailedOpWithRevert(uint256,string,bytes)", expectedOpIndex, expectedMessage, expectedAdditionalData
);

vm.expectRevert(expectedRevertReason);
_entryPoint.handleOps(userOps, payable(_owner));
}

Expand Down Expand Up @@ -470,12 +478,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("duplicate owners");
}

function test_RevertWhen_WithEmptyArray() public {
Expand All @@ -498,12 +505,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq(stdError.indexOOBError, false);
}

function test_RevertWhen_WithManyOwners() public {
Expand All @@ -530,12 +536,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW-rs: invalid array");
}

function test_RevertWhen_WithoutKYCSigner() public {
Expand Down Expand Up @@ -648,22 +653,23 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);

vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[1]),
userOps[1].sender,
userOps[1].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[1]), userOps[1].sender, userOps[1].nonce, bytes("")
);

// Execute the transaction via the entry point
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));

bytes[] memory reasons = new bytes[](2);
reasons[0] = "invalid policy";
reasons[1] = "Address: low-level call with value failed";
assertRevertReasonEq(reasons);

assertEq(_kintoWalletv1.owners(0), _owner);
assertEq(_kintoWalletv1.signerPolicy(), _kintoWalletv1.SINGLE_SIGNER());
}
Expand Down Expand Up @@ -1212,12 +1218,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW-at: app not whitelisted");
assertEq(_kintoWalletv1.isTokenApproved(app, tokens[0]), tokenApprovalBefore);
}

Expand Down Expand Up @@ -1293,12 +1298,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[1]),
userOps[1].sender,
userOps[1].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[1]), userOps[1].sender, userOps[1].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW: Direct ERC20 approval not allowed");
assertEq(_engenCredits.allowance(address(_kintoWalletv1), app), allowanceBefore);
}

Expand All @@ -1324,12 +1328,11 @@ contract KintoWalletTest is AATestScaffolding, UserOp {
// @dev handleOps fails silently (does not revert)
vm.expectEmit(true, true, true, false);
emit UserOperationRevertReason(
_entryPoint.getUserOpHash(userOps[0]),
userOps[0].sender,
userOps[0].nonce,
bytes("") // todo: add revert reason
_entryPoint.getUserOpHash(userOps[0]), userOps[0].sender, userOps[0].nonce, bytes("")
);
vm.recordLogs();
_entryPoint.handleOps(userOps, payable(_owner));
assertRevertReasonEq("KW-apk: invalid address");
assertEq(_kintoWalletv1.appSigner(app), appSignerBefore);
}

Expand Down
54 changes: 54 additions & 0 deletions test/helpers/AATestScaffolding.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,58 @@ abstract contract AATestScaffolding is KYCSignature {

vm.stopPrank();
}

////// helper methods to assert the revert reason on UserOperationRevertReason events ////

// string reasons

function assertRevertReasonEq(bytes memory _reason) public {
bytes[] memory reasons = new bytes[](1);
reasons[0] = _reason;
_assertRevertReasonEq(reasons, true);
}

// @dev if UserOperationRevertReason is string or bytes, we can specify it with isStringType
function assertRevertReasonEq(bytes memory _reason, bool isStringType) public {
bytes[] memory reasons = new bytes[](1);
reasons[0] = _reason;
_assertRevertReasonEq(reasons, isStringType);
}

// @dev if 2 or more UserOperationRevertReason events are emitted
function assertRevertReasonEq(bytes[] memory _reasons) public {
_assertRevertReasonEq(_reasons, true);
}

function _assertRevertReasonEq(bytes[] memory _reasons, bool isStringType) internal {
uint256 idx = 0;
Vm.Log[] memory logs = vm.getRecordedLogs();
for (uint256 i = 0; i < logs.length; i++) {
// check if this is the correct event
if (logs[i].topics[0] == keccak256("UserOperationRevertReason(bytes32,address,uint256,bytes)")) {
(, bytes memory revertReasonBytes) = abi.decode(logs[i].data, (uint256, bytes));

// check that the revertReasonBytes is long enough (at least 4 bytes for the selector + additional data for the message)
if (revertReasonBytes.length > 4) {
//decode the remaining bytes as a string
if (isStringType) {
// remove the first 4 bytes (the function selector)
bytes memory errorBytes = new bytes(revertReasonBytes.length - 4);
for (uint256 j = 4; j < revertReasonBytes.length; j++) {
errorBytes[j - 4] = revertReasonBytes[j];
}
string memory decodedRevertReason = abi.decode(errorBytes, (string));
// compare as strings
assertEq(decodedRevertReason, string(_reasons[idx]), "Revert reason does not match");
} else {
// compare as bytes
assertEq(revertReasonBytes, _reasons[idx], "Revert reason does not match");
}
idx++;
} else {
revert("Revert reason bytes too short to decode");
}
}
}
}
}

0 comments on commit 03ee289

Please sign in to comment.