Skip to content

Conversation

@PaulRBerg
Copy link
Member

@PaulRBerg PaulRBerg commented Jan 15, 2023

Implements #261.

As discussed and agreed in #232, this PR adds the Lockup prefix to all relevant entities - contracts, structs, and events.

I ended up separating the lockup streaming logic from the administrative functions of the protocol because of one simple reason - it's a nice developer experience for the ISablierV2Lockup interface to contain only those functions that can be called by end users (as opposed to also containing such functions as setComptroller which are admin-protected).

Later on - if and when we implement the payroll contract - we can adjust the ISablierV2 to fit the new common functions between all contracts. Interfaces can be updated even retroactively.

Update - if you don't like the name of the LockupCreateAmounts struct (as I don't now), don't worry, I have refactored it in a later PR (see #284).

@PaulRBerg PaulRBerg force-pushed the prb/lockup branch 9 times, most recently from 8e791d2 to 56ecf6c Compare January 20, 2023 15: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.

The constructor is not tested in SablierV2(MAX_FEE and comptroller).

we can adjust the ISablierV2 to fit the new common functions between all contracts. Interfaces can be updated even retroactively.

If we plan on doing this, it would be better to rename the actual ISablierV2 to something like ISablierV2Administrative, to be able to inherit it in the future common "ISablierV2" interface, and implicitly in the payroll as well.

if you don't like the name of the LockupCreateAmounts struct (as I don't now), don't worry, I have refactored it in a later PR (see TODO).

Yes, I don't like it either. It is also redundant to have a LockupAmounts in the LockupLinearStream and LockupProStream structs.

@PaulRBerg PaulRBerg force-pushed the prb/lockup branch 2 times, most recently from 5eff3b9 to d30a8c5 Compare January 22, 2023 17:43
@PaulRBerg
Copy link
Member Author

The constructor of SablierV2 is implicitly tested in the constructor tests of SablierV2LockupLinear and SablierV2LockupPro.

rename the actual ISablierV2 to something like ISablierV2Administrative

I'm not against this but finding a good name won't be easy - in fact, this is why I used ISablierV2. Can you open a discussion about this, please?

It is also redundant to have a LockupAmounts in the LockupLinearStream and LockupProStream structs.

I added that struct as part of my painstaking effort to get rid of StackTooDeep in #220 - so it is not redundant, not at all.

The amounts struct also provides type encapsulation - i.e. grouping of similar struct fields under the same "roof".

@andreivladbrg
Copy link
Member

andreivladbrg commented Jan 22, 2023

I added that struct as part of my painstaking effort to get rid of StackTooDeep in #220 - so it is not redundant, not at all.
The amounts struct also provides type encapsulation - i.e. grouping of similar struct fields under the same "roof".

I was just referring to the names. LockupAmounts is used only in LockupLinearStream and LockupProStream structs, the word "LockUp" is repeated and it is unnecessary.

@PaulRBerg
Copy link
Member Author

PaulRBerg commented Jan 22, 2023

Oh, I see, thanks for clarifying!

There's a reason why I used the Lockup prefix, and I argue it is a good reason. That is, we have to think about the V2 protocol as a set of smart contracts that each has different streaming logic. And "amounts" is probably something that all streaming contracts will have. To avoid an ugly namespace clash in the future (when we get to implement what we currently refer to as the "payroll contract"), I have to decided to preemptively add the Lockup prefix.

We may end up never needing another Amounts in other contracts, but we can't know that right now. The cost of adding the prefix isn't high.

@PaulRBerg PaulRBerg mentioned this pull request Jan 22, 2023
@PaulRBerg
Copy link
Member Author

@andreivladbrg let me know if the PR is good to be merged now.

I have addressed all of your comments, except for those related to the names of the scripts and CI workflows, which I have fixed in #284 (re-fixing them here would tigger a cascade of git conflicts).

@PaulRBerg
Copy link
Member Author

@andreivladbrg, just so you know, I tested all the values initialized at construction time in PR #284.

refactor: add separators in "Events" library
refactor: be more specific in event names
refactor: separate lockup-specific features from sablier v2 configs
test: separate "sablierV2" test contract in "sablierV2" and "lockup"
test: remove "Test" suffix from file names
test: rename all test contracts to have "Something_Test" format
test: rename "sablier-v2" directory to "lockup"
chore: do not git ignore all html files
refactor: override "ERC721" and "IERC721Metadata" in "tokenURI"
test: add tests for "tokenURI"
ci: store artifacts for SablierV2Lockup
ci: update contract names in artifact storage list
that is, in the "_cancel" and the "_cancelMultiple" tests
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