Version 0 Review PR #29

0age · 2024-11-07T17:05:33Z

This PR is meant to serve as a hub for review comments on Version 0.

hensha256 · 2024-11-07T18:14:26Z

src/lib/SharedLogic.sol

+    using SafeTransferLib for address;
+
+    // Storage slot seed for ERC6909 state, used in computing balance slots.
+    uint256 private constant _ERC6909_MASTER_SLOT_SEED = 0xedcaa89a82293940;


what is this value? where did it come from?

this is from solady's ERC6909 implementation, since the _release and _withdraw functions use the same general pathways as the standard transfers (but skip allowance checks and transfer hooks)

src/lib/ClaimProcessorFunctionCastLib.sol

hensha256 · 2024-11-07T18:19:07Z

src/lib/ClaimProcessorFunctionCastLib.sol

+     * @param fnIn   Function pointer to `ClaimProcessorLib.processSimpleClaim`.
+     * @return fnOut Modified function used in `ClaimProcessorLogic._processBasicClaim`.
+     */
+    function usingBasicClaim(function(bytes32, uint256, uint256, bytes32, bytes32, function(address, address, uint256, uint256) internal returns (bool)) internal returns (bool) fnIn)


Can you explain at a high level why this is necessary? I've read the natspec but still not sure. Which one is the correct function type for the function thats going to be called? Why would the type ever be incorrect and need changing?

Yeah, using this function as an example:

ClaimProcessorLib has a function processSimpleClaim that is used as a shared entrypoint for processing four different claim types (basic claim, claim with witness, multichain claim, and multichain claim with witness) where the only key differences are how the claim hash is derived and what the calldata offset to the relevant data is. To facilitate this, the processSimpleClaim function takes a uint256 calldataPointer argument instead of like a BasicClaim calldata argument.

When we're actually calling processSimpleClaim inside of ClaimProcessorLogic._processBasicClaim, the claimPayload has a BasicClaim calldata claimPayload type. While you could do a typecast to a uint256 inside of that function before calling processSimpleClaim, it uses extra gas and critically takes up more contract size, so instead we can use a function cast as a "zero-cost abstraction" to accomplish the same thing

The fnIn function matches the arguments that processSimpleClaim expects, but the fnOut function matches the argument types that are actually being provided

Fix typos

yorhodes · 2024-11-13T16:57:45Z

src/types/EIP712Types.sol

+// Message signed by the sponsor that specifies the conditions under which their
+// tokens can be claimed; the specified arbiter verifies that those conditions
+// have been met and specifies a set of beneficiaries that will receive up to the
+// specified amount of tokens.
+struct Compact {
+    address arbiter; // The account tasked with verifying and submitting the claim.
+    address sponsor; // The account to source the tokens from.
+    uint256 nonce; // A parameter to enforce replay protection, scoped to allocator.
+    uint256 expires; // The time at which the claim expires.
+    uint256 id; // The token ID of the ERC6909 token to allocate.
+    uint256 amount; // The amount of ERC6909 tokens to allocate.
+        // Optional witness may follow.
+}


do you expect that the "conditions under which their tokens can be claimed" to be encapsulated in optional witness data or in the address of the arbiter?

I think generally the conditions would be specified in the witness data which is in turn leveraged by the arbiter; that way the arbiter can be more general-purpose!

vimageDE · 2024-11-19T16:16:08Z

src/TheCompact.sol

+ *         formation and mediation of reusable "resource locks."
+ *         This contract has not yet been properly tested, audited, or reviewed.
+ */
+contract TheCompact is ITheCompact, ERC6909, TheCompactLogic {


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

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

vimageDE · 2024-11-19T16:17:59Z

src/TheCompact.sol

+        _registerWithDefaults(claimHash, typehash);
+    }
+
+    function deposit(address token, address allocator, uint256 amount) external returns (uint256) {


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

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!

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.

vimageDE · 2024-11-19T16:23:46Z

src/TheCompact.sol

+        _registerWithDefaults(claimHash, typehash);
+    }
+
+    function deposit(address allocator, ResetPeriod resetPeriod, Scope scope, address recipient) external payable returns (uint256) {


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.

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

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.

vimageDE · 2024-11-19T16:25:50Z

src/TheCompact.sol

+    }
+
+    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);


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.

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

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.

vimageDE · 2024-11-19T16:27:16Z

src/TheCompact.sol

