Skip to content

Simple allocator #5

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

Merged
merged 41 commits into from
Apr 1, 2025
Merged

Simple allocator #5

merged 41 commits into from
Apr 1, 2025

Conversation

mgretzke
Copy link
Collaborator

Pull Request

Description

A simple, fully decentralized allocator that allows for a single claim per token. This means the contract will lock down all tokens of a sponsor for an id for a single claim, so it is not possible to start multiple claims for the same sponsor and id at the same time.
The contract does keep track of the amount of unlocked tokens and will faithfully attest for a transfer of those, even during an ongoing claim. The contract is a good starting point when learning about allocators and it is kept very simple on purpose, to learn about the concept of an allocator or use this contract as a template. To be used in production, the contract would require the ability to work with witness data, since a real cross chain swap will always require additional witness data.

How Has This Been Tested?

The contract is still in development and neither audited, nor has it been fully tested.

@mgretzke mgretzke requested a review from a team as a code owner February 28, 2025 14:39
Copy link
Contributor

@0age 0age left a comment

Choose a reason for hiding this comment

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

Left a few comments for your consideration, but looking great!

function isValidSignature(bytes32 hash, bytes calldata) external view returns (bytes4 magicValue) {
// The hash is the digest of the compact
bytes32 tokenHash = _sponsor[hash];
if (tokenHash == bytes32(0) || _claim[tokenHash] <= block.timestamp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this also confirm that hasConsumedAllocatorNonce is false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not think we need to do that, since we can confidently rely on the Compact contract to have checked the nonce consumption prior to the signature verification... Or did I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

aah yeah that check does happen prior; so i even think this check would always fail 🙃

}

/// @dev copied from IdLib.sol
function _resetPeriodToSeconds(ResetPeriod resetPeriod_) internal pure returns (uint256 duration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to actually import IdLib and use it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about that... Kind of felt a little overkill for these three lines of code. Also, I felt like it would make the contract easier accessible to someone who has not previously interacted with the Compact before. But definitely open to a different opinion!

Copy link
Contributor

Choose a reason for hiding this comment

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

main reason i'd advise it is so that any upstream changes get incorporated; but agreed it's not strictly necessary!

@@ -2,14 +2,14 @@

pragma solidity ^0.8.27;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have tests that use the actual contract and not just this mock? should probably use the proper contract in at least some cases if we can

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, thats why I ended up using the proper Compact contract in the ERC7683Allocator tests. The reason why I did not use it in this allocator "template" is just so the compile times stay fast for devs building on top of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it!

@mgretzke mgretzke requested a review from hensha256 March 12, 2025 09:16
@mgretzke mgretzke merged commit fb8f8e5 into main Apr 1, 2025
4 checks passed
@mgretzke mgretzke deleted the SimpleAllocator branch July 29, 2025 10:11
mgretzke added a commit that referenced this pull request Aug 1, 2025
commit 66fd55f
Author: mgretzke <[email protected]>
Date:   Fri Aug 1 15:01:56 2025 +0200

    cleanup

commit fb8f8e5
Merge: 324782a 0ac955d
Author: Mark Gretzke <[email protected]>
Date:   Tue Apr 1 14:11:04 2025 +0200

    Merge pull request #5 from Uniswap/SimpleAllocator

    Simple allocator

commit 0ac955d
Merge: a2fb41b 273e632
Author: Mark Gretzke <[email protected]>
Date:   Tue Apr 1 09:45:16 2025 +0200

    Merge pull request #6 from Uniswap/ERC7683Allocator

    Erc7683 allocator

commit 273e632
Merge: 4bed9de 10162ae
Author: 0age <[email protected]>
Date:   Thu Mar 27 12:36:19 2025 -0400

    Merge pull request #7 from Uniswap/mandate-reverse-dutch-auction

    Updates to support reversed dutch auction

commit a2fb41b
Author: vimageDE <[email protected]>
Date:   Thu Mar 6 18:51:00 2025 +0100

    enable relayed locks in child contracts

commit 32dafd3
Author: vimageDE <[email protected]>
Date:   Wed Mar 5 11:34:37 2025 +0100

    use this.attest.selector

commit f91a8cc
Author: vimageDE <[email protected]>
Date:   Wed Mar 5 11:05:53 2025 +0100

    Removed sponsor == operator requirement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants