Revert on zero amounts #636

grandizzy · 2023-02-24T10:38:20Z

validate user input and revert in case of noops
added Pool.updateInterest function to be called by actors wanting to update interest rate
added Pool.stampLoan function to be called by borrowers for restamping Neutral Price of their loan
- added IPoolBorrowerActions interface for common stampLoan function
- added Pool.stampLoan to accrue interest and stamp Neutral price of the loan
- added BorrowerActions.stampLoan to update Neutral Price of the loan: reverts if loan in auction or borrower under collateralized
- unit tests
renamed MoveToSamePrice error to MoveToSameIndex
introduced InvalidAmount error, use to revert on calling functions with 0 amounts (add/remove quote tokens and collateral, take auction and take reserve auction, draw / repay debt)
added input validation tests

contract size with change

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  22,771B  (92.65%)
  ERC20Pool                -  22,484B  (91.48%)
  Auctions                 -  20,272B  (82.48%)

vs

============ Deployment Bytecode Sizes ============
  ERC721Pool               -  22,444B  (91.32%)
  ERC20Pool                -  22,125B  (90.02%)
  Auctions                 -  20,172B  (82.08%)

grandizzy · 2023-02-24T11:25:40Z

tests/forge/ERC20Pool/ERC20PoolPrecision.t.sol

@@ -522,7 +523,8 @@ contract ERC20PoolPrecisionTest is ERC20DSTestPlus {
 assertEq(bucketLpBalance, lenderLpBalance + bidderLpBalance);
 }

