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

small improvements for readability and gas #4

Merged
merged 4 commits into from
Jun 13, 2024
Merged

small improvements for readability and gas #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71fe6c7
feat: use calldata slicing in module
zeroknots Jun 13, 2024
9c7bade
feat: readability
zeroknots Jun 13, 2024
8cc744b
fix: make email_recovery internal since there is an external getter f…
zeroknots Jun 13, 2024
72057f1
feat: minor coding best practice improvement
zeroknots Jun 13, 2024
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
7 changes: 3 additions & 4 deletions src/EmailRecoveryManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,9 @@ contract EmailRecoveryManager is EmailAccountRecoveryNew, IEmailRecoveryManager
address _dkimRegistry,
address _emailAuthImpl,
address _subjectHandler
) {
verifierAddr = _verifier;
dkimAddr = _dkimRegistry;
emailAuthImplementationAddr = _emailAuthImpl;
)
EmailAccountRecoveryNew(_verifier, _dkimRegistry, _emailAuthImpl)
{
subjectHandler = _subjectHandler;
}

Expand Down
12 changes: 9 additions & 3 deletions src/experimental/EmailAccountRecoveryNew.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,15 @@ import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy
/// new guardian and recovering a wallet.
abstract contract EmailAccountRecoveryNew {
uint8 constant EMAIL_ACCOUNT_RECOVERY_VERSION_ID = 1;
address public verifierAddr;
address public dkimAddr;
address public emailAuthImplementationAddr;
address internal immutable verifierAddr;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these have getter functions. so will reduce deployment bytecode size

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, posted this suggestion in zkemail/email-tx-builder#28

address internal immutable dkimAddr;
address internal immutable emailAuthImplementationAddr;

constructor(address _verifierAddr, address _dkimAddr, address _emailAuthImplementationAddr) {
verifierAddr = _verifierAddr;
dkimAddr = _dkimAddr;
emailAuthImplementationAddr = _emailAuthImplementationAddr;
}

/// @notice Returns the address of the verifier contract.
/// @dev This function is virtual and can be overridden by inheriting contracts.
Expand Down
6 changes: 3 additions & 3 deletions src/handlers/EmailRecoverySubjectHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {

function validateAcceptanceSubject(
uint256 templateIdx,
bytes[] memory subjectParams
bytes[] calldata subjectParams
)
external
view
pure
returns (address)
{
if (templateIdx != 0) {
Expand All @@ -70,7 +70,7 @@ contract EmailRecoverySubjectHandler is IEmailRecoverySubjectHandler {

function validateRecoverySubject(
uint256 templateIdx,
bytes[] memory subjectParams,
bytes[] calldata subjectParams,
address recoveryManager
)
public
Expand Down
6 changes: 3 additions & 3 deletions src/handlers/SafeRecoverySubjectHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {

function validateAcceptanceSubject(
uint256 templateIdx,
bytes[] memory subjectParams
bytes[] calldata subjectParams
)
external
view
pure
returns (address)
{
if (templateIdx != 0) {
Expand All @@ -73,7 +73,7 @@ contract SafeRecoverySubjectHandler is IEmailRecoverySubjectHandler {

function validateRecoverySubject(
uint256 templateIdx,
bytes[] memory subjectParams,
bytes[] calldata subjectParams,
address recoveryManager
)
public
Expand Down
71 changes: 0 additions & 71 deletions src/libraries/BytesLib.sol
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was able to axe BytesLib entirely by using calldata slicing. should reduce audit LoC

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah good shout since the calldata isn't stored in memory anymore. This is a good win

This file was deleted.

36 changes: 16 additions & 20 deletions src/modules/EmailRecoveryModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,30 @@ pragma solidity ^0.8.25;
import { ERC7579ExecutorBase } from "@rhinestone/modulekit/src/Modules.sol";
import { IERC7579Account } from "erc7579/interfaces/IERC7579Account.sol";
import { IModule } from "erc7579/interfaces/IERC7579Module.sol";
import { ExecutionLib } from "erc7579/lib/ExecutionLib.sol";
import { ModeLib } from "erc7579/lib/ModeLib.sol";

import { IRecoveryModule } from "../interfaces/IRecoveryModule.sol";
import { IEmailRecoveryManager } from "../interfaces/IEmailRecoveryManager.sol";
import { ISafe } from "../interfaces/ISafe.sol";
import { BytesLib } from "../libraries/BytesLib.sol";

contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
using BytesLib for bytes;
/*//////////////////////////////////////////////////////////////////////////
CONSTANTS
//////////////////////////////////////////////////////////////////////////*/

address public immutable emailRecoveryManager;
address public immutable EMAIL_RECOVERY_MANAGER;

event NewValidatorRecovery(address indexed validatorModule, bytes4 recoverySelector);

error NotTrustedRecoveryManager();
error InvalidSubjectParams();
error InvalidValidator(address validator);
error InvalidSelector(bytes4 selector);
error InvalidOnInstallData();

mapping(address validatorModule => mapping(address account => bytes4 allowedSelector)) internal
allowedSelectors;
mapping(address account => address validator) internal validators;

constructor(address _zkEmailRecovery) {
emailRecoveryManager = _zkEmailRecovery;
EMAIL_RECOVERY_MANAGER = _zkEmailRecovery;
}

modifier withoutUnsafeSelector(bytes4 recoverySelector) {
Expand All @@ -55,6 +50,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
* @param data The data to initialize the module with
*/
function onInstall(bytes calldata data) external {
if (data.length == 0) revert InvalidOnInstallData();
(
address validator,
bytes4 selector,
Expand All @@ -65,11 +61,11 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
uint256 expiry
) = abi.decode(data, (address, bytes4, address[], uint256[], uint256, uint256, uint256));

allowValidatorRecovery(validator, bytes("0"), selector);
_allowValidatorRecovery(validator, bytes("0"), selector);
validators[msg.sender] = validator;

_execute({
to: emailRecoveryManager,
to: EMAIL_RECOVERY_MANAGER,
value: 0,
data: abi.encodeCall(
IEmailRecoveryManager.configureRecovery,
Expand All @@ -78,7 +74,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
});
}

function allowValidatorRecovery(
function _allowValidatorRecovery(
address validator,
bytes memory isInstalledContext,
bytes4 recoverySelector
Expand Down Expand Up @@ -107,7 +103,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
address validator = validators[msg.sender];
delete allowedSelectors[validator][msg.sender];
delete validators[msg.sender];
IEmailRecoveryManager(emailRecoveryManager).deInitRecoveryFromModule(msg.sender);
IEmailRecoveryManager(EMAIL_RECOVERY_MANAGER).deInitRecoveryFromModule(msg.sender);
}

/**
Expand All @@ -116,32 +112,32 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
* @return true if the module is initialized, false otherwise
*/
function isInitialized(address smartAccount) external view returns (bool) {
return IEmailRecoveryManager(emailRecoveryManager).getGuardianConfig(smartAccount).threshold
!= 0;
return IEmailRecoveryManager(EMAIL_RECOVERY_MANAGER).getGuardianConfig(smartAccount)
.threshold != 0;
}

/*//////////////////////////////////////////////////////////////////////////
MODULE LOGIC
//////////////////////////////////////////////////////////////////////////*/

function recover(address account, bytes memory recoveryCalldata) external {
if (msg.sender != emailRecoveryManager) {
function recover(address account, bytes calldata recoveryCalldata) external {
if (msg.sender != EMAIL_RECOVERY_MANAGER) {
revert NotTrustedRecoveryManager();
}

bytes4 selector = bytes4(recoveryCalldata.slice({ _start: 0, _length: 4 }));
bytes4 selector = bytes4(recoveryCalldata[:4]);

address validator = validators[account];
bytes4 allowedSelector = allowedSelectors[validator][account];
if (allowedSelector != selector) {
revert InvalidSelector(selector);
}

_execute({ account: account, to: validators[account], value: 0, data: recoveryCalldata });
_execute({ account: account, to: validator, value: 0, data: recoveryCalldata });
}

function getTrustedRecoveryManager() external view returns (address) {
return emailRecoveryManager;
return EMAIL_RECOVERY_MANAGER;
}

/*//////////////////////////////////////////////////////////////////////////
Expand All @@ -153,7 +149,7 @@ contract EmailRecoveryModule is ERC7579ExecutorBase, IRecoveryModule {
* @return name The name of the module
*/
function name() external pure returns (string memory) {
return "EmailRecoveryModule";
return "ZKEmail.EmailRecoveryModule";
}

/**
Expand Down
Loading