Skip to content

Commit

Permalink
:white_checkmark: addGuardian tests
Browse files Browse the repository at this point in the history
  • Loading branch information
Filipp Makarov authored and Filipp Makarov committed Oct 13, 2023
1 parent d4f3bc6 commit 6753638
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 25 deletions.
38 changes: 23 additions & 15 deletions contracts/smart-account/modules/AccountRecoveryModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
uint256 requiredSignatures = _smartAccountSettings[userOp.sender]
.recoveryThreshold;
if (requiredSignatures == 0) revert("AccRecovery: Threshold not set");

bytes calldata moduleSignature = userOp.signature[96:];

require(
Expand All @@ -196,13 +196,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
for (uint256 i; i < requiredSignatures; ) {
address currentUserOpSignerAddress = userOpHash
.toEthSignedMessageHash()
.recover(
moduleSignature[2 * i * 65:(2 * i + 1) * 65]
);
.recover(moduleSignature[2 * i * 65:(2 * i + 1) * 65]);

currentGuardianSig = moduleSignature[
(2 * i + 1) * 65 : (2 * i + 2) * 65
];
currentGuardianSig = moduleSignature[(2 * i + 1) * 65:(2 * i + 2) *
65];

currentGuardianAddress = CONTROL_HASH
.toEthSignedMessageHash()
Expand Down Expand Up @@ -292,7 +289,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
if (_guardians[guardian][msg.sender].validUntil != 0)
revert GuardianAlreadySet(guardian, msg.sender);

(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter);
(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(
validUntil,
validAfter
);

// TODO:
// make a test case that it fails if validAfter + securityDelay together overflow uint48
Expand All @@ -319,7 +319,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
if (guardian == bytes32(0)) revert ZeroGuardian();
if (newGuardian == bytes32(0)) revert ZeroGuardian();

(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter);
(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(
validUntil,
validAfter
);

// make the new one valid
_guardians[newGuardian][msg.sender] = TimeFrame(
Expand Down Expand Up @@ -358,11 +361,13 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
// REMOVE EXPIRED GUARDIAN
// not permissioned - anyone can call it but the check if the guardian is expired is on-chain
// it will allow us clearing expired guardians from the backend and maintain the list of guardians actual
function removeExpiredGuardian(bytes32 guardian, address smartAccount) external {
function removeExpiredGuardian(
bytes32 guardian,
address smartAccount
) external {
uint48 validUntil = _guardians[guardian][smartAccount].validUntil;
if (validUntil == 0)
revert GuardianNotSet(guardian, smartAccount);
if(validUntil>=block.timestamp)
if (validUntil == 0) revert GuardianNotSet(guardian, smartAccount);
if (validUntil >= block.timestamp)
revert GuardianNotExpired(guardian, smartAccount);
_removeGuardianAndChangeTresholdIfNeeded(guardian, smartAccount);
}
Expand All @@ -376,7 +381,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
// @note if validAfter is less then now + securityDelay, it is set to now + securityDelay
// as for security reasons new guardian is only active after securityDelay
function _checkAndAdjustValidUntilValidAfter(
uint48 validUntil,
uint48 validUntil,
uint48 validAfter
) internal view returns (uint48, uint48) {
if (validUntil == 0) validUntil = type(uint48).max;
Expand All @@ -392,7 +397,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
return (validUntil, validAfter);
}

function _removeGuardianAndChangeTresholdIfNeeded(bytes32 guardian, address smartAccount) internal {
function _removeGuardianAndChangeTresholdIfNeeded(
bytes32 guardian,
address smartAccount
) internal {
delete _guardians[guardian][smartAccount];
--_smartAccountSettings[smartAccount].guardiansCount;
emit GuardianRemoved(smartAccount, guardian);
Expand Down
99 changes: 89 additions & 10 deletions test/module/AccountRecovery.Module.specs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2054,22 +2054,31 @@ describe("Account Recovery Module: ", async () => {
});

describe("addGuardian", async () => {
it("Can add a guardian and proper event is emitted", async () => {
let newGuardian: String;

before (async () => {
const {
entryPoint,
userSA,
accountRecoveryModule,
ecdsaModule,
controlMessage,
} = await setupTests();

const messageHash = ethers.utils.id(controlMessage);
const messageHashBytes = ethers.utils.arrayify(messageHash);

const newGuardian = ethers.utils.keccak256(
newGuardian = ethers.utils.keccak256(
await eve.signMessage(messageHashBytes)
);

});

it("Can add a guardian and proper event is emitted", async () => {
const {
entryPoint,
userSA,
accountRecoveryModule,
ecdsaModule,
controlMessage,
} = await setupTests();

const guardiansBefore = (
await accountRecoveryModule.getSmartAccountSettings(userSA.address)
).guardiansCount;
Expand Down Expand Up @@ -2125,13 +2134,83 @@ describe("Account Recovery Module: ", async () => {
expect(guardiansAfter).to.equal(guardiansBefore + 1);
});

/*
it("Should revert if zero guardian is provided", async () => {});

it("Should revert if zero guardian is provided", async () => {
const {
userSA,
accountRecoveryModule,
} = await setupTests();

it("Should revert if such a guardian has already been set", async () => {});
//assign empty bytes32
const zeroGuardian = ethers.utils.hexZeroPad("0x", 32);
const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;

await expect(accountRecoveryModule.addGuardian(
zeroGuardian,
16741936496,
0,
)).to.be.revertedWith("ZeroGuardian");

const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
expect(guardiansCountAfter).to.equal(guardiansCountBefore);
});

it("Should set validUntil to uint48.max if validUntil = 0 is provided", async () => {});
it("Should revert if such a guardian has already been set", async () => {
const {
accountRecoveryModule,
guardians
} = await setupTests();

const guardian = guardians[1];

//add guardian first
await accountRecoveryModule.addGuardian(
guardian,
15555934444,
0,
);

const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;

await expect(accountRecoveryModule.addGuardian(
guardian,
15555934444,
0,
)).to.be.revertedWith("GuardianAlreadySet")
.withArgs(
guardian,
deployer.address
);

const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
expect(guardiansCountAfter).to.equal(guardiansCountBefore);
});

it("Should set validUntil to uint48.max if validUntil = 0 is provided", async () => {
const {
accountRecoveryModule,
} = await setupTests();

const validUntil = 0;
const validAfter = 0;

//add guardian first
await accountRecoveryModule.addGuardian(
newGuardian,
validUntil,
validAfter,
);

const newGuardianTimeFrame = await accountRecoveryModule.getGuardianParams(
newGuardian,
deployer.address
);

expect(newGuardianTimeFrame.validUntil).to.equal(2**48-1);
});


/*
it("Should set validAfter as guardian.timeframe.validAfter if it is bigger than now+securityDelay", async () => {});
it("Should set now+securityDelay if validAfter is less than it", async () => {});
Expand Down

0 comments on commit 6753638

Please sign in to comment.