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

refactor: update repo for the new iteration #43

Merged
merged 11 commits into from
Feb 3, 2025

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Jan 15, 2025

Closes #42

Changes

feat: add airdrops dir
feat: add linear curves creator
feat: add tranched curves creator
refactor: restructure lockup dir
refactor: update lockup to singleton version
refactor: use latest deployments addresses
build: install staging version in for all repos
test: update the block number forked
test: add tests for linear and tranched curves creators

refactor: restructure lockup dir
refactor: update lockup to singleton version
refactor: use latest deployments addresses
build: install staging version in for all repos
test: update the block number forked
@andreivladbrg andreivladbrg marked this pull request as draft January 15, 2025 17:17
refactor: rename airstream to merkle
feat: add tranched curves creator
refactor: move from dynamy to linear and tranched the correct shapes
@andreivladbrg andreivladbrg marked this pull request as ready for review January 16, 2025 17:15
@andreivladbrg
Copy link
Member Author

@smol-ninja I’ve marked the PR as ready for review.

One thing that wasn’t mentioned in the OP is the need to move the dynamic shapes from (CurvesCreator) to linear and to tranched.

Copy link
Member

@smol-ninja smol-ninja 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 on the PR. Some feedback below. I have also added a commit with some changes on comments. There were some linting warnings so the changes to the solhint config are to disable them.

@andreivladbrg
Copy link
Member Author

I see that many comments are about the names used in lockup linear curves. My intention was to keep the existing terminology from the docs, but it seems that it’s not working well.

IMO we shouldn't explicitly include “linear” in the shape names (except for the default one), as all shapes are within the “lockup linear” model, which is implicit enough. Therefore, I propose the following new names, based on what distinguishes them from the default linear shape.

Scenario Shape Name Cliff Time Start Amount Cliff Amount
1 Linear
3 Cliff Unlock
3 Initial Unlock
4 Initial Cliff Unlock
5 Constant Cliff
6 Initial Unlock Constant Cliff

(still not sure about shape 6 because it is too long)

Plot graphs

Scenario 5

image

Scenario 6

image

lmk what you think

@smol-ninja
Copy link
Member

as all shapes are within the “lockup linear” model

Its implicit to us but may not be for someone who is reading the examples. The expliciteness in names can help them understand that the stream is still Linear with start and cliff unlocks. All the functions under Linear examples create a Linear stream with different configurations.

was to keep the existing terminology from the docs

In the docs, it looks good because of the different levels of heading. Cliff is under Linear categories so it is very clear.

Your proposal is good for docs because we have a level 2 heading called Linear and then level 3 headings called cliff unlock, initial unlock etc. but imo we can have function names suffixed with Linear in examples.

Think of it this way.

function createStream_CliffLinear is made of cliff which is level 3 heading in docs and Linear which is level 2 heading.

@andreivladbrg
Copy link
Member Author

andreivladbrg commented Jan 21, 2025

Its implicit to us but may not be for someone who is reading the examples The expliciteness in names can help them understand that the stream is still Linear with start and cliff unlocks.

In the docs, it looks good because of the different levels of heading. Cliff is under Linear categories so it is very clear.

ser, the contract name is LockupLinearCurvesCreator. if it is clear on the docs, it is clear here as well IMO

Your proposal is good for docs because we have a level 2 heading called Linear and then level 3 headings called cliff unlock, initial unlock etc. but imo we can have function names suffixed with Linear in examples.

here the level 1 heading is the contract name

so, i still think the list from above (without including the Linear word) is a good one

@smol-ninja
Copy link
Member

Fair points. Lets go it your way.

@andreivladbrg andreivladbrg force-pushed the refactor/new-iteration branch from 43ff218 to 5328ac3 Compare January 21, 2025 16:02
@smol-ninja
Copy link
Member

Just a note that Batch examples do not contain reference to unlock amounts in case of batch LL. So they are being passed as 0.

Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

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

Approving this since we are ready to merge it tomorrow. Repeating my previous note ICYMI:

Batch examples do not contain reference to unlock amounts in case of batch LL. So they are being passed as 0.

@andreivladbrg
Copy link
Member Author

just pushed a new commit adding the unlock amounts in the batchLL example dc85aa6

we should sync this merge with the docs PR

@andreivladbrg andreivladbrg merged commit 0630cac into main Feb 3, 2025
3 checks passed
@andreivladbrg andreivladbrg deleted the refactor/new-iteration branch February 3, 2025 12:32
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.

Update examples for Q1 2025 release
2 participants