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.

Sign up for GitHub

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

Jump to bottom

feat: transfer tokens directly if claimed after vesting end time #77

Merged
merged 2 commits into from
Feb 28, 2025
Merged

feat: transfer tokens directly if claimed after vesting end time #77

merged 2 commits into from
Feb 28, 2025

Conversation

smol-ninja
Copy link
Member

Closes #37

Changelog

  • Transfers tokens to recipient if claim time exceeds vesting end time in Merkle Lockup
  • Adds notes in claim()'s natspecs
  • Add a new Claim event in Merkle Lockup interface as discussed here
  • Fuzzes start time in Merkle Lockup's fork tests
  • Makes ranged streams as the default streams for integration tests

@smol-ninja smol-ninja changed the title feat: transfer tokens to recipient if claim time exceeds vesting endt… feat: transfer tokens if claim is made after vesting end time Feb 25, 2025
@smol-ninja smol-ninja changed the title feat: transfer tokens if claim is made after vesting end time feat: transfer tokens if claimed after vesting end time Feb 25, 2025
@smol-ninja smol-ninja changed the title feat: transfer tokens if claimed after vesting end time feat: transfer tokens directly if claimed after vesting end time Feb 25, 2025
andreivladbrg
andreivladbrg reviewed Feb 26, 2025
View reviewed changes
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.

Just an idea (no strong opinion): now since there is the no guarantee that all aggregateAmount is going to be placed Lockup contract, should we move the .approve function in the _claim before creating the stream. The con is obvious, gas increase for the user.

left some comments on src:

@smol-ninja
Copy link
Member Author

Just an idea (no strong opinion): now since there is the no guarantee that all aggregateAmount is going to be placed Lockup contract, should we move the .approve function in the _claim before creating the stream. The con is obvious, gas increase for the user.

I understand your rationale and agree with you. But since we have to find a balance between gas efficiency and security, imo this is a very unlikely event where campaign creator passes a malicious lockup and also funds the campaign without checking it. It also means he has not used the Sablier UI to do the same. So thats a low probability security concern.

At the same time, gas usage per claim will add ~35k which is a lot.

So IMO, the gas increase is not justified here.

@andreivladbrg
Copy link
Member

I understand your rationale and agree with you. But since we have to find a balance between gas efficiency and security, imo this is a very unlikely event where campaign creator passes a malicious lockup and also funds the campaign without checking it. It also means he has not used the Sablier UI to do the same. So thats a low probability security concern.

At the same time, gas usage per claim will add ~35k which is a lot.
So IMO, the gas increase is not justified here.

good points, agree, it was just an idea, let's keep the approve in the constructor

@smol-ninja
Copy link
Member Author

@andreivladbrg included all your suggestion. Feel free to give it a final review.

andreivladbrg
andreivladbrg requested changes Feb 28, 2025
View reviewed changes
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.

left some more comments re tests

if you agree with them, i can push a commit with the changes, as i implemented them while reviewing

@smol-ninja smol-ninja force-pushed the feat/direct-transfer-vested-tokens branch from df60eaa to 1c7a3a6 Compare February 28, 2025 18:37
@smol-ninja smol-ninja changed the base branch from staging to feat/chainlink-oracle February 28, 2025 18:38
@smol-ninja smol-ninja force-pushed the feat/direct-transfer-vested-tokens branch 2 times, most recently from 38b5117 to 49b8b70 Compare February 28, 2025 18:39
Base automatically changed from feat/chainlink-oracle to staging February 28, 2025 18:40
@smol-ninja @andreivladbrg
feat: transfer tokens to recipient if claim time exceeds vesting endt…
c60dd77 
…ime in merkle lockup

docs: add notes in claim natspecs
feat: add a new Claim event in merkle lockup interface
test: fuzz start time in merkle lockup
test: use ranged streams as default streams for integration tests

perf: use schedule in memory
docs: polish natspecs

test(fork): assertEq for expected lockup stream

test(fork): move hasClaimed assert outside if block
test(claim): re order tree branches
test: don't use variable names in tree branches
chore: set max-line-length rule to 128

test: polish comment

Co-authored-by: Andrei Vlad Birgaoanu <andreivladbrg@gmail.com>
@smol-ninja smol-ninja force-pushed the feat/direct-transfer-vested-tokens branch from 49b8b70 to c60dd77 Compare February 28, 2025 18:41
andreivladbrg
andreivladbrg approved these changes Feb 28, 2025
View reviewed changes
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.

shogun_lgtm

@andreivladbrg andreivladbrg merged commit 7385325 into staging Feb 28, 2025
7 checks passed
@andreivladbrg andreivladbrg deleted the feat/direct-transfer-vested-tokens branch February 28, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreivladbrg andreivladbrg andreivladbrg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smol-ninja @andreivladbrg