+        return _performCustomERC20Deposit(token, allocator, resetPeriod, scope, amount, recipient);
+    }
+
+    function deposit(uint256[2][] calldata idsAndAmounts, address recipient) external payable returns (bool) {


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.

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

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.

vimageDE · 2024-11-20T14:00:30Z

src/lib/IdLib.sol

+            y := add(y, shr(32, y))
+
+            // Look up final value in the sequence.
+            compactFlag := and(shr(and(sub(72, and(y, 127)), not(3)), 0xfedcba9876543210000), 15)


Can we add some sort of mathematical proof to show, that the last 88 bits of an address + the compactFlag is sufficient to be fully unique?

would love that (assuming you mean as a comment or a test, since this is getting evaluated frequently)

Yes, that is what I mean

vimageDE · 2024-11-20T14:14:00Z

src/lib/RegistrationLib.sol

+            let expires := add(timestamp(), duration)
+
+            // Ensure new expiration does not exceed current and duration does not exceed 30 days.
+            if or(lt(expires, sload(cutoffSlot)), gt(duration, 0x278d00)) {


Can we move the hardcoded max of 30 days expiration (0x278d00) to a constant for easy adaptation and readability?

yeah that's a great idea, named constants FTW

vimageDE · 2024-11-20T14:16:45Z

src/lib/RegistrationLib.sol

+            // Compute new expiration based on current timestamp and supplied duration.
+            let expires := add(timestamp(), duration)
+
+            // Ensure new expiration does not exceed current and duration does not exceed 30 days.


Should probably be:
// Ensure new expiration is not earlier than current and duration does not exceed 30 days.

good catch!

vimageDE · 2024-11-20T14:20:37Z

src/types/ResetPeriod.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.27;
+


Could help to add a comment to ensure this is enum is not getting changed by mistake during development:
// This enum cannot be increased. Else, the first bit will be cut off in the token id

a bit confused why you would use an enum to represent a duration in the first place

So the logic behind using an enum rather than just an arbitrary value (which was the original design) is twofold:

having the capability to pack all the parameters of the resource lock into the ID is a big win for indexing, and so only taking up 3 bits for the reset period leaves more room for encoding the allocator ID

standardizing on a set of common reset periods will promote composability so there's a greater chance that a given lock can be transferred directly rather than needing to be withdrawn and redeposited into a new lock

vimageDE · 2024-11-20T14:21:09Z

src/types/Scope.sol

@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.27;
+


Could help to add a comment to ensure this is enum is not getting changed by mistake during development:
// This enum cannot be increased. Else, the first bit will be cut off in the token id

yeah maybe with a //@dev tag

yorhodes · 2024-11-20T18:37:12Z

src/types/EIP712Types.sol

+// specified amount of tokens.
+struct Compact {
+    address arbiter; // The account tasked with verifying and submitting the claim.
+    address sponsor; // The account to source the tokens from.


why does this commit to the sponsor address if the sponsor is the signer?

a few reasons:

the allocator also signs the payload, so that way one payload can't be replayed for multiple sponsors

by including this in the payload, the resultant claim hash can uniquely identify the compact in question

you can run validation on the payload without the final signature (say a filler wants to provide a quote and wants to ensure the balance is available, but the allocator doesn't want to leak the full signed payload prematurely)

yorhodes · 2024-11-20T18:38:07Z

src/types/Claims.sol

+    uint256 id; // The token ID of the ERC6909 token to allocate.
+    uint256 allocatedAmount; // The original allocated amount of ERC6909 tokens.
+    address claimant; // The claim recipient; specified by the arbiter.
+    uint256 amount; // The claimed token amount; specified by the arbiter.


something about <= allocatedAmount would be helpful
my mental model for this is fee

maybe originalAmount or maxAmount would be clearer? I definitely want to avoid fee as it has specific connotations that might not be applicable, but it's a good mental model for many applications

yorhodes · 2024-11-20T18:38:55Z

src/types/Claims.sol

+    bytes allocatorSignature; // Authorization from the allocator.
+    bytes sponsorSignature; // Authorization from the sponsor.


find it unintuitive that dynamic length fields are at the beginning (rather than end)

I guess they have a known length but ABI encoder doesnt know that

could you type these to bytes32[2] instead? I see you are using vm.signCompact (https://eips.ethereum.org/EIPS/eip-2098) in tests but not sure if you want to assume all wallets use this

they're at the beginning so that the pointers are always present at specific calldata offsets, regardless of the struct; agree that the ordering is a bit weird, but it's also never directly exposed to end users

there are definitely cases where you can only provide 65 byte signatures, plus we want to support 1271 schemes that might not even pass in an ECDSA signature or need extra data

yorhodes · 2024-11-20T18:40:54Z

src/types/Claims.sol

+    uint256 amount; // The claimed token amount; specified by the arbiter.
+}
+
+struct ClaimWithWitness {


imo it would make these a lot more readable if they used nested structs for the shared fields which I believe should have equivalent memory layout

they have equivalent calldata layouts i think, but memory layout is definitely less compact; main "win" is when comparing across multiple claim endpoints, I'm guessing? when considering a single endpoint in isolation it's kind of a wash

I'm quite bullish on using some kind of custom ABI encoding scheme for V1, though maybe the benefits outweigh the complexity since they're internal calls being performed anyway... 🤔

yorhodes · 2024-11-20T18:42:03Z

src/types/EIP712Types.sol

+struct Compact {
+    address arbiter; // The account tasked with verifying and submitting the claim.
+    address sponsor; // The account to source the tokens from.
+    uint256 nonce; // A parameter to enforce replay protection, scoped to allocator.


what is the sponsor's view into this if its scoped to the allocator? seems hard for clients to avoid nonce collisions

or is it scoped to the allocator and the sponsor?

it's just scoped to the allocator, but a pretty simple / easy to enforce additional scoping scheme that allocators could implement is "first 20 bytes of the nonce must equal the sponsor"

yorhodes · 2024-11-20T18:43:26Z

src/types/ResetPeriod.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.27;
+


a bit confused why you would use an enum to represent a duration in the first place

yorhodes · 2024-11-20T18:45:25Z

src/types/Scope.sol

+enum Scope {
+    Multichain,
+    ChainSpecific
+}


why is this important to distinguish? the arbiter implicitly commits to this

ah, this actually doesn't refer to spending the tokens to receive tokens back on another chain; this is enabling MultichainCompact messages signed on other domains to spend the tokens on the current domain. So like if I had a resource lock on mainnet with Scope.Multichain then I could sign a MultichainCompact against Optimism that incudes a segment with mainnet (1) as the chainId that resource lock and submit it on mainnet to claim the tokens in the resource lock on mainnet, whereas if it's chain-specific I can't use it like that. It's basically just an optional added safety feature if you're on your "home chain"

yorhodes · 2024-11-20T18:49:00Z

src/interfaces/ITheCompactClaims.sol

+ * @notice Claim endpoints can only be called by the arbiter indicated on the associated
+ * compact, and are used to settle the compact in question. There are 96 endpoints in total,
+ * based on all the various possible combinations of a number of factors:
+ *  - transfer vs. withdrawal: whether to transfer the claimed ERC6909 tokens directly, or to


this dimension doubles the number of endpoints (for sponsor clients and arbiters)
is it that unreasonable to require the claimant be ERC6909 compatible?

oh I guess 6909 does not define a deposit/withdraw interface...

yeah it does mean more endpoints, though arbiters could also just opt not to use claimAndWithdraw (or of course they could only use claimAndWithdraw if they preferred)

Sponsor clients don't ever interact with these endpoints fwiw, only with allocatedTransfer and allocatedWithdrawal (which does mean double the endpoints but sponsors would almost certainly want to be able to do both)

you can use the default 6909 transfer methods but there's no way to pass in a signature from the allocator or other context as part of the call, so you need to leverage the attest callback on the allocator for it

vimageDE · 2024-11-25T14:56:06Z

src/TheCompact.sol

+        return _depositBatchAndRegisterViaPermit2(depositor, permitted, resetPeriod, claimHash, compactCategory, witness, signature);
+    }
+
+    function allocatedTransfer(BasicTransfer calldata transfer) external returns (bool) {


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.

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

vimageDE · 2024-11-26T14:54:20Z

src/TheCompact.sol

+        return _getForcedWithdrawalStatus(account, id);
+    }
+
+    function getLockDetails(uint256 id) external view returns (address, address, ResetPeriod, Scope) {


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

vimageDE · 2024-11-26T14:55:26Z

src/lib/ConsumerLib.sol

+library ConsumerLib {
+    // Storage scope identifiers for nonce buckets.
+    uint256 private constant _ALLOCATOR_NONCE_SCOPE = 0x03f37b1a;
+    uint256 private constant _SPONSOR_NONCE_SCOPE = 0x8ccd9613;


Can we make it clear where both of these are coming from?

vimageDE · 2024-11-26T14:57:56Z

src/lib/ConsumerLib.sol

+            // keccak256(_CONSUMER_NONCE_SCOPE ++ account ++ nonce[0:31])
+            mstore(0x20, account)
+            mstore(0x0c, scope)
+            mstore(0x40, nonce)


We could safely use the first 56 bytes (instead of leaving them empty) , this way we do not have to store and rewrite the free memory pointer, because we would not override it.

vimageDE · 2024-11-26T15:00:42Z

src/lib/DepositLogic.sol

+    using SafeTransferLib for address;
+
+    // Storage slot seed for ERC6909 state, used in computing balance slots.
+    uint256 private constant _ERC6909_MASTER_SLOT_SEED = 0xedcaa89a82293940;


We currently have this exact seed stored in multiple places (DepositLogic.sol & SharedLogic.sol), lets stick to one source instead?

vimageDE · 2024-11-26T15:09:15Z

src/lib/DepositViaPermit2Lib.sol

+                    mstore(add(categorySpecificStart, 0x40), PERMIT2_ACTIVATION_MULTICHAIN_COMPACT_TYPESTRING_FRAGMENT_THREE)
+                    mstore(add(categorySpecificStart, 0x60), PERMIT2_ACTIVATION_MULTICHAIN_COMPACT_TYPESTRING_FRAGMENT_FOUR)
+                    mstore(add(categorySpecificStart, 0x70), PERMIT2_ACTIVATION_MULTICHAIN_COMPACT_TYPESTRING_FRAGMENT_SIX)
+                    mstore(add(categorySpecificStart, 0x60), PERMIT2_ACTIVATION_MULTICHAIN_COMPACT_TYPESTRING_FRAGMENT_FIVE)


POSSIBLE BUG:
FRAGMENT_FIVE needs to be starting at 0x80 to not override FRAGMENT_FOUR. And FRAGMENT_SIX needs therefor to be starting at 0x90

vimageDE · 2024-11-26T15:21:15Z

src/lib/DepositViaPermit2Lib.sol

+                    mstore(add(categorySpecificStart, 0x60), PERMIT2_ACTIVATION_MULTICHAIN_COMPACT_TYPESTRING_FRAGMENT_FIVE)
+
+                    // Set memory pointers for Activation and Category-specific data end.
+                    categorySpecificEnd := add(categorySpecificStart, 0x90)


POSSIBLE BUG:
The end is based on the (likely wrong) previous values. When those are fixed,
FRAGMENT_SIX would actually be ending at categorySpecificStart + 0xb0, not 0x90

vimageDE · 2024-11-26T15:30:42Z

src/lib/DepositViaPermit2Lib.sol

+
+                // Insert tokenPermissions typestring fragment.
+                let tokenPermissionsFragmentStart := add(categorySpecificEnd, witnessLength)
+                mstore(add(tokenPermissionsFragmentStart, 0x0e), TOKEN_PERMISSIONS_TYPESTRING_FRAGMENT_TWO)


This will override the last byte of the witness data, which is also supposed to be a ) (closing bracket).
After talking with @0age about this, a solution for version 1 would be to limit the witness data to the input of the struct.
Example of a CURRENTLY expected witness:

Witness witness)Witness(uint256 witnessArgument)

The NEW expected witness data would look like this:

uint256 witnessArgument

The new approach will be clearer for developers and lead to smaller calldata. It also ensures the requirements of EIP-712, that all struct definitions need to be in alphanumerical order in the typestring (as long as the Witness does not contain additional structs). This makes the approach less prone to errors for other developers.

vimageDE · 2024-11-26T16:38:07Z

src/lib/DepositViaPermit2Logic.sol

+     * @notice Internal function for depositing ERC20 tokens using Permit2 authorization. The
+     * depositor must approve Permit2 to transfer the tokens on its behalf unless the token in
+     * question automatically grants approval to Permit2. The ERC6909 token amount received by the
+     * by the recipient is derived from the difference between the starting and ending balance held


2x by the

add files back

058c2d1

0age changed the title ~~add files back~~ Version 0 Review PR Nov 7, 2024

hensha256 reviewed Nov 7, 2024

View reviewed changes

0age added 2 commits November 7, 2024 11:18

Fix typos

5a5b649

Merge pull request #30 from Uniswap/main

6708e48

Fix typos

yorhodes reviewed Nov 13, 2024

View reviewed changes

vimageDE self-requested a review November 19, 2024 16:12

vimageDE reviewed Nov 20, 2024

View reviewed changes

yorhodes reviewed Nov 20, 2024

View reviewed changes

vimageDE reviewed Nov 25, 2024

View reviewed changes

vimageDE reviewed Nov 26, 2024

View reviewed changes

		@@ -0,0 +1,13 @@
		// SPDX-License-Identifier: MIT
		pragma solidity ^0.8.27;

		@@ -0,0 +1,7 @@
		// SPDX-License-Identifier: MIT
		pragma solidity ^0.8.27;

		bytes allocatorSignature; // Authorization from the allocator.
		bytes sponsorSignature; // Authorization from the sponsor.

Version 0 Review PR #29

Are you sure you want to change the base?

Version 0 Review PR #29

Conversation

0age commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorhodes Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorhodes Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vimageDE Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0age commented Nov 7, 2024 •

edited

Loading

yorhodes Nov 13, 2024 •

edited

Loading

yorhodes Nov 20, 2024 •

edited

Loading

vimageDE Nov 25, 2024 •

edited

Loading