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

LockupTranched contract #817

Merged
merged 3 commits into from
Mar 6, 2024
Merged

LockupTranched contract #817

merged 3 commits into from
Mar 6, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Feb 7, 2024

Closes #787

About

It introduces the SablierV2LockupTranched contract, similar to the Dynamic contract. However, it differentiates itself by storing tranches instead of segments. The Tranche data structure mirrors the Segment, having the same variables , excluding the exponent.

The calculation of the streamed amount is basically the sum of all vested tranche.amounts.

Other changes

  • alphabetical order in ISablierV2LockupDynamic
  • "dry-fy" the Base_Test contract to approve each core contract when a user is created.

@andreivladbrg andreivladbrg changed the base branch from refactor/common-logic to staging February 7, 2024 16:09
@andreivladbrg andreivladbrg force-pushed the feat/lockup-tranched branch 4 times, most recently from c9374a1 to 2a97859 Compare February 14, 2024 12:53
@andreivladbrg andreivladbrg changed the title [WIP] LockupTranched contract LockupTranched contract Feb 15, 2024
src/types/DataTypes.sol Outdated Show resolved Hide resolved
src/interfaces/ISablierV2LockupTranched.sol Outdated Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Feb 19, 2024

What should be the symbol of SablierV2LockupTranched NFT?

  • SAB-V2-LOCKUP-TRA
  • SAB-V2-LOCKUP-TRANCHED

@PaulRBerg @smol-ninja do you have other suggestions?

@smol-ninja
Copy link
Member

Wouldn't changing it to anything else other than SAB-V2-LOCKUP-TRA require changing Linear and Dynamic symbol name too?

I prefer SAB-V2-LOCKUP-TRA (unless we want to rename all of them).

@PaulRBerg
Copy link
Member

What should be the symbol of SablierV2LockupTranched NFT?

SAB-V2-LOCKUP-TRA

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.

Thanks for the PR. I really enjoyed reviewing this one. It gave me a better insight into the codebase. During the review, I had a few questions and suggestions which I have put down in the comments below.

src/libraries/Helpers.sol Show resolved Hide resolved
src/interfaces/ISablierV2LockupTranched.sol Outdated Show resolved Hide resolved
test/fork/LockupTranched.t.sol Outdated Show resolved Hide resolved
test/utils/Fuzzers.sol Outdated Show resolved Hide resolved
test/utils/Fuzzers.sol Outdated Show resolved Hide resolved
test/utils/Defaults.sol Outdated Show resolved Hide resolved
test/utils/Defaults.sol Outdated Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

@smol-ninja, i've addressed your feedback. lmk if there is anything left, otherwise can you approve?

planning to resolve the conflicts and merge tomorrow

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.

Looks good now. Please go ahead and merge it.

@andreivladbrg andreivladbrg force-pushed the feat/lockup-tranched branch 2 times, most recently from 261d8dc to 7a1abfa Compare March 6, 2024 11:03
feat: include tranched in mapSymbol in NFTDescriptor
docs: specify LockupTranched in README
build: add SablierV2LockupTranched in shell scripts
test: test SablierV2LockupTranched contract
test: deploy core contracts in Base_Test
test: use changePrank instead of vm.StartPrank
test: update Precompiles bytecode
@andreivladbrg
Copy link
Member Author

@smol-ninja i have realized that i have not included SablierV2LockupTranched in scripts, so i pushed a new commit.

can you confirm that it looks good again? sorry

@smol-ninja
Copy link
Member

Looks good.

@andreivladbrg andreivladbrg merged commit 34fee6b into staging Mar 6, 2024
8 checks passed
@smol-ninja smol-ninja deleted the feat/lockup-tranched branch March 8, 2024 14:26
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