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

Refactoring open ended #52

Merged
merged 6 commits into from
Apr 29, 2024
Merged

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Apr 27, 2024

I will add to it.

Changelog:

  • Separates notNull from notCanceled to improve readability. This should not have any gas impact.
  • Adds override modifier wherever it was missing.
  • Changes implementation of notCanceled modifier because isCanceled, being a public function, would consume more gas when called.
  • Removes _isCallerStreamSender function as it is nowhere used except onlySender modifier.
  • Rename streamDebt to streamDebtOf because rest of the getters are suffixed with "Of".

@andreivladbrg would love to have your take on these changes. Ofc these are not final until you agree too.

@smol-ninja smol-ninja changed the title Refactoring Open ended repo Refactoring open ended repo Apr 27, 2024
@smol-ninja smol-ninja changed the title Refactoring open ended repo Refactoring open ended Apr 27, 2024
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.

I agree with the changes, thank you and nice findings.

I really missed that override specifier 😅

src/SablierV2OpenEnded.sol Outdated Show resolved Hide resolved
@andreivladbrg andreivladbrg merged commit e15c02c into feat/add-batch-functions Apr 29, 2024
5 checks passed
@smol-ninja smol-ninja deleted the open-ended-review branch April 29, 2024 12:11
andreivladbrg added a commit that referenced this pull request May 10, 2024
* feat: add withdrawMultiple function

feat: add cancelMultiple functions

* chore: capitalize ID in comments

* test: rename function to expectRevertNull

test: rename function to expectRevertCanceled

* test: cancelMultiple function

test: add user eve and use for the malicious third party tests

* test: add defaultStreamIds array in Integration_Test

* test: withdrawMultiple function

test: set the block.timestamp to May 1 2024

* feat: implement createMultiple function

* feat: implement createAndDepositMultiple function

refactor: use specific amount names instead of a generic one

* docs: improve readability for streamId requirements

test: say "given" for balance zero tests

* refactor: change order of array counts in custom error

test: createMultiple function

* test: createAndDepositMultiple function

* Refactoring open ended (#52)

* 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>

* test: refactor streamDebtOf

* test: remove unneeded console log

* refactor: say "amount" in function paramaters instead of explicit names

* test: correct the contract name

test: remove unneeded delegate call test

* perf: optimize createAndDepositMultiple

* test: remove unneeded delegatecall test in cancelMultiple

* test: refactoring relates changes

* test: remove unneeded tree branches

* test: add modifiers in test_CancelMultiple

---------

Co-authored-by: smol-ninja <shubhamy2015@gmail.com>
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.

2 participants