Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bubble up errors from UserOperationRevertReason #29

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Bubble up errors from UserOperationRevertReason #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 54 additions & 51 deletions test/KintoWallet.t.sol
View file
Open in desktop
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
Open in desktop
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");
}
}
}
}
}