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: "withdraw" callable by any account, removes "to" from "withdrawMultiple" #785

Merged
merged 5 commits into from
Jan 23, 2024
Merged

feat: "withdraw" callable by any account, removes "to" from "withdrawMultiple" #785

merged 5 commits into from
Jan 23, 2024

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Jan 8, 2024 •
edited
Loading

Closes #731 and #774.

This PR introduces the following changes.

New terminologies

  1. UnknownCaller: when the caller is neither a sender nor a recipient nor an approved third party, it is called an unknown caller.
  2. Authorized: an address which is authorized to withdraw to any address. Stream's recipient or an approved third party are examples of authorized callers.
  3. Unauthorized: an address which is only authorized to withdraw to the stream's recipient address. Sender or an unknown caller are examples of unauthorized callers..

Changes to withdraw feature

  1. Anybody can call withdraw to recipient address but only the stream's owner or an approved third party is allowed withdraw to any address.
  2. Introduces a new error SablierV2Lockup_WithdrawalAddressNotRecipient and removes SablierV2Lockup_InvalidSenderWithdrawal.
  3. A new test that fuzzes caller address while keeping to as recipient.
  4. New tests in withdraw.t.sol.

Changes to withdrawMultiple feature

  1. withdrawMultiple now requires streamIds and amounts as input and automatically calls withdraw on each stream ID's recipient address.

@smol-ninja smol-ninja changed the base branch from main to staging January 8, 2024 15:16
andreivladbrg
andreivladbrg requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@andreivladbrg andreivladbrg 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.

Feedback below.

Note: I have already made the changes locally on my machine, if we agree on them I can push them here, just lmk.

src/abstracts/SablierV2Lockup.sol Outdated Show resolved Hide resolved
src/abstracts/SablierV2Lockup.sol Outdated Show resolved Hide resolved
src/interfaces/ISablierV2Lockup.sol Show resolved Hide resolved
src/interfaces/ISablierV2Lockup.sol Outdated Show resolved Hide resolved
src/libraries/Errors.sol Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw/withdraw.t.sol Show resolved Hide resolved
test/integration/concrete/lockup/withdraw/withdraw.t.sol Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw/withdraw.t.sol Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw-multiple/withdrawMultiple.tree Show resolved Hide resolved
test/integration/fuzz/lockup/withdraw.t.sol Outdated Show resolved Hide resolved
@smol-ninja
Copy link
Member Author

@andreivladbrg I have added my replies to your comments.

I have already made the changes locally on my machine, if we agree on them I can push them here, just lmk.

OK after we resolve all the above comments.

@andreivladbrg
Copy link
Member

@smol-ninja i have pushed the commit to address the findings

can you confirm that i have not missed anything? and ofc, if you don't agree with something to say

@smol-ninja smol-ninja force-pushed the feat/withdraw-multiple branch 3 times, most recently from 05f08ef to 9239c5d Compare January 11, 2024 17:29
@andreivladbrg andreivladbrg force-pushed the feat/withdraw-multiple branch 2 times, most recently from 05f08ef to 8ac610d Compare January 11, 2024 23:04
@smol-ninja smol-ninja mentioned this pull request Jan 12, 2024
Merged
@PaulRBerg
Copy link
Member

Just wanted to comment to say that I want to review this before merging. I will do it next week.

@andreivladbrg
Copy link
Member

Just wanted to comment to say that I want to review this before merging. I will do it next week.

@PaulRBerg any update?

@PaulRBerg
Copy link
Member

Apologies. I will try to review during the weekend.

@smol-ninja @andreivladbrg
feat: withdraw function callable by any account
f2493e8 
feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist
@smol-ninja @andreivladbrg
refactor: withdraw related notes (#791)
3dd0cd5
@andreivladbrg andreivladbrg force-pushed the feat/withdraw-multiple branch from b0b73a0 to 3dd0cd5 Compare January 21, 2024 20:37
@PaulRBerg
docs: improve writing in NatSpec comments
ca3a023 
refactor: reorder errors
test: improve names for functions and variables
PaulRBerg
PaulRBerg requested changes Jan 23, 2024
View reviewed changes
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

I've reviewed this PR and it generally looks good to me. Thanks for the detailed summary in the OP, @smol-ninja! Super helpful.

The only change I would like to see made is the one about refactoring the withdraw tree to include new modifiers:

  • givenWithdrawalAddressIsRecipient
  • givenWithdrawalAddressNotRecipient

@smol-ninja
Copy link
Member Author

Thanks @PaulRBerg for giving your feedback. I have pushed the changes. Please have a look and let me know if there is anything else you'd like to change.

@smol-ninja smol-ninja mentioned this pull request Jan 23, 2024
Merged
@PaulRBerg
refactor: order of checks in "withdraw"
4fa5ad1 
test: polish withdraw tree and tests
PaulRBerg
PaulRBerg approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

Thanks, @smol-ninja. I have pushed a few revisions. Let me know if they look good to you.

test/integration/concrete/lockup/withdraw/withdraw.tree Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw/withdraw.t.sol Outdated Show resolved Hide resolved
@smol-ninja
Copy link
Member Author

Thanks for those changes @PaulRBerg. It looks more organized now.

@andreivladbrg please let me know if you approve of these changes as well.

@smol-ninja smol-ninja merged commit 3d30aaf into staging Jan 23, 2024
8 of 9 checks passed
@smol-ninja smol-ninja deleted the feat/withdraw-multiple branch January 23, 2024 20:49
andreivladbrg pushed a commit that referenced this pull request Feb 12, 2024
@smol-ninja @PaulRBerg @andreivladbrg
feat: withdraw callable by any account (#785)
ed12ad9 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
andreivladbrg pushed a commit that referenced this pull request Feb 15, 2024
@smol-ninja @PaulRBerg @andreivladbrg
feat: withdraw callable by any account (#785)
6812f53 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
andreivladbrg pushed a commit that referenced this pull request Feb 17, 2024
@smol-ninja @PaulRBerg @andreivladbrg
feat: withdraw callable by any account (#785)
7106c79 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
@smol-ninja smol-ninja mentioned this pull request Mar 28, 2024
Merged
@PaulRBerg PaulRBerg mentioned this pull request Apr 5, 2024
Merged
@PaulRBerg PaulRBerg mentioned this pull request May 16, 2024
Merged
andreivladbrg pushed a commit that referenced this pull request Jul 3, 2024
@smol-ninja @PaulRBerg @andreivladbrg
feat: withdraw callable by any account (#785)
aefbdf5 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
andreivladbrg pushed a commit that referenced this pull request Jul 3, 2024
@smol-ninja @PaulRBerg @andreivladbrg
feat: withdraw callable by any account (#785)
88b15b9 
* feat: `withdraw` function callable by any account

feat: `withdrawMultiple` calls withdraw on streams recipient
test: fuzz test for with when the caller is an unknown address
refactor: rename custom error
chore: improve explanatory comments
test: undo authorized test branches
test: say uknown address instead of public caller
test: make withdraw fuzz test more minimalist

* refactor: withdraw related notes (#791)

* docs: improve writing in NatSpec comments

refactor: reorder errors
test: improve names for functions and variables

* refactor: withdraw tree

* refactor: order of checks in "withdraw"

test: polish withdraw tree and tests

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreivladbrg andreivladbrg andreivladbrg requested changes

@PaulRBerg PaulRBerg PaulRBerg 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.

Make withdraw function callable by any account
3 participants
@smol-ninja @andreivladbrg @PaulRBerg