refactor: `amount` param in `_withdraw` #232

andreivladbrg · 2024-09-09T00:21:35Z

Closes

Depends on #220

Changes

change time with amount in withdraw
remove time param in all functions and rely only on block.timestamp
remove balance == 0 check in withdraw
remove unneeded totalWithdrawAmount variable
update withdraw and amount calculation functions
dry-fy tests

Some notes:

We now can allow withdrawals for an amount less than snapshotDebt which previously not possible with time param.

Since now the withdraw amount is bounded between [1, coveredDebt], I have used unchecked for the balance subtraction, as we know for sure is not going to be greater than it.

flow/src/SablierFlow.sol

Lines 787 to 790 in c772b5b

    
           // Check: the withdraw amount is not zero. 
        
           if (amount == 0) { 
        
               revert Errors.SablierFlow_WithdrawAmountZero(streamId); 
        
           }

and

flow/src/SablierFlow.sol

Lines 823 to 826 in c772b5b

    
           // Check: the withdraw amount is not greater than the withdrawable amount. 
        
           if (amount > withdrawableAmount) { 
        
               revert Errors.SablierFlow_Overdraw(streamId, amount, withdrawableAmount); 
        
           }

Also, this is why I've added the equal check in else branch:

flow/src/SablierFlow.sol

Line 843 in c772b5b

