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.

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

Version 0 Review PR #29

Open
wants to merge 3 commits into
base: Version-0-Review
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
240 changes: 240 additions & 0 deletions src/TheCompact.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;

import { ITheCompact } from "./interfaces/ITheCompact.sol";

import { BatchTransfer, SplitBatchTransfer } from "./types/BatchClaims.sol";
import { BasicTransfer, SplitTransfer } from "./types/Claims.sol";
import { CompactCategory } from "./types/CompactCategory.sol";
import { Lock } from "./types/Lock.sol";
import { Scope } from "./types/Scope.sol";
import { ResetPeriod } from "./types/ResetPeriod.sol";
import { ForcedWithdrawalStatus } from "./types/ForcedWithdrawalStatus.sol";

import { TheCompactLogic } from "./lib/TheCompactLogic.sol";

import { ERC6909 } from "solady/tokens/ERC6909.sol";
import { ISignatureTransfer } from "permit2/src/interfaces/ISignatureTransfer.sol";

/**
* @title The Compact
* @custom:version 0 (early-stage proof-of-concept)
* @author 0age (0age.eth)
* @notice The Compact is an ownerless ERC6909 contract that facilitates the voluntary
* formation and mediation of reusable "resource locks."
* This contract has not yet been properly tested, audited, or reviewed.
*/
contract TheCompact is ITheCompact, ERC6909, TheCompactLogic {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets notify readers of the NatSpec for the functions in the interface

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah an InheritDoc declaration is a good call to signal that to both humans and machines

function deposit(address allocator) external payable returns (uint256) {
return _performBasicNativeTokenDeposit(allocator);
}

function depositAndRegister(address allocator, bytes32 claimHash, bytes32 typehash) external payable returns (uint256 id) {
id = _performBasicNativeTokenDeposit(allocator);

_registerWithDefaults(claimHash, typehash);
}

function deposit(address token, address allocator, uint256 amount) external returns (uint256) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also be used for native tokens to reduce numbers of functions.
amount could be used to verify the intended amount of native tokens to be deposited

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does seem nice to have a very simple interface for depositing native tokens where we can infer the amount from msg.value, but yes this is an option!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually agree, the reason why I am hesitant in this case is because we already have this big amount of deposit functions. For the sake of keeping the interface for the compact as simple as possible, I feel like this could be one of the ways to do so easily. Very much dependent on the amount of deposit functions in the final product of course.

return _performBasicERC20Deposit(token, allocator, amount);
}

function depositAndRegister(address token, address allocator, uint256 amount, bytes32 claimHash, bytes32 typehash) external returns (uint256 id) {
id = _performBasicERC20Deposit(token, allocator, amount);

_registerWithDefaults(claimHash, typehash);
}

function deposit(address allocator, ResetPeriod resetPeriod, Scope scope, address recipient) external payable returns (uint256) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

General thoughts on the deposit functions:

  • Having the recipient as a parameter (as we do here) allows us to potentially outsource depositAndRegister functions to helper contracts.
  • The contract feels a little like uniswap core and periphery contracts were combined in one, since it offers so much. Could be interesting to explore where to separate logic to keep it easy to read and modular at the same time.
  • In the case of splitting the logic, we could use a callback for the router contract to send in the tokens (like in uniswap V3), to save gas by skipping approval for this contract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the issue with this is that the caller can't (safely) register on behalf of the recipient; effectively since the only valid sponsor is msg.sender unless they sign for it like with permit2, you're not able to spend the deposited tokens as part of the registration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that is indeed an issue.

But I wonder if we should think about this in a different way:
In theory, the sponsor (whoever is paying for the deposit) could be trusted to also be able to register what he has sponsored for directly. The receiver (while via a router also the original sponsor), needs to trust the router with his tokens to do the right thing anyway (depositing it), so it might be fair enough to fully trust the sponsor and see the recipient as a 'receiver' of the finished deposit (and maybe already registered) token deposit.

Indeed it gives the sponsor full responsibility with the process of registering. But he is trusted with full control over the users tokens in the first place anyway.

return _performCustomNativeTokenDeposit(allocator, resetPeriod, scope, recipient);
}