- function testDepositTwoLendersSameBucket(
+ // FIXME: the scaled amounts are always 0
+ function _testDepositTwoLendersSameBucket(


@EdNoepel can you pls check why the calculated scaledQuoteAmount1 and scaledQuoteAmount2 are always 0? It looks like the test is not properly configured. Same for _testDepositTwoActorSameBucket above

nm, I think I solved that by setting the amount to the dust limit if lower than

mattcushman

i think these are all fine changes to make. Most apply to token quantities where passing a 0 is a null op. In one case it's applied to a loop limit which is a somewhat different case, but I don't think it harms anything there either as the caller can always pass a value >0 as well and get more or less identical results. Philosophically I still really don't think this buys anything and just clutters the code, but I don't think it causes any harm either.

src/libraries/external/Auctions.sol

EdNoepel

Generally happy with this. Just a question regarding borrower updating neutral price, and a removed test.

tests/forge/ERC20Pool/ERC20PoolPrecision.t.sol

EdNoepel · 2023-02-28T00:28:06Z

tests/forge/ERC20Pool/ERC20PoolBorrow.t.sol

@@ -462,15 +457,14 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {
 borrower: _borrower,
 borrowerDebt: expectedDebt,
 borrowerCollateral: 50 * 1e18,
- borrowert0Np: 448.381722115384615591 * 1e18,
+ borrowert0Np: 445.838278846153846359 * 1e18,


Conceivably, borrowing zero (old line 435) was a way to update the borrower's neutral price. What's the new mechanism for doing so? Drawing or repaying 1 wei or whatever the dust limit is?

Must admit I don't understand the mechanism of updating NP by borrowing 0, can you explain what's the scenario that is used for? Maybe if we need such out of context of draw/repay debt we should expose an updateNP function for borrowers?

My understanding of this is rudimentary. A borrower's neutral price is a function of the MOMP and LUP, which change over time. Before a borrower is liquidated, I thought the borrower had the right to restamp their neutral price if conditions were favorable to do so. That's what BorrowerActions old line 125 was permitting, with the restamp happening on old line 219.

Off topic: I do not understand why the borrower needs a neutral price stamp at all. Seems it would be easier to stamp that on the liquidation upon kick and calculate it on-the-fly until then.

thanks! @mattcushman can you please share your thoughts on this matter?

The reason for stamping the neutral price on the loan at borrowing is to provide some certainty to the borrower as to at what price they can expect to be liquidated. If we made it entirely dynamic, it would invite kickets to try to lower the dynamic measure of price in such a way to make kicking the loans profitable. We struggled to find a good dynamic measure, but this made the most sense over the lifetime of the loan.

And yes, the borrower should be able to restamp their NP as long as they are fully collateralized. THey can always trivially do so by removing 1 unit of debt or collateral.

sure, can add such function

@mattcushman should we allow other parties but borrower to restamp the loan? I think we should as we're currently allowing this as draw / repay debt can be invoked by different actors but want to get your confirmation on this

added with e07f6bd please review

per our discussion 08b8943 removes the ability of 3rd parties to restamp Neutral Price of loan

…f the loan

src/interfaces/pool/commons/IPoolEvents.sol

…kets (settle only with reserves)

MikeHathaway · 2023-03-01T05:36:33Z

src/libraries/external/BorrowerActions.sol

+ ) external returns (
+ uint256 newLup_
+ ) {
+ address borrowerAddress = msg.sender;


since borrowerAddress is always msg.sender, can we just remove that local variable entirely to save a bit of gas (CALLER vs MLOAD: https://stackoverflow.com/questions/64854256/how-much-accessing-msg-sender-costs-is-it-useful-to-store-it-in-a-variable-and)?

Good call, will do

changed with 55f44a1

MikeHathaway · 2023-03-01T05:47:38Z

src/libraries/external/BorrowerActions.sol

+ newLup_ = _lup(deposits_, poolState_.debt);
+
+ uint256 borrowerDebt = Maths.wmul(borrower.t0Debt, poolState_.inflator);
+ bool isCollateralized = _isCollateralized(borrowerDebt, borrower.collateral, newLup_, poolState_.poolType);


Ugly but might want to also remove borrowerDebt local variable for gas savings since its only used once

removed all local vars with d82eda7

MikeHathaway

LGTM - just had a few comments about potential gas optimizations

Please review again with latest changes

EdNoepel

LGTM

Take 1

c276e77

grandizzy added Priority: High Status: Work In Progress labels Feb 24, 2023

grandizzy force-pushed the revert-on-zero-amounts branch from 99745df to 10b891e Compare February 24, 2023 10:54

Merge remote-tracking branch 'origin' into revert-on-zero-amounts

5f143f6

grandizzy force-pushed the revert-on-zero-amounts branch from 10b891e to 5f143f6 Compare February 24, 2023 11:17

grandizzy requested a review from EdNoepel February 24, 2023 11:23

grandizzy commented Feb 24, 2023

View reviewed changes

Fix fuzz tests

71568b5

grandizzy force-pushed the revert-on-zero-amounts branch from 9263e04 to 71568b5 Compare February 24, 2023 14:08

grandizzy added Status: Ready for Review and removed Status: Work In Progress labels Feb 24, 2023

grandizzy marked this pull request as ready for review February 24, 2023 14:11

grandizzy requested review from ith-harvey, MikeHathaway, prateek105 and mattcushman February 24, 2023 14:11

mattcushman previously approved these changes Feb 27, 2023

View reviewed changes

src/libraries/external/Auctions.sol Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin' into revert-on-zero-amounts

51e0182

EdNoepel reviewed Feb 28, 2023

View reviewed changes

grandizzy requested review from EdNoepel and mattcushman February 28, 2023 06:36

grandizzy added Scope: Style/Consistency Status: Work In Progress and removed Status: Ready for Review labels Feb 28, 2023

grandizzy added 2 commits February 28, 2023 16:27

Add stampLoan for borrowers to be able to restamp the Neutral Price o…

e07f6bd

…f the loan

Remove ability of different actors updating Neutral Price of loans

08b8943

grandizzy force-pushed the revert-on-zero-amounts branch from 84e6870 to 08b8943 Compare February 28, 2023 14:50

EdNoepel reviewed Feb 28, 2023

View reviewed changes

src/interfaces/pool/commons/IPoolEvents.sol Show resolved Hide resolved

Style and grouping

05826d6

Remove bucket depth constraint, add test for settle called with 0 buc…

e87949c

…kets (settle only with reserves)

grandizzy requested a review from EdNoepel March 1, 2023 03:50

grandizzy added Status: Ready for Review and removed Status: Work In Progress labels Mar 1, 2023

MikeHathaway reviewed Mar 1, 2023

View reviewed changes

MikeHathaway approved these changes Mar 1, 2023

View reviewed changes

grandizzy added 2 commits March 1, 2023 07:49

Use msg.sender instead local var for gas improvements

55f44a1

Remove local vars used only once for gas savings

d82eda7

EdNoepel approved these changes Mar 1, 2023

View reviewed changes

grandizzy merged commit 937c6ba into develop Mar 1, 2023

grandizzy deleted the revert-on-zero-amounts branch March 1, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert on zero amounts #636

Revert on zero amounts #636

grandizzy commented Feb 24, 2023 •

edited

Loading

grandizzy Feb 24, 2023

grandizzy Feb 24, 2023

mattcushman left a comment

EdNoepel left a comment

EdNoepel Feb 28, 2023

grandizzy Feb 28, 2023 •

edited

Loading

EdNoepel Feb 28, 2023

grandizzy Feb 28, 2023

mattcushman Feb 28, 2023

grandizzy Feb 28, 2023

grandizzy Feb 28, 2023

grandizzy Feb 28, 2023

grandizzy Feb 28, 2023 •

edited

Loading

mattcushman Mar 1, 2023

MikeHathaway Mar 1, 2023 •

edited

Loading

grandizzy Mar 1, 2023

grandizzy Mar 1, 2023 •

edited

Loading

MikeHathaway Mar 1, 2023

grandizzy Mar 1, 2023

MikeHathaway left a comment

EdNoepel left a comment

Revert on zero amounts #636

Revert on zero amounts #636

Conversation

grandizzy commented Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcushman left a comment

Choose a reason for hiding this comment

EdNoepel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHathaway Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHathaway left a comment

Choose a reason for hiding this comment

EdNoepel left a comment

Choose a reason for hiding this comment

grandizzy commented Feb 24, 2023 •

edited

Loading

grandizzy Feb 28, 2023 •

edited

Loading

grandizzy Feb 28, 2023 •

edited

Loading

MikeHathaway Mar 1, 2023 •

edited

Loading

grandizzy Mar 1, 2023 •

edited

Loading