Skip to content

Commit

Permalink
Refactor contracts and interfaces for cleaner code and optimize gas u…
Browse files Browse the repository at this point in the history
…sage by removing unnecessary whitespace and redundant code in Solidity files
  • Loading branch information
ylv-io committed Jan 29, 2024
1 parent 827f6f4 commit ccf1439
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 215 deletions.
33 changes: 11 additions & 22 deletions src/access/AccessPoint.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,7 @@ import "../interfaces/IAccessRegistry.sol";
/**
* @title AccessPoint
*/
contract AccessPoint is
IAccessPoint,
Initializable,
BaseAccount,
TokenCallbackHandler
{
contract AccessPoint is IAccessPoint, Initializable, BaseAccount, TokenCallbackHandler {
using UserOperationLib for UserOperation;
using ECDSA for bytes32;

Expand Down Expand Up @@ -69,13 +64,7 @@ contract AccessPoint is

/* ============ View Functions ============ */

function getNonce()
public
view
virtual
override(BaseAccount, IAccessPoint)
returns (uint256)
{
function getNonce() public view virtual override(BaseAccount, IAccessPoint) returns (uint256) {
return super.getNonce();
}

Expand Down Expand Up @@ -112,10 +101,7 @@ contract AccessPoint is

/// @notice Executes a DELEGATECALL to the provided target with the provided data.
/// @dev Shared logic between the constructor and the `execute` function.
function _execute(address target, bytes memory data)
internal
returns (bytes memory response)
{
function _execute(address target, bytes memory data) internal returns (bytes memory response) {
if (!registry.isWorkflowAllowed(target)) {
revert WorkflowUnauthorized(target);

Check warning on line 106 in src/access/AccessPoint.sol

View check run for this annotation

Codecov / codecov/patch

src/access/AccessPoint.sol#L106

Added line #L106 was not covered by tests
}
Expand Down Expand Up @@ -147,13 +133,16 @@ contract AccessPoint is
}

/// @notice Valides that owner signed the user operation
function _validateSignature(
UserOperation calldata userOp,
bytes32 userOpHash
) internal virtual override returns (uint256 validationData) {
function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash)
internal
virtual
override
returns (uint256 validationData)
{
bytes32 hash = userOpHash.toEthSignedMessageHash();
if (owner != ECDSA.recover(hash, userOp.signature))
if (owner != ECDSA.recover(hash, userOp.signature)) {
return SIG_VALIDATION_FAILED;

Check warning on line 144 in src/access/AccessPoint.sol

View check run for this annotation

Codecov / codecov/patch

src/access/AccessPoint.sol#L144

Added line #L144 was not covered by tests
}
return 0;
}
}
69 changes: 16 additions & 53 deletions src/access/AccessRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@ import "../proxy/SafeBeaconProxy.sol";

import "../interfaces/IAccessRegistry.sol";

contract AccessRegistry is
Initializable,
UUPSUpgradeable,
OwnableUpgradeable,
IAccessRegistry
{
contract AccessRegistry is Initializable, UUPSUpgradeable, OwnableUpgradeable, IAccessRegistry {
/* ============ Constants ============ */

/* ============ State Variables ============ */
Expand Down Expand Up @@ -75,16 +70,9 @@ contract AccessRegistry is
* @param newImpl The new implementation
*/
function upgradeAll(IAccessPoint newImpl) external override onlyOwner {
require(
address(newImpl) != address(0) &&
address(newImpl) != beacon.implementation(),
"invalid address"
);
require(address(newImpl) != address(0) && address(newImpl) != beacon.implementation(), "invalid address");
factoryVersion++;
emit AccessPointFactoryUpgraded(
beacon.implementation(),
address(newImpl)
);
emit AccessPointFactoryUpgraded(beacon.implementation(), address(newImpl));
beacon.upgradeTo(address(newImpl));

Check warning on line 76 in src/access/AccessRegistry.sol

View check run for this annotation

Codecov / codecov/patch

src/access/AccessRegistry.sol#L74-L76

Added lines #L74 - L76 were not covered by tests
}

Expand All @@ -93,12 +81,7 @@ contract AccessRegistry is
* @param newImplementation address of the new implementation
*/
// This function is called by the proxy contract when the factory is upgraded
function _authorizeUpgrade(address newImplementation)
internal
view
override
onlyOwner
{
function _authorizeUpgrade(address newImplementation) internal view override onlyOwner {
(newImplementation);
}

Expand All @@ -110,29 +93,21 @@ contract AccessRegistry is
}

/// @inheritdoc IAccessRegistry
function getAccessPoint(address user)
external
view
returns (IAccessPoint accessPoint)
{
function getAccessPoint(address user) external view returns (IAccessPoint accessPoint) {
accessPoint = _accessPoints[user];

Check warning on line 97 in src/access/AccessRegistry.sol

View check run for this annotation

Codecov / codecov/patch

src/access/AccessRegistry.sol#L97

Added line #L97 was not covered by tests
}

/// @inheritdoc IAccessRegistry
function getAddress(address owner) public view override returns (address) {
return
Create2.computeAddress(
bytes32(abi.encodePacked(owner)),
keccak256(
abi.encodePacked(
type(SafeBeaconProxy).creationCode,
abi.encode(
address(beacon),
abi.encodeCall(IAccessPoint.initialize, (owner))
)
)
return Create2.computeAddress(

Check warning on line 102 in src/access/AccessRegistry.sol

View check run for this annotation

Codecov / codecov/patch

src/access/AccessRegistry.sol#L102

Added line #L102 was not covered by tests
bytes32(abi.encodePacked(owner)),
keccak256(
abi.encodePacked(
type(SafeBeaconProxy).creationCode,
abi.encode(address(beacon), abi.encodeCall(IAccessPoint.initialize, (owner)))
)
);
)
);
}

/// @dev TODO:Should be removed. Added because Kinto EntryPoint needs this function.
Expand Down Expand Up @@ -167,31 +142,19 @@ contract AccessRegistry is
/* ============ Internal ============ */

/// @dev See `deploy`.
function _deploy(address owner)
internal
returns (IAccessPoint accessPoint)
{
function _deploy(address owner) internal returns (IAccessPoint accessPoint) {
// Use the address of the owner as the CREATE2 salt.
bytes32 salt = bytes32(abi.encodePacked(owner));

// Deploy the accessPoint with CREATE2.
accessPoint = IAccessPoint(
payable(
new SafeBeaconProxy{salt: salt}(
address(beacon),
abi.encodeCall(IAccessPoint.initialize, (owner))
)
)
payable(new SafeBeaconProxy{salt: salt}(address(beacon), abi.encodeCall(IAccessPoint.initialize, (owner))))
);

// Associate the owner and the accessPoint.
_accessPoints[owner] = accessPoint;

// Log the creation of the accessPoint.
emit DeployAccessPoint({
operator: msg.sender,
owner: owner,
accessPoint: accessPoint
});
emit DeployAccessPoint({operator: msg.sender, owner: owner, accessPoint: accessPoint});
}
}
2 changes: 1 addition & 1 deletion src/access/workflows/WithdrawWorkflow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ contract WithdrawWorkflow {

function withdrawNative(uint256 amount) external {
address owner = _getOwner();
(bool sent, ) = owner.call{value: amount}("");
(bool sent,) = owner.call{value: amount}("");
if (!sent) {
revert NativeWithdrawalFailed();

Check warning on line 29 in src/access/workflows/WithdrawWorkflow.sol

View check run for this annotation

Codecov / codecov/patch

src/access/workflows/WithdrawWorkflow.sol#L29

Added line #L29 was not covered by tests
}
Expand Down
5 changes: 1 addition & 4 deletions src/interfaces/IAccessPoint.sol
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,5 @@ interface IAccessPoint {
/// @param target The address of the target contract.
/// @param data Function selector plus ABI encoded data.
/// @return response The response received from the target contract, if any.
function execute(address target, bytes calldata data)
external
payable
returns (bytes memory response);
function execute(address target, bytes calldata data) external payable returns (bytes memory response);
}
25 changes: 5 additions & 20 deletions src/interfaces/IAccessRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,13 @@ interface IAccessRegistry {
/* ============ Events ============ */

/// @notice Emitted when workflow is allowed or dissallowed.
event WorkflowStatusChanged(
address indexed workflow,
bool indexed status
);
event WorkflowStatusChanged(address indexed workflow, bool indexed status);

/// @notice Emitted when access point is upgraded.
event AccessPointFactoryUpgraded(
address indexed beacon,
address indexed accessPoint
);
event AccessPointFactoryUpgraded(address indexed beacon, address indexed accessPoint);

/// @notice Emitted when a new access point is deployed.
event DeployAccessPoint(
address indexed operator,
address indexed owner,
IAccessPoint accessPoint
);
event DeployAccessPoint(address indexed operator, address indexed owner, IAccessPoint accessPoint);

/* ============ Structs ============ */

Expand All @@ -49,10 +39,7 @@ interface IAccessRegistry {

/// @notice Retrieves the accessPoint for the provided user.
/// @param user The user address for the query.
function getAccessPoint(address user)
external
view
returns (IAccessPoint accessPoint);
function getAccessPoint(address user) external view returns (IAccessPoint accessPoint);

/**
* @dev Calculates the counterfactual address of this account as it would be returned by deploy()
Expand Down Expand Up @@ -84,7 +71,5 @@ interface IAccessRegistry {
///
/// @param user The address that will own the access point.
/// @return accessPoint The address of the newly deployed access point.
function deployFor(address user)
external
returns (IAccessPoint accessPoint);
function deployFor(address user) external returns (IAccessPoint accessPoint);
}
75 changes: 24 additions & 51 deletions src/paymasters/SignaturePaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ contract SignaturePaymaster is BasePaymaster, Initializable, UUPSUpgradeable {

// ========== Constructor & Upgrades ============

constructor(IEntryPoint _entryPoint, address _verifyingSigner)
BasePaymaster(_entryPoint)
{
constructor(IEntryPoint _entryPoint, address _verifyingSigner) BasePaymaster(_entryPoint) {
_disableInitializers();
verifyingSigner = _verifyingSigner;
}
Expand All @@ -53,22 +51,14 @@ contract SignaturePaymaster is BasePaymaster, Initializable, UUPSUpgradeable {
* @param newImplementation address of the new implementation
*/
// This function is called by the proxy contract when the implementation is upgraded
function _authorizeUpgrade(address newImplementation)
internal
view
override
{
function _authorizeUpgrade(address newImplementation) internal view override {
require(msg.sender == owner(), "VP: not owner");
(newImplementation);
}

/* =============== Viewers & Validation ============= */

function pack(UserOperation calldata userOp)
internal
pure
returns (bytes memory ret)
{
function pack(UserOperation calldata userOp) internal pure returns (bytes memory ret) {
// lighter signature scheme. must match UserOp.ts#packUserOp
bytes calldata pnd = userOp.paymasterAndData;
// copy directly the userOp from calldata up to (but not including) the paymasterAndData.
Expand All @@ -92,24 +82,18 @@ contract SignaturePaymaster is BasePaymaster, Initializable, UUPSUpgradeable {
* note that this signature covers all fields of the UserOperation, except the "paymasterAndData",
* which will carry the signature itself.
*/
function getHash(
UserOperation calldata userOp,
uint48 validUntil,
uint48 validAfter
) public view returns (bytes32) {
function getHash(UserOperation calldata userOp, uint48 validUntil, uint48 validAfter)
public
view
returns (bytes32)
{
//can't use userOp.hash(), since it contains also the paymasterAndData itself.

return
keccak256(
abi.encode(
pack(userOp),
block.chainid,
address(this),
senderNonce[userOp.getSender()],
validUntil,
validAfter
)
);
return keccak256(
abi.encode(
pack(userOp), block.chainid, address(this), senderNonce[userOp.getSender()], validUntil, validAfter
)
);
}

/**
Expand All @@ -120,27 +104,22 @@ contract SignaturePaymaster is BasePaymaster, Initializable, UUPSUpgradeable {
* paymasterAndData[20:84] : abi.encode(validUntil, validAfter)
* paymasterAndData[84:] : signature
*/
function _validatePaymasterUserOp(
UserOperation calldata userOp,
bytes32, /*userOpHash*/
uint256 requiredPreFund
) internal override returns (bytes memory context, uint256 validationData) {
function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32, /*userOpHash*/ uint256 requiredPreFund)
internal
override
returns (bytes memory context, uint256 validationData)
{
(requiredPreFund);

(
uint48 validUntil,
uint48 validAfter,
bytes calldata signature
) = parsePaymasterAndData(userOp.paymasterAndData);
(uint48 validUntil, uint48 validAfter, bytes calldata signature) =
parsePaymasterAndData(userOp.paymasterAndData);
//ECDSA library supports both 64 and 65-byte long signatures.
// we only "require" it here so that the revert reason on invalid signature will be of "SignaturePaymaster", and not "ECDSA"
require(
signature.length == 64 || signature.length == 65,
"SignaturePaymaster: invalid signature length in paymasterAndData"
);
bytes32 hash = ECDSA.toEthSignedMessageHash(
getHash(userOp, validUntil, validAfter)
);
bytes32 hash = ECDSA.toEthSignedMessageHash(getHash(userOp, validUntil, validAfter));
senderNonce[userOp.getSender()]++;

//don't revert on signature failure: return SIG_VALIDATION_FAILED
Expand All @@ -156,16 +135,10 @@ contract SignaturePaymaster is BasePaymaster, Initializable, UUPSUpgradeable {
function parsePaymasterAndData(bytes calldata paymasterAndData)
public
pure
returns (
uint48 validUntil,
uint48 validAfter,
bytes calldata signature
)
returns (uint48 validUntil, uint48 validAfter, bytes calldata signature)
{
(validUntil, validAfter) = abi.decode(
paymasterAndData[VALID_TIMESTAMP_OFFSET:SIGNATURE_OFFSET],
(uint48, uint48)
);
(validUntil, validAfter) =
abi.decode(paymasterAndData[VALID_TIMESTAMP_OFFSET:SIGNATURE_OFFSET], (uint48, uint48));
signature = paymasterAndData[SIGNATURE_OFFSET:];
}
}
Loading

0 comments on commit ccf1439

Please sign in to comment.