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/add batch functions #49

Merged
merged 21 commits into from
May 10, 2024
Merged

Feat/add batch functions #49

merged 21 commits into from
May 10, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Apr 22, 2024 •
edited by smol-ninja
Loading

Closes https://github.com/sablier-labs/v2-open-ended/issues/10 and #31

About

Implements the following batch functions:

  • cancelMultiple
  • createMultiple
  • createAndDepositMultiple
  • withdrawMultiple

Notes:

  • we don't need to check explicitly check the array counts in createAndDepositMultiple because they are already checked in the function called (createMultiple & depositMultiple).
  • the createAndDepositMultiple can be optimized to not call depositMultiple because this function checks if all the streams are canceled, i am not sure if we modify this would be worth the complexity

Also, the tests for createMultipleand createMultipleAndDepositMultiple are going to be added later.

@andreivladbrg andreivladbrg mentioned this pull request Apr 22, 2024
Merged
@andreivladbrg
feat: add withdrawMultiple function
9cfe2ff 
feat: add cancelMultiple functions
@andreivladbrg
chore: capitalize ID in comments
cb8fe0a
@andreivladbrg
test: rename function to expectRevertNull
ea43360 
test: rename function to expectRevertCanceled
@andreivladbrg
test: cancelMultiple function
7683de2 
test: add user eve and use for the malicious third party tests
@andreivladbrg
test: add defaultStreamIds array in Integration_Test
14f1024
@andreivladbrg
test: withdrawMultiple function
8e52416 
test: set the block.timestamp to May 1 2024
@andreivladbrg
feat: implement createMultiple function
b4d9d47
@andreivladbrg
feat: implement createAndDepositMultiple function
34ed95c 
refactor: use specific amount names instead of a generic one
@andreivladbrg
docs: improve readability for streamId requirements
53f7b3a 
test: say "given" for balance zero tests
@andreivladbrg
refactor: change order of array counts in custom error
0d0576a 
test: createMultiple function
@andreivladbrg andreivladbrg force-pushed the feat/add-batch-functions branch from 409cd67 to 0d0576a Compare April 22, 2024 15:48
@andreivladbrg andreivladbrg force-pushed the feat/add-batch-functions branch from 85ac77e to 6cca0e6 Compare April 24, 2024 13:53
smol-ninja
smol-ninja requested changes Apr 26, 2024
View reviewed changes
Copy link
Member

@smol-ninja smol-ninja left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should we separate functions that are related to multiple calls from the core functions? It would make it easier to review and read the contract. An example:

  1. SablierV2OpenEndedCore contains all core logic and can be put in the abstracts folder
  2. SablierV2OpenEnded contains functions related o multiple. This will be a child contract of SablierV2OpenEndedCore.

Do we need to prefix error names with SablierV2OpenEnded? Since they are emitted by OpenEnded contracts, it this necessary? I am fine with prefixing because its same in Lockup contracts. But there, it made sense because it has more than one contracts. Here, I would prefer them without prefix.

Also there are some changes related to refactor for which I have opened a separate PR.

src/SablierV2OpenEnded.sol Show resolved Hide resolved
src/SablierV2OpenEnded.sol Show resolved Hide resolved
src/SablierV2OpenEnded.sol Show resolved Hide resolved
src/SablierV2OpenEnded.sol Outdated Show resolved Hide resolved
src/SablierV2OpenEnded.sol Outdated Show resolved Hide resolved
test/integration/create-and-deposit-multiple/createAndDepositMultiple.t.sol Outdated Show resolved Hide resolved
test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree Outdated Show resolved Hide resolved
test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree Show resolved Hide resolved
test/integration/create-multiple/createMultiple.tree Outdated Show resolved Hide resolved
test/integration/withdraw/withdraw.t.sol Show resolved Hide resolved
@smol-ninja @andreivladbrg
Refactoring open ended (#52)
e15c02c 
* perf: optimize modifiers

* refactor: rename streamDebt to streamDebtOf

* fix: add override

* style: solhint-disable no-console

* chore: use return variable in streamDebtOf

* test: update streamDebt files

---------

Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Apr 29, 2024 •
edited by smol-ninja
Loading

Should we separate functions that are related to multiple calls from the core functions? It would make it easier to review and read the contract. An example:
SablierV2OpenEndedCore contains all core logic and can be put in the abstracts folder
SablierV2OpenEnded contains functions related o multiple. This will be a child contract of SablierV2OpenEndedCore

interesting proposal Shub, i don't have an opinion yet on this, could you please a separate discussion for this?

also, i won't name the contract with "core" but with smth like: SablierV2OpenEndedBase

I am fine with prefixing because its same in Lockup contracts. But there, it made sense because it has more than one contracts. Here, I would prefer them without prefix.

answered in the separate discussion #51

@andreivladbrg
Copy link
Member Author

@smol-ninja i addressed your feedback, please lmk if i missed something

smol-ninja
smol-ninja requested changes May 1, 2024
View reviewed changes
Copy link
Member

@smol-ninja smol-ninja left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have reviewed it once more and left some suggestions below.

Also what do you think about using "arrays length" instead of "arrays count" throughout the code?

test/integration/cancel-multiple/cancelMultiple.tree Show resolved Hide resolved
test/integration/cancel-multiple/cancelMultiple.tree Show resolved Hide resolved
test/integration/cancel-multiple/cancelMultiple.t.sol Outdated Show resolved Hide resolved
test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree Outdated Show resolved Hide resolved
test/integration/create-and-deposit-multiple/createAndDepositMultiple.t.sol Outdated Show resolved Hide resolved
test/integration/create-multiple/createMultiple.tree Outdated Show resolved Hide resolved
test/integration/create-multiple/createMultiple.t.sol Outdated Show resolved Hide resolved
test/integration/withdraw-multiple/withdrawMultiple.tree Show resolved Hide resolved
test/utils/Modifiers.sol Show resolved Hide resolved
test/utils/Modifiers.sol Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

can we merge this now @smol-ninja ? it has been opened for a while

@smol-ninja
Copy link
Member

@andreivladbrg yes you can go ahead and merge it.

@andreivladbrg andreivladbrg merged commit e424083 into main May 10, 2024
6 checks passed
@smol-ninja smol-ninja deleted the feat/add-batch-functions branch May 10, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smol-ninja smol-ninja smol-ninja 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
@andreivladbrg @smol-ninja