Skip to content
This repository was archived by the owner on Apr 3, 2026. It is now read-only.

Feat/add batch functions#49

Merged
andreivladbrg merged 21 commits intomainfrom
feat/add-batch-functions
May 10, 2024
Merged

Feat/add batch functions#49
andreivladbrg merged 21 commits intomainfrom
feat/add-batch-functions

Conversation

@andreivladbrg
Copy link
Copy Markdown
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 suggested changes Apr 26, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread src/SablierV2OpenEnded.sol
Comment thread src/SablierV2OpenEnded.sol
Comment thread src/SablierV2OpenEnded.sol
Comment thread src/SablierV2OpenEnded.sol Outdated
Comment thread src/SablierV2OpenEnded.sol Outdated
Comment thread test/integration/create-and-deposit-multiple/createAndDepositMultiple.t.sol Outdated
Comment thread test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree Outdated
Comment thread test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree
Comment thread test/integration/create-multiple/createMultiple.tree Outdated
Comment thread test/integration/withdraw/withdraw.t.sol
@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>
smol-ninja
smol-ninja reviewed Apr 29, 2024
View reviewed changes
Comment thread src/SablierV2OpenEnded.sol
smol-ninja
smol-ninja reviewed Apr 29, 2024
View reviewed changes
Comment thread test/integration/stream-debt-of/streamDebtOf.t.sol Outdated
@andreivladbrg
Copy link
Copy Markdown
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
Copy Markdown
Member Author

andreivladbrg commented Apr 29, 2024

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

smol-ninja
smol-ninja suggested changes May 1, 2024
View reviewed changes
Copy link
Copy Markdown
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?

Comment thread test/integration/cancel-multiple/cancelMultiple.tree
Comment thread test/integration/cancel-multiple/cancelMultiple.tree
Comment thread test/integration/cancel-multiple/cancelMultiple.t.sol Outdated
Comment thread test/integration/create-and-deposit-multiple/createAndDepositMultiple.tree Outdated
Comment thread test/integration/create-and-deposit-multiple/createAndDepositMultiple.t.sol Outdated
Comment thread test/integration/create-multiple/createMultiple.tree Outdated
Comment thread test/integration/create-multiple/createMultiple.t.sol Outdated
Comment thread test/integration/withdraw-multiple/withdrawMultiple.tree
Comment thread test/utils/Modifiers.sol
Comment thread test/utils/Modifiers.sol
smol-ninja
smol-ninja reviewed May 8, 2024
View reviewed changes
Comment thread src/SablierV2OpenEnded.sol
@andreivladbrg
Copy link
Copy Markdown
Member Author

andreivladbrg commented May 10, 2024

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

@smol-ninja
Copy link
Copy Markdown
Member

smol-ninja commented May 10, 2024

@andreivladbrg yes you can go ahead and merge it.

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

Reviewers

1 more reviewer

@smol-ninja smol-ninja smol-ninja approved these changes

Reviewers whose approvals may not affect merge requirements

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