function deposit(address token, address allocator, ResetPeriod resetPeriod, Scope scope, uint256 amount, address recipient) external returns (uint256) {
return _performCustomERC20Deposit(token, allocator, resetPeriod, scope, amount, recipient);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could eliminate a lot of the functions in the deposit contracts by handling the default parameters in this contract. When first reading through it, it felt like this would also increase readability, because it would be easy to see, that the deposit function is the same as before, just with real input params, instead of the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah we should prototype a router contract and compare gas usage! big kicker is persisting the depositor as "authing" the deposit; one big simplification would be to only have the Permit2 deposit methods for tokens which would let you use a router at will but of course that implies a hard dependency on Permit2 which may not be ideal in all cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand what you mean, but I think this was a misunderstanding of this specific comment:
In this case I was more speaking about moving the default data, like the default scope and default resetPeriod into this contract, rather then having it applied in the DirectDepositLogic.sol.
This would mean we could use the same internal deposit function in DirectDepositLogic.sol for both external deposit functions which I think would increase readability.

}

function deposit(uint256[2][] calldata idsAndAmounts, address recipient) external payable returns (bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In here, we actually combine native and ERC20 tokens in the same function, so I feel like we can do the same in previous functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the idea here is that you might want to simultaneously perform a native token + erc20 token deposit, but on the single case it's always one or the other

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand where the decision is coming from, I was more focused on the part of having the msg.value being verified via the amount input as a backup to confirm the sponsors deposit decision.

_processBatchDeposit(idsAndAmounts, recipient);

return true;
}

function depositAndRegister(uint256[2][] calldata idsAndAmounts, bytes32[2][] calldata claimHashesAndTypehashes, uint256 duration) external payable returns (bool) {
_processBatchDeposit(idsAndAmounts, msg.sender);

return _registerBatch(claimHashesAndTypehashes, duration);
}

function deposit(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to rename this function to depositViaPermit2 for a distinct separation to the direct deposit functions.

address token,
uint256, // amount
uint256, // nonce
uint256, // deadline
address, // depositor
address, // allocator
ResetPeriod, // resetPeriod
Scope, //scope
address recipient,
bytes calldata signature
) external returns (uint256) {
return _depositViaPermit2(token, recipient, signature);
}

function depositAndRegister(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to rename this function to depositAndRegisterViaPermit2 (or similar) for a distinct separation to the other deposit functions.

address token,
uint256, // amount
uint256, // nonce
uint256, // deadline
address depositor, // also recipient
address, // allocator
ResetPeriod resetPeriod,
Scope, //scope
bytes32 claimHash,
CompactCategory compactCategory,
string calldata witness,
bytes calldata signature
) external returns (uint256) {
return _depositAndRegisterViaPermit2(token, depositor, resetPeriod, claimHash, compactCategory, witness, signature);
}

function deposit(
address, // depositor
ISignatureTransfer.TokenPermissions[] calldata permitted,
uint256, // nonce
uint256, // deadline
address, // allocator
ResetPeriod, // resetPeriod
Scope, //scope
address recipient,
bytes calldata signature
) external payable returns (uint256[] memory) {
return _depositBatchViaPermit2(permitted, recipient, signature);
}

function depositAndRegister(
address depositor,
ISignatureTransfer.TokenPermissions[] calldata permitted,
uint256, // nonce
uint256, // deadline
address, // allocator
ResetPeriod resetPeriod,
Scope, //scope
bytes32 claimHash,
CompactCategory compactCategory,
string calldata witness,
bytes calldata signature
) external payable returns (uint256[] memory) {
return _depositBatchAndRegisterViaPermit2(depositor, permitted, resetPeriod, claimHash, compactCategory, witness, signature);
}

function allocatedTransfer(BasicTransfer calldata transfer) external returns (bool) {
Copy link
Collaborator

@vimageDE vimageDE Nov 25, 2024

Choose a reason for hiding this comment

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

The functions allocatedTransfer as well as allocatedWithdrawal always requires the msg.sender to be the sponsor. Why do we not add a from address as an input and make use of the granular approval system of ERC 6909?
So either the msg.sender is the sponsor directly, or the msg.sender has previously received a sufficient token approval from the sponsor. This would provide flexibility and enable a lot of use cases with the ERC6909 tokens, while approval would also be a well recognized and still simple system to handle this.

The signature / ERC 1271 approval of the allocator would of course still be required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

left a note in "Path To V1" section of the README to explore this idea: a5f6b0a

return _processBasicTransfer(transfer, _release);
}

