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

fix: avoid duplicate calls on withdraw hook when sender is also the recipient #826

Merged
merged 4 commits into from
Feb 20, 2024
Merged

fix: avoid duplicate calls on withdraw hook when sender is also the recipient #826

merged 4 commits into from
Feb 20, 2024

Conversation

smol-ninja
Copy link
Member

Fixes #822

andreivladbrg
andreivladbrg requested changes Feb 20, 2024
View reviewed changes
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 like the idea to divide the tests in multiple files to avoid super complicated tree branches.

test/integration/concrete/lockup/withdraw-hooks/withdrawHooks.t.sol Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw-hooks/withdrawHooks.t.sol Outdated Show resolved Hide resolved
test/integration/shared/lockup/Lockup.t.sol Outdated Show resolved Hide resolved
test/integration/shared/lockup/Lockup.t.sol Outdated Show resolved Hide resolved
test/integration/concrete/lockup/withdraw-hooks/withdrawHooks.t.sol Outdated Show resolved Hide resolved
@andreivladbrg
test: rename create default stream functions
1ebddc7 
test: remove test headers
test: rename withdrawHooks test functions
test: add more modifiers in withdrawHooks tests
@andreivladbrg
Copy link
Member

@smol-ninja i've pushed a commit to address my feedback, lmk if it does look good

@smol-ninja
Copy link
Member Author

Yes. Looks good to me.

PaulRBerg
PaulRBerg requested changes Feb 20, 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.

Feedback in the comments.

Also, could you confirm that setting count to 0 will fail the test if there's at least one call made?

Please answer here:

foundry-rs/foundry#7187

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

Also, could you confirm that setting count to 0 will fail the test if there's at least one call made?

It fails. See the log below.

Screenshot 2024-02-20 at 13 36 00

@smol-ninja
refactor: low priority to "sender != recipient" check for gas optimiz…
01cd652 
…ation

test: rename modifiers name in withdrawHooks.t.sol
test: refactor withdrawHooks.tree
PaulRBerg
PaulRBerg approved these changes Feb 20, 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.

looks good now

@andreivladbrg andreivladbrg merged commit 90dfc22 into staging Feb 20, 2024
6 of 8 checks passed
@smol-ninja smol-ninja deleted the fix/withdraw-hook branch February 20, 2024 15:59
@PaulRBerg
Copy link
Member

This PR introduced the withdrawHooks.t.sol file to separate out the hook tests from the other tests for the withdraw function.

I can no longer see that file on staging. When and why did you undo this structure, @smol-ninja, @andreivladbrg?

@PaulRBerg
Copy link
Member

Also, I can't identify where this boolean check is tested. Do we have any concrete tests written for it? Or are we relying on fuzzing?

SCR-20240329-lwkg

@PaulRBerg
Copy link
Member

It looks like the hook tests have been removed in this PR:

#817

But I still don't understand why.

@smol-ninja
Copy link
Member Author

smol-ninja commented Mar 31, 2024 •
edited
Loading

I can no longer see that file on staging. When and why did you undo this structure.

You are right. It got deleted in LockupTranched PR. I was the reviewer so its my mistake that I overlooked it. @andreivladbrg can you please confirm that it was an accident or is there a reason that I cannot recall?

I can't identify where this boolean check is tested

In withdrawHooks.tree, there is a branch that tests it when sender and recipient are same.

I have fixed it in #873 in case it was an accident. And thank you very much @PaulRBerg for identifying it 🙏🫡

@smol-ninja smol-ninja mentioned this pull request Mar 31, 2024
Merged
@andreivladbrg
Copy link
Member

I was the reviewer so its my mistake that I overlooked it.

It is also my mistake.

can you please confirm that it was an accident or is there a reason that I cannot recall?

I confirm that it was an accident. There was no reason to remove them.

I can't explain what happened there, most likely something to lots of git rebases and git force pushes.

Nice finding @PaulRBerg and thanks for pointing out.

@PaulRBerg
Copy link
Member

Thanks guys for confirming.

And no worries - error is the default state of affairs.

@PaulRBerg PaulRBerg mentioned this pull request May 16, 2024
Merged
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
@smol-ninja @andreivladbrg
fix: avoid duplicate calls on withdraw hook when sender is also the r…
f3944ed 
…ecipient (#826)

* fix: mitigate duplicate calls on withdraw hook when sender is recipient

* test: add integration tests for withdraw hook calls

* test: rename create default stream functions

test: remove test headers
test: rename withdrawHooks test functions
test: add more modifiers in withdrawHooks tests

* refactor: low priority to "sender != recipient" check for gas optimization
test: rename modifiers name in withdrawHooks.t.sol
test: refactor withdrawHooks.tree

---------

Co-authored-by: andreivladbrg <[email protected]>
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
@smol-ninja @andreivladbrg
fix: avoid duplicate calls on withdraw hook when sender is also the r…
a259d18 
…ecipient (#826)

* fix: mitigate duplicate calls on withdraw hook when sender is recipient

* test: add integration tests for withdraw hook calls

* test: rename create default stream functions

test: remove test headers
test: rename withdrawHooks test functions
test: add more modifiers in withdrawHooks tests

* refactor: low priority to "sender != recipient" check for gas optimization
test: rename modifiers name in withdrawHooks.t.sol
test: refactor withdrawHooks.tree

---------

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

@andreivladbrg andreivladbrg andreivladbrg approved these 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.
3 participants
@smol-ninja @andreivladbrg @PaulRBerg