Skip to content

Commit

Permalink
✅ linting and typo
Browse files Browse the repository at this point in the history
  • Loading branch information
Filipp Makarov authored and Filipp Makarov committed Oct 15, 2023
1 parent e8c0729 commit fcc266d
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 62 deletions.
80 changes: 45 additions & 35 deletions contracts/smart-account/modules/AccountRecoveryModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
/**
* @title Account Recovery module for Biconomy Smart Accounts.
* @dev Compatible with Biconomy Modular Interface v 0.1
* - It allows to submit and execute recovery requests
* - It allows the submission and execution of recovery requests
* - EOA guardians only
* - For security reasons guardian address is not stored,
* instead its signature over CONTROL_HASH is used
* - For security reasons the guardian's address is not stored,
* instead, its signature over CONTROL_HASH is used
* - Security delay is always applied to new guardians and recovery requests
* - It is highly recommended to not set security delay to 0
* @dev For the validation stage (validateUserOp) can not use custom errors
Expand Down Expand Up @@ -77,7 +77,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
TimeFrame timeFrame
);
event ThresholdChanged(address indexed smartAccount, uint8 threshold);
event SecurityDelayChanged(address indexed smartAccount, uint48 securityDelay);
event SecurityDelayChanged(
address indexed smartAccount,
uint48 securityDelay
);

error AlreadyInitedForSmartAccount(address smartAccount);
error ZeroGuardian();
Expand Down Expand Up @@ -152,8 +155,8 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
* @dev validates userOps to submut and execute recovery requests
* - if securityDelay is 0, it allows to execute the request immediately
* - if securityDelay is non 0, the request is submitted and stored on-chain
* - if userOp.callData matches the callData of the request already submitted,
* - and the security delays has passed, it allows to execute the request
* - if userOp.callData matches the callData of the request already submitted,
* - and the security delay has passed, it allows to execute the request
* @param userOp User Operation to be validated.
* @param userOpHash Hash of the User Operation to be validated.
* @return validation data (sig validation result + validUntil + validAfter)
Expand Down Expand Up @@ -223,7 +226,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
userOp.sender
].validUntil;

// validUntil == 0 means the `currentGuardian` has not been set as guardian
// validUntil == 0 means the `currentGuardian` has not been set as guardian
// for the userOp.sender smartAccount
// validUntil can never be 0 as it is set to type(uint48).max in initForSmartAccount
if (validUntil == 0) {
Expand Down Expand Up @@ -363,7 +366,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
/**
* @dev Removes guardian for a Smart Account (msg.sender)
* Should be called by the Smart Account
* @param guardian guardian to remove
* @param guardian guardian to remove
*/
function removeGuardian(bytes32 guardian) external {
if (_guardians[guardian][msg.sender].validUntil == 0)
Expand All @@ -373,9 +376,9 @@ contract AccountRecoveryModule is BaseAuthorizationModule {

/**
* @dev Removes the expired guardian for a Smart Account
* Can be called by anyone. Allows clearing expired guardians automatically
* Can be called by anyone. Allows clearing expired guardians automatically
* and maintain the list of guardians actual
* @param guardian guardian to remove
* @param guardian guardian to remove
*/
function removeExpiredGuardian(
bytes32 guardian,
Expand All @@ -388,7 +391,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
_removeGuardianAndChangeTresholdIfNeeded(guardian, smartAccount);
}

/**
/**
* @dev Internal method to check and adjust validUntil and validAfter
* @dev if validUntil is 0, guardian is considered active forever
* Thus we put type(uint48).max as value for validUntil in this case,
Expand All @@ -403,7 +406,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
* thus we do not need to check than validUntil is gte now
* @param validUntil guardian validity end timestamp
* @param validAfter guardian validity start timestamp
*/
*/
function _checkAndAdjustValidUntilValidAfter(
uint48 validUntil,
uint48 validAfter
Expand All @@ -420,13 +423,13 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
return (validUntil, validAfter);
}

/**
/**
* @dev Internal method to remove guardian and adjust threshold if needed
* It is needed when after removing guardian, the threshold becomes higher than
* the number of guardians
* @param guardian guardian to remove
* @param guardian guardian to remove
* @param smartAccount smartAccount to remove guardian from
*/
*/
function _removeGuardianAndChangeTresholdIfNeeded(
bytes32 guardian,
address smartAccount
Expand All @@ -447,27 +450,34 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
}
}

/**
/**
* @dev Changes guardian validity timeframes for the Smart Account (msg.sender)
* @param guardian guardian to change
* @param guardian guardian to change
* @param validUntil guardian validity end timestamp
* @param validAfter guardian validity start timestamp
*/
*/
function changeGuardianParams(
bytes32 guardian,
uint48 validUntil,
uint48 validAfter
) external {
(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(validUntil, validAfter);
(validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(
validUntil,
validAfter
);
_guardians[guardian][msg.sender] = TimeFrame(validUntil, validAfter);
emit GuardianChanged(msg.sender, guardian, TimeFrame(validUntil, validAfter));
emit GuardianChanged(
msg.sender,
guardian,
TimeFrame(validUntil, validAfter)
);
}

/**
* @dev Changes recovery threshold for a Smart Account (msg.sender)
* Should be called by the Smart Account
* @param newThreshold new recovery threshold
*/
*/
function setThreshold(uint8 newThreshold) external {
if (newThreshold == 0) revert ZeroThreshold();
if (newThreshold > _smartAccountSettings[msg.sender].guardiansCount)
Expand All @@ -483,30 +493,30 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
* @dev Changes security delay for a Smart Account (msg.sender)
* Should be called by the Smart Account
* @param newSecurityDelay new security delay
*/
*/
function setSecurityDelay(uint48 newSecurityDelay) external {
_smartAccountSettings[msg.sender].securityDelay = newSecurityDelay;
emit SecurityDelayChanged(msg.sender, newSecurityDelay);
}

/**
* @dev Returns guardian validity timeframes for the Smart Account
* @param guardian guardian to get params for
* @param smartAccount smartAccount to get params for
* @return TimeFrame struct
*/
/**
* @dev Returns guardian validity timeframes for the Smart Account
* @param guardian guardian to get params for
* @param smartAccount smartAccount to get params for
* @return TimeFrame struct
*/
function getGuardianParams(
bytes32 guardian,
address smartAccount
) external view returns (TimeFrame memory) {
return _guardians[guardian][smartAccount];
}

/**
* @dev Returns Smart Account settings
* @param smartAccount smartAccount to get settings for
* @return Smart Account Settings struct
*/
/**
* @dev Returns Smart Account settings
* @param smartAccount smartAccount to get settings for
* @return Smart Account Settings struct
*/
function getSmartAccountSettings(
address smartAccount
) external view returns (SaSettings memory) {
Expand All @@ -518,7 +528,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
* Only one request per Smart Account is stored at a time
* @param smartAccount smartAccount to get recovery request for
* @return RecoveryRequest struct
*/
*/
function getRecoveryRequest(
address smartAccount
) external view returns (RecoveryRequest memory) {
Expand All @@ -529,7 +539,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
* @dev Submits recovery request for a Smart Account
* Hash of the callData is stored on-chain along with the timestamp of the request submission
* @param recoveryCallData callData of the recovery request
*/
*/
function submitRecoveryRequest(bytes calldata recoveryCallData) public {
if (recoveryCallData.length == 0) revert EmptyRecoveryCallData();
if (
Expand Down
13 changes: 4 additions & 9 deletions test/bundler-integration/module/AccountRecovery.Module.specs.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { expect, use } from "chai";
import { ethers, deployments, waffle } from "hardhat";
import {
makeEcdsaModuleUserOp,
makeUnsignedUserOp,
} from "../../utils/userOp";
import {
makeMultiSignedUserOpWithGuardiansList,
} from "../../utils/accountRecovery";
import { expect } from "chai";
import { ethers, deployments } from "hardhat";
import { makeEcdsaModuleUserOp, makeUnsignedUserOp } from "../../utils/userOp";
import { makeMultiSignedUserOpWithGuardiansList } from "../../utils/accountRecovery";
import {
getEntryPoint,
getSmartAccountImplementation,
Expand Down
56 changes: 38 additions & 18 deletions test/module/AccountRecovery.Module.specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
makeMultisignedSubmitRecoveryRequestUserOp,
} from "../utils/accountRecovery";
import { arrayify } from "ethers/lib/utils";
import {Contract} from "ethers";
import { Contract } from "ethers";

describe("Account Recovery Module: ", async () => {
const [
Expand Down Expand Up @@ -1599,7 +1599,7 @@ describe("Account Recovery Module: ", async () => {
"changeGuardianParams",
[
guardians[1],
17641936496,
17641936496,
closestValidUntil + 1000, // validAfter for this guardian is after previous one expires
]
),
Expand Down Expand Up @@ -2595,7 +2595,8 @@ describe("Account Recovery Module: ", async () => {

describe("changeGuardianParams", async () => {
it("Can change Guardian params and event is emitted", async () => {
const { accountRecoveryModule, guardians, defaultSecurityDelay } = await setupTests();
const { accountRecoveryModule, guardians, defaultSecurityDelay } =
await setupTests();

const validUntilBefore = 16741936496;
const validAfterBefore = 0;
Expand All @@ -2610,8 +2611,14 @@ describe("Account Recovery Module: ", async () => {
const validAfterAfter = validAfterBefore + 100;

// by some reason blockchain timestamp for the txn is 1 sec higher
const nowPlusSecurityDelay = defaultSecurityDelay + 1 + (await ethers.provider.getBlock("latest")).timestamp;
const expectedOnchainValidAfter = validAfterAfter > nowPlusSecurityDelay ? validAfterAfter : nowPlusSecurityDelay;
const nowPlusSecurityDelay =
defaultSecurityDelay +
1 +
(await ethers.provider.getBlock("latest")).timestamp;
const expectedOnchainValidAfter =
validAfterAfter > nowPlusSecurityDelay
? validAfterAfter
: nowPlusSecurityDelay;

const newTimeFrame = {
validUntil: validUntilAfter,
Expand Down Expand Up @@ -2639,7 +2646,6 @@ describe("Account Recovery Module: ", async () => {
});
});


describe("setThreshold", async () => {
let accountRecoveryModuleWithGuardiansForDeployer: Contract;
before(async () => {
Expand All @@ -2653,17 +2659,27 @@ describe("Account Recovery Module: ", async () => {

it("Can set a new threshold", async () => {
const currentThreshold = (
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address)
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(
deployer.address
)
).recoveryThreshold;
const newThreshold = currentThreshold - 1;
const setThresholdTxn = await accountRecoveryModuleWithGuardiansForDeployer.setThreshold(newThreshold);
const setThresholdTxn =
await accountRecoveryModuleWithGuardiansForDeployer.setThreshold(
newThreshold
);
expect(setThresholdTxn)
.to.emit(accountRecoveryModuleWithGuardiansForDeployer, "ThresholdChanged")
.to.emit(
accountRecoveryModuleWithGuardiansForDeployer,
"ThresholdChanged"
)
.withArgs(deployer.address, newThreshold);
const newThresholdAfter = (
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address)
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(
deployer.address
)
).recoveryThreshold;
expect(newThresholdAfter).to.equal(newThreshold);
expect(newThresholdAfter).to.equal(newThreshold);
});

it("Reverts for zero threshold", async () => {
Expand All @@ -2674,21 +2690,27 @@ describe("Account Recovery Module: ", async () => {

it("Reverts if threshold is more than number of guardians", async () => {
const currentNumberOfGuardians = (
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(deployer.address)
await accountRecoveryModuleWithGuardiansForDeployer.getSmartAccountSettings(
deployer.address
)
).guardiansCount;
await expect(
accountRecoveryModuleWithGuardiansForDeployer.setThreshold(currentNumberOfGuardians + 1)
accountRecoveryModuleWithGuardiansForDeployer.setThreshold(
currentNumberOfGuardians + 1
)
).to.be.revertedWith("ThresholdTooHigh");
});
});


describe("setSecurityDelay", async () => {
it("Can set security delay and event is emitted", async () => {
const { accountRecoveryModule, defaultSecurityDelay } = await setupTests();
const { accountRecoveryModule, defaultSecurityDelay } =
await setupTests();

const newSecurityDelay = defaultSecurityDelay + 1000;
const setSecurityDelayTxn = await accountRecoveryModule.setSecurityDelay(newSecurityDelay);
const setSecurityDelayTxn = await accountRecoveryModule.setSecurityDelay(
newSecurityDelay
);
expect(setSecurityDelayTxn)
.to.emit(accountRecoveryModule, "SecurityDelayChanged")
.withArgs(deployer.address, newSecurityDelay);
Expand All @@ -2701,6 +2723,4 @@ describe("Account Recovery Module: ", async () => {

// for the execution stage (all methods not related to validateUserOp) custom errors can be used
// make the according explanation in the smart contract header


});

0 comments on commit fcc266d

Please sign in to comment.