function allocatedWithdrawal(BasicTransfer calldata withdrawal) external returns (bool) {
return _processBasicTransfer(withdrawal, _withdraw);
}

function allocatedTransfer(SplitTransfer calldata transfer) external returns (bool) {
return _processSplitTransfer(transfer, _release);
}

function allocatedWithdrawal(SplitTransfer calldata withdrawal) external returns (bool) {
return _processSplitTransfer(withdrawal, _withdraw);
}

function allocatedTransfer(BatchTransfer calldata transfer) external returns (bool) {
return _processBatchTransfer(transfer, _release);
}

function allocatedWithdrawal(BatchTransfer calldata withdrawal) external returns (bool) {
return _processBatchTransfer(withdrawal, _withdraw);
}

function allocatedTransfer(SplitBatchTransfer calldata transfer) external returns (bool) {
return _processSplitBatchTransfer(transfer, _release);
}

function allocatedWithdrawal(SplitBatchTransfer calldata withdrawal) external returns (bool) {
return _processSplitBatchTransfer(withdrawal, _withdraw);
}

function enableForcedWithdrawal(uint256 id) external returns (uint256) {
return _enableForcedWithdrawal(id);
}

function disableForcedWithdrawal(uint256 id) external returns (bool) {
return _disableForcedWithdrawal(id);
}

function forcedWithdrawal(uint256 id, address recipient, uint256 amount) external returns (bool) {
return _processForcedWithdrawal(id, recipient, amount);
}

function register(bytes32 claimHash, bytes32 typehash, uint256 duration) external returns (bool) {
_register(msg.sender, claimHash, typehash, duration);
return true;
}

function getRegistrationStatus(address sponsor, bytes32 claimHash, bytes32 typehash) external view returns (bool isActive, uint256 expires) {
expires = _getRegistrationStatus(sponsor, claimHash, typehash);
isActive = expires > block.timestamp;
}

function register(bytes32[2][] calldata claimHashesAndTypehashes, uint256 duration) external returns (bool) {
return _registerBatch(claimHashesAndTypehashes, duration);
}

function consume(uint256[] calldata nonces) external returns (bool) {
return _consume(nonces);
}

function __registerAllocator(address allocator, bytes calldata proof) external returns (uint96) {
return _registerAllocator(allocator, proof);
}

function getForcedWithdrawalStatus(address account, uint256 id) external view returns (ForcedWithdrawalStatus, uint256) {
return _getForcedWithdrawalStatus(account, id);
}

function getLockDetails(uint256 id) external view returns (address, address, ResetPeriod, Scope) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we name the output of the function to make it very clear what address is the token and which is the allocator?

return _getLockDetails(id);
}

function hasConsumedAllocatorNonce(uint256 nonce, address allocator) external view returns (bool) {
return _hasConsumedAllocatorNonce(nonce, allocator);
}

function DOMAIN_SEPARATOR() external view returns (bytes32) {
return _domainSeparator();
}

/// @dev Returns the symbol for token `id`.
function name(uint256 id) public view virtual override returns (string memory) {
return _name(id);
}

/// @dev Returns the symbol for token `id`.
function symbol(uint256 id) public view virtual override returns (string memory) {
return _symbol(id);
}

/// @dev Returns the Uniform Resource Identifier (URI) for token `id`.
function tokenURI(uint256 id) public view virtual override returns (string memory) {
return _tokenURI(id);
}

/// @dev Returns the name for the contract.
function name() external pure returns (string memory) {
// Return the name of the contract.
assembly ("memory-safe") {
mstore(0x20, 0x20)
mstore(0x4b, 0x0b54686520436f6d70616374)
return(0x20, 0x60)
}
}

function _beforeTokenTransfer(address from, address to, uint256 id, uint256 amount) internal virtual override {
_ensureAttested(from, to, id, amount);
}
}
8 changes: 8 additions & 0 deletions src/interfaces/IAllocator.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.27;

// NOTE: Allocators with smart contract implementations should also implement EIP1271.
interface IAllocator {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As suggested in the server-allocator PR, I would have the IAllocator inherit IERC1271 as the isValidSignature function is required when creating an allocator contract.

// Called on standard transfers; must return this function selector (0x1a808f91).
function attest(address operator, address from, address to, uint256 id, uint256 amount) external returns (bytes4);
}
Loading