GEN-1630: increase test coverage by fedealconada · Pull Request #12 · sense-finance/auto-roller

fedealconada · 2022-11-08T19:02:08Z

ignore files on Codecov
add tests
fix typos
add comment on getMaturityBounds

codecov · 2022-11-08T19:10:42Z

Codecov Report

Merging #12 (aa29a35) into main (b3f33b7) will increase coverage by 12.91%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main      #12       +/-   ##
===========================================
+ Coverage   69.87%   82.79%   +12.91%     
===========================================
  Files           9        8        -1     
  Lines         405      372       -33     
  Branches       66       66               
===========================================
+ Hits          283      308       +25     
+ Misses         98       44       -54     
+ Partials       24       20        -4

Impacted Files	Coverage Δ
src/AutoRoller.sol	`92.79% <ø> (+10.16%)`	⬆️
src/test/AutoRoller.t.sol	`100.00% <ø> (ø)`
script/AutoRoller.s.sol
src/external/DateTime.sol	`62.00% <0.00%> (+2.00%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jparklev

Coverage is awesome – will check back in again tomorrow in case there's any new discussion 👍

jparklev · 2022-11-08T19:10:48Z


 * `RollerPeriphery` - the slippage protected LP management interface to all AutoRollers.
-* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`.
+* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must **override** `getMaturityBounds` so only the roller can sponsor preventing a potential DoS attack.


Suggested change

* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must **override** `getMaturityBounds` so only the roller can sponsor preventing a potential DoS attack.

* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must **override** `getMaturityBounds` so only the roller can sponsor, preventing a potential DoS attack.

jparklev · 2022-11-08T19:13:41Z

            balances[1 - _pti] = targetToJoin;

-            if (assets - targetToJoin > 0) { // Assumption: this is false if Space has only Target liquidity.
+            if (assets - targetToJoin > 0) { // Assumption: this is false if Space has only Target liquidity. TODO: can this ever happen?


Perhaps not if we don't allow the pool to be initialized at 0%!

But is this something we should keep just in case?

I think so, not much harm in being safe here besides bloat, but i'll think about it as I continue audit resolutions

jparklev · 2022-11-10T06:41:30Z

+        vm.stopPrank();
+
+        (, bytes32[] memory writes) = vm.accesses(address(autoRoller));
+        // Check that no storage slots were written to


Suggested change

// Check that no storage slots were written to

// Check that the expected number of storage slots were written to

jparklev · 2022-11-10T06:42:20Z

+
+    function testAfterDepositWithOnlyTargetLiquidity() public {
+        // The idea is making a test that covers line case on the `afterDeposit` function
+        // where `if (assets - targetToJoin > 0)` is false.


This is true, because even if we have someone try to buy all of the PTs out of the pool, I don't think it's possible to get it down to true 0 after initialization

perfect, I'll remove all this

jparklev · 2022-11-10T06:42:58Z

+        autoRoller.deposit(0.2e18, address(this));
+        assertEq(autoRoller.balanceOf(address(this)), 0.2e18);
+
+        // 3. Roll the Target into the first Series.


Suggested change

// 3. Roll the Target into the first Series.

// 2. Roll the Target into the first Series.

jparklev · 2022-11-10T06:43:43Z

+
+        // 5. Redeem all shares while still in the active phase.
+        uint256 targetBalPre = target.balanceOf(address(this));
+        vm.expectCall(address(periphery), abi.encodeWithSelector(periphery.swapYTsForTarget.selector));


Nice check here 👌

jparklev · 2022-11-10T06:45:56Z

+        vm.expectCall(address(periphery), abi.encodeWithSelector(periphery.swapYTsForTarget.selector));
+        autoRoller.redeem(autoRoller.balanceOf(address(this)), address(this), address(this));
+        uint256 targetBalPost = target.balanceOf(address(this));
+        // assertRelApproxEq(targetBalPost - targetBalPre, 0.5e18, 0.0001e18 /* 0.01% */); // TODO: what should it be the expected value?


I think we should expect it to be pretty close to "total deposited amount - firstDeposit" (since firstDeposit is kept locked in the contract forever, similar to the min bpt for our space pool). What were you seeing here for this val?

hmm yeah, i was seeing something similar (I forgot to consider the firstDeposit) but now, considering the firstDeposit:

I have deposited in total 0.5e18 and I'm asserting that 0.5e18 - 0.01e18 (total deposited - firstDeposit) should almost equal to the target I received back which is 499642339889372330, shouldn't that be lower? Or how do I establish which is a good delta in this case?

Ahh yea, after you roll the math starts to get a little more complicated I think. You can always do a previewRedeem here to see what it should be out, or you can walk through the trace to see what it's doing. I might do the latter today or tomorrow just out of curiosity

jparklev · 2022-11-10T06:47:08Z

+
+        vm.warp(autoRoller.maturity());
+
+        // 4. Increase scale and settle the first Series.


Did we want to manually increase scale somewhere here?

no no, it's not needed

Why not? 😅

jparklev · 2022-11-10T06:48:32Z

+
+        // 6. Assert max redeem is same as TODO.
+        sharesOut = autoRoller.maxRedeem(address(this));
+        // assertEq(sharesOut, TODO);


think we could do assertEq(autoRoller.maxRedeem(address(this)), sharesOut); again here perhaps?

oh, it works :), i would have sworn i tried that and was not working 🤔 and that;s why i was confused

jparklev · 2022-11-10T06:53:55Z

+                kind: BalancerVault.SwapKind.GIVEN_IN,
+                assetIn: address(pt),
+                assetOut: address(autoRoller.asset()),
+                amount: 0.01e18, // TODO: how do I calculate a number such that it would make the maxPTSale >= diff? This number works though


So, the only case where this isn't true IIRC is if order we'd have to swap here such that the pool becomes very close to the maxRate value (time shifted) so that the user can't safely make the PT in swap needed to withdraw their position without exceeding the max rate

Note that you can check the rate "(pt reserves + total supply / (target reserve * scale))"

chore: add more tests

4e5a87b

fedealconada force-pushed the GEN-1630-more-tests branch from 6990830 to 4e5a87b Compare November 8, 2022 19:03

chore: fix ignore

ea91ae1

fedealconada force-pushed the GEN-1630-more-tests branch 3 times, most recently from 0aa5c0b to 6ced5fd Compare November 9, 2022 14:02

chore: polish some tests

a31f1d4

fedealconada force-pushed the GEN-1630-more-tests branch from 6ced5fd to a31f1d4 Compare November 9, 2022 14:21

jparklev reviewed Nov 10, 2022

View reviewed changes

chore: polish tests II

aa29a35

fedealconada force-pushed the GEN-1630-more-tests branch from 1313180 to aa29a35 Compare November 14, 2022 11:03

	* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must override `getMaturityBounds` so only the roller can sponsor preventing a potential DoS attack.
	* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must override `getMaturityBounds` so only the roller can sponsor, preventing a potential DoS attack.

	// Check that no storage slots were written to
	// Check that the expected number of storage slots were written to

	// 3. Roll the Target into the first Series.
	// 2. Roll the Target into the first Series.


		vm.warp(autoRoller.maturity());

		// 4. Increase scale and settle the first Series.

Conversation

fedealconada commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jparklev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedealconada Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fedealconada commented Nov 8, 2022 •

edited

Loading

codecov Bot commented Nov 8, 2022 •

edited

Loading

fedealconada Nov 11, 2022 •

edited

Loading