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

Reservations #7

Merged
merged 13 commits into from
Jan 2, 2024
Merged

Reservations #7

merged 13 commits into from
Jan 2, 2024

Conversation

m1guelpf
Copy link
Member

No description provided.

@m1guelpf m1guelpf requested a review from philsippl June 15, 2023 09:34
@m1guelpf m1guelpf self-assigned this Jun 15, 2023
@philsippl philsippl requested a review from recmo September 28, 2023 13:55
src/LaunchGrant.sol Outdated Show resolved Hide resolved
src/RecurringGrantDrop.sol Outdated Show resolved Hide resolved
src/GrantReservations.sol Outdated Show resolved Hide resolved
src/GrantReservations.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@recmo recmo left a comment

Choose a reason for hiding this comment

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

LGTM, but I hope we can come up with a cleaner design.

src/LaunchGrant.sol Outdated Show resolved Hide resolved
src/RecurringGrantDrop.sol Show resolved Hide resolved
src/RecurringGrantDrop.sol Outdated Show resolved Hide resolved
src/RecurringGrantDrop.sol Show resolved Hide resolved
src/RecurringGrantDrop.sol Show resolved Hide resolved

grant.checkReservationValidity(timestamp);

address signer = ECDSA.recover(keccak256(abi.encode(timestamp, nullifierHash)), signature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note the OpenZeppelin version has additional restrictions on the signature, which are not necessary for our application. It also doesn't hurt, as long as we take care of this at the signer side (otherwise ~50% of signatures will be rejected).

src/RecurringGrantDrop.sol Outdated Show resolved Hide resolved
src/RecurringGrantDrop.sol Outdated Show resolved Hide resolved
src/RecurringGrantDrop.sol Outdated Show resolved Hide resolved
@philsippl
Copy link
Contributor

philsippl commented Oct 11, 2023

Summary

Since the beginning users could "reserve" their grants instead of claiming in case they are not verified yet. However, the redemption of those reservations was not yet supported on the contract. This PR implements this redemption process.

Reservations

Due to cost considerations the reservation happens fully off-chain. A backend service acting as a "timestamping" service signs the current timestamp and the user's nullifier and stores it in the db. At time of redemption, this signature is submitted on-chain together with the same parameters as a normal claim.

Design

The previous (audited) version of the contracts did not include the possibility of accessing the nullifiers map publicly. However, this is an issue because at the time of redemption one also needs to know whether the same grant might have been also claimed as the "current grant".
The idea to solve for this issue was to try/catch the checkClaim function in the previous version of the contract. Since the nullifier check is the first error in this function it is safe to catch this specific error and ignore any other. However, this is an ugly hack and we don't think this should stay in the contracts forever. Therefore, there are now two contract versions (with suffix Legacy and without). They share nearly the same logic apart from the existence of this nullifier check to the other contract and that the Legacy contract has the normal claim functionality removed. To make those contracts only valid for the grant periods they are needed, they are also set with the according Grant contracts (LaunchGrantLegacy and WLDGrant). Both of them implement the according time restriction.

Intended behaviour

  • Users can claim only at the previous RecurringGrantDrop for grantIds [13,20]
  • Users can redeem reservations at RecurringGrantDropLegacy for grantIds [13,20]
  • Users can claim or redeem at the new RecurringGrantDrop for grantIds [21,∞)

assertEq(token.balanceOf(user), grant.getAmount(20));
}

/// @notice Tests that the user is able to claim tokens if the World ID proof is valid
Copy link
Contributor

Choose a reason for hiding this comment

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

copy-pasta comment needs to be updated :)

// No future grants can be claimed.
if (grantId >= this.getCurrentId()) revert InvalidGrant();

// Only grants 20 and above can be reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Only grants 21 and above can be reserved

@karankurbur karankurbur marked this pull request as ready for review January 2, 2024 20:38
@karankurbur karankurbur merged commit 58d5962 into main Jan 2, 2024
@karankurbur karankurbur deleted the reservations branch January 2, 2024 20:38
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.

4 participants