else if (amount == totalDebt) {

smol-ninja · 2024-09-09T13:39:10Z

As a reminder to us:

Update invariant test: fix: bug in snapshot time in the presence of low rps #220 (comment)
Allow timeJump to have 0 as the lower bound in both invariants as well as fuzz.

test: update tests accordingly

smol-ninja

Thanks for the PR @andreivladbrg. Great work on this. Here is a partial review and the rest of the review depends on the conclusion from the following comments.

package.json

benchmark/Flow.Gas.t.sol

src/interfaces/ISablierFlow.sol

src/SablierFlow.sol

src/interfaces/ISablierFlow.sol

src/libraries/Helpers.sol

src/SablierFlow.sol

test/integration/concrete/withdraw-max/withdrawMax.t.sol

test/integration/concrete/batch/batch.t.sol

andreivladbrg · 2024-09-11T12:32:18Z

@smol-ninja I've pushed a commit a8b77f5 to address your feedback, lmk if it looks good

test: change token symbols

test: assertions in withdrawMax fuzz test

smol-ninja

Given a full review. I must say with amount as input parameter, the code now looks a bit better.

Except https://github.com/sablier-labs/flow/actions/runs/10811580895/job/29991351943?pr=232 (which I have yet to investigate), everything looks great. Left some comments below.

src/SablierFlow.sol

test/integration/fuzz/coveredDebtOf.t.sol

src/SablierFlow.sol

test/integration/fuzz/withdrawMax.t.sol

test/integration/concrete/withdraw/withdraw.tree

smol-ninja · 2024-09-13T14:45:29Z

@andreivladbrg feel free to merge this PR now. And open separate PRs for other works such as related to invariant or the ones mentioned above (refactoring withdraw branches). I am convinced this is the way to go so lets gooo.

smol-ninja

Great work with this btw. I still look forward to some failing tests but until then, this is the winner.

andreivladbrg · 2024-09-13T14:50:16Z

feel free to merge this PR now

sounds good, merging now

Great work with this btw. I still look forward to some failing tests but until then, this is the winner.

thank you, great suggestions as well

* refactor: change the time param in withdraw with an amount test: update tests accordingly * docs: fix * perf: remove redudant assert * Shub's feedback * test: remove redundant lines in withdraw handler test: change token symbols * undeeded imports * test: add getRenormalizedAmount in Utils.sol test: assertions in withdrawMax fuzz test * test: set lower bound for timejump to 0 in fuzz and invariant * remove comment * test: lower bound for rps * refactor: remove corrected time logic (#236) * chore: polish * chore: polish --------- Co-authored-by: smol-ninja <[email protected]> Co-authored-by: gavriliumircea <[email protected]>

andreivladbrg marked this pull request as draft September 9, 2024 00:21

smol-ninja mentioned this pull request Sep 9, 2024

fix: bug in snapshot time in the presence of low rps #220

Merged

smol-ninja mentioned this pull request Sep 9, 2024

chore: use plural form in test dir #233

Merged

andreivladbrg force-pushed the refactor/withdraw-amount branch 3 times, most recently from e12c17c to 6dd2d15 Compare September 9, 2024 13:56

andreivladbrg marked this pull request as ready for review September 9, 2024 13:56

andreivladbrg requested a review from smol-ninja September 9, 2024 13:56

andreivladbrg force-pushed the refactor/withdraw-amount branch 2 times, most recently from 2d2eb19 to dd917e2 Compare September 9, 2024 14:21

refactor: change the time param in withdraw with an amount

c772b5b

test: update tests accordingly

andreivladbrg force-pushed the refactor/withdraw-amount branch from dd917e2 to c772b5b Compare September 9, 2024 14:21

smol-ninja mentioned this pull request Sep 10, 2024

Add assert statement in Helpers.calculateAmountsFromFee #230

Closed

smol-ninja requested changes Sep 10, 2024

View reviewed changes

docs: fix

4a37bfe

smol-ninja changed the title ~~Refactor/withdraw amount~~ refactor: amount param in _withdraw Sep 10, 2024

andreivladbrg added 2 commits September 11, 2024 00:39

perf: remove redudant assert

b7c3457

Shub's feedback

a8b77f5

andreivladbrg and others added 3 commits September 11, 2024 17:01

test: remove redundant lines in withdraw handler

7fd0f16

test: change token symbols

undeeded imports

b77bc17

test: add getRenormalizedAmount in Utils.sol

ea68666

test: assertions in withdrawMax fuzz test

smol-ninja requested changes Sep 12, 2024

View reviewed changes

smol-ninja and others added 5 commits September 12, 2024 11:54

test: set lower bound for timejump to 0 in fuzz and invariant

21ac7f4

remove comment

9adf0e5

test: lower bound for rps

2c59bd4

refactor: remove corrected time logic (#236)

345f8df

chore: polish

0c7a3b1

smol-ninja mentioned this pull request Sep 13, 2024

Polish withdraw tree #238

Open

smol-ninja approved these changes Sep 13, 2024

View reviewed changes

chore: polish

9e13ab9

andreivladbrg merged commit 40dd6a0 into main Sep 13, 2024

andreivladbrg deleted the refactor/withdraw-amount branch September 13, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `amount` param in `_withdraw` #232

refactor: `amount` param in `_withdraw` #232

andreivladbrg commented Sep 9, 2024 •

edited

Loading

smol-ninja commented Sep 9, 2024

smol-ninja left a comment

andreivladbrg commented Sep 11, 2024

smol-ninja left a comment

smol-ninja commented Sep 13, 2024

smol-ninja left a comment

andreivladbrg commented Sep 13, 2024 •

edited

Loading

	// Check: the withdraw amount is not zero.
	if (amount == 0) {
	revert Errors.SablierFlow_WithdrawAmountZero(streamId);
	}

	// Check: the withdraw amount is not greater than the withdrawable amount.
	if (amount > withdrawableAmount) {
	revert Errors.SablierFlow_Overdraw(streamId, amount, withdrawableAmount);
	}

refactor: amount param in _withdraw #232

refactor: amount param in _withdraw #232

Conversation

andreivladbrg commented Sep 9, 2024 • edited Loading

Closes

Changes

smol-ninja commented Sep 9, 2024

smol-ninja left a comment

Choose a reason for hiding this comment

andreivladbrg commented Sep 11, 2024

smol-ninja left a comment

Choose a reason for hiding this comment

smol-ninja commented Sep 13, 2024

smol-ninja left a comment

Choose a reason for hiding this comment

andreivladbrg commented Sep 13, 2024 • edited Loading

refactor: `amount` param in `_withdraw` #232

refactor: `amount` param in `_withdraw` #232

andreivladbrg commented Sep 9, 2024 •

edited

Loading

andreivladbrg commented Sep 13, 2024 •

edited

Loading