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

feat: MerkleVCA contract #58

Merged
merged 9 commits into from
Feb 18, 2025
Merged

feat: MerkleVCA contract #58

merged 9 commits into from
Feb 18, 2025

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Feb 6, 2025

Closes #43

Note

The factory won't be able to deploy SablierMerkleVCA contract if the following requirements are not met:

  1. The expiration must be >= vesting.end + 7 days (follow this discussion).
  2. vesting.end > vesting.start

Unlike other Merkle campaigns where users are incentivised to claim early, in MerkleVCA, users are incentivised to claim late. That means, its entirely possible to see no claim until the vesting ends. So its important to check for the constraints during the deployment instead of the _claim function.

Blocker

Related discussions

refactor: separate out parameters struct for Merkle Instant and Merkle Lockups
refactor: refactor shape and camapign name as bytes32 in Merkle campaign constructor arguments

chore: contract inheritance

test: deploy factory locally for fork tests

feat: merkle vca contract

feat: add CreateMerkleVCA run script
refactor: rename schedule to _vestingSchedule in MerkleVCA
feat: add SablierMerkleVCA in prepare-artifacts bash script
refactor: replace internal variables with private in non-abstract contracts

refactor: 7 days claim window post vesting unlock
fix: set sacrificedAmount to 0 in refundSacrificedAmount function
refactor: error name

feat: revert claim if vesting start time is in future
feat: revert createMerkleVCA if either of vesting times is zero

test: integration tests for createMerkleVCA

fix: calculate claimable amount only if blocktime is in the vesting range
test: add merkleVCA integration tests

test: integration tests

test: fork tests for merkleVCA

test: remove Vars struct from merkle instant contructor test

refactore: rename sacrifiedAmount to forgoneAmount
@smol-ninja smol-ninja marked this pull request as draft February 6, 2025 11:07
@smol-ninja smol-ninja marked this pull request as ready for review February 6, 2025 12:31
@smol-ninja

This comment was marked as resolved.

@smol-ninja smol-ninja force-pushed the feat/avca branch 2 times, most recently from 52474ad to 0f7a97f Compare February 9, 2025 18:24
@smol-ninja
Copy link
Member Author

This PR is now ready for your review @andreivladbrg.

Base automatically changed from refactor/merkle-campaign-params to staging February 12, 2025 15:23
@smol-ninja smol-ninja force-pushed the feat/avca branch 2 times, most recently from cb87898 to 320595e Compare February 12, 2025 22:27
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

while reviewing, a question came to mind—should we allow creating a MerkleVCA campaign with an end time in the past? or should we revert and instead, recommend the user to create a MerkleInstant campaign?

this would mean adding the following check in the constructor:

if(endTime <= block.timestamp) {
   revert();
}

haven’t reviewed the tests yet, but in order to be more time-efficient, i’m going to leave these comments:

chore: use named args in transfer function
chore: correct prepare artifacts interface
@smol-ninja
Copy link
Member Author

while reviewing, a question came to mind—should we allow creating a MerkleVCA campaign with an end time in the past? or should we revert and instead, recommend the user to create a MerkleInstant campaign?

My reply is #58 (comment).

Co-authored-by: Andrei Vlad Birgaoanu <[email protected]>
@smol-ninja smol-ninja force-pushed the feat/avca branch 2 times, most recently from c54bc79 to 8a9eb88 Compare February 18, 2025 13:20
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

great work, left some comments re tests:

test: rename dir to tokens
small polishes
@smol-ninja
Copy link
Member Author

Added a new commit, 651f982, incorporating your feedback as well as fixing a bug in VCA fork tests.

@smol-ninja
Copy link
Member Author

@andreivladbrg looking forward to your approval on this PR.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

r_downey_thumbs_up

@smol-ninja smol-ninja merged commit 50487e0 into staging Feb 18, 2025
7 checks passed
@smol-ninja smol-ninja deleted the feat/avca branch February 18, 2025 17:09
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.

3 participants