FIXED rebalance extension

[ckoopmann]

Extension / Adapter to rebalance FIXED products and maintain target allocation among fCash maturities.

[coveralls]

Coverage decreased (-3.3%) to 82.369% when pulling ef1a7f3b5d40d2c82da8d13d04ac00e0852f5dff on fixed-rebalance-extension into c00d1719496b1eb6a91ece8dc50262a30bf4cb1e on master.

[ckoopmann]

Before making a detailed review of this PR, I've got a few questions to answer:

1. How do you envision this contract to fit into our existing keeper system. Currently, I've got this lambda that continuously calls the extension contract method shouldRebalance() and depending on the response either calls rebalance(), iterateRebalance() or ripcord(). The difference with them is that the caller only needs to specify the ExchangeAdapter that they need to be called with. How are _share and _minPositions calculated by the caller?
2. How often should rebalance be called? Is a cooldown period?
3. How does a keeper know not to call rebalance?

From my understanding we will only call these rebalance methods when one of the underlying fcash tokens matures. (so every three months). So I don't think we need to necessarily integrate this into the keeper system at all but can call it manually once every three months. (the name "RebalanceExtension" might be a bit misleading here since we are not planning to do the kind of continous rebalancing that we do for some of our other products - although the contract would support rebalancing at any point)

_minPositions can be calculated by simulating the rebalance call checking the positions afterwards and then subtracting a few percentage points. This is basically just for frontrunning protection.

_share allows us to split the rebalance trade over multiple transactions to limit price impact. I'm not sure if this is an issue at all and either way wont be one until we reach significant AUM. So for the beginning we can probably just set it to "1.0".

[ckoopmann]

Before making a detailed review of this PR, I've got a few questions to answer:

1. How do you envision this contract to fit into our existing keeper system. Currently, I've got this lambda that continuously calls the extension contract method shouldRebalance() and depending on the response either calls rebalance(), iterateRebalance() or ripcord(). The difference with them is that the caller only needs to specify the ExchangeAdapter that they need to be called with. How are _share and _minPositions calculated by the caller?
2. How often should rebalance be called? Is a cooldown period?
3. How does a keeper know not to call rebalance?

From my understanding we will only call these rebalance methods when one of the underlying fcash tokens matures. (so every three months). So I don't think we need to necessarily integrate this into the keeper system at all but can call it manually once every three months. (the name "RebalanceExtension" might be a bit misleading here since we are not planning to do the kind of continous rebalancing that we do for some of our other products - although the contract would support rebalancing at any point)

_minPositions can be calculated by simulating the rebalance call checking the positions afterwards and then subtracting a few percentage points. This is basically just for frontrunning protection.

_share allows us to split the rebalance trade over multiple transactions to limit price impact. I'm not sure if this is an issue at all and either way wont be one until we reach significant AUM. So for the beginning we can probably just set it to "1.0".

Note that if we at some point did decide to do "continous rebalancing" (which we currently don't plan to do). Then the keeper could determine wether rebalancing is needed by calling getUnderweightPositions, and using a certain minimum threshold. (since there will always be some minor deviation from the target allocations due to fluctuations in interest rates).

[ckoopmann]

just to confirm - it looks like we'd need to deploy an extension contract per FIXED product that i.e. FIXED-DAI and FIXED-USDC will each need their own extension contract

Yes, we would deploy one instance for each FIXED product. I assumed this was standard for our extensions. (Please correct me if i'm wrong).

Note that the smart contract code should work for any FIXED product. I will extend the tests to run for all of our fixed products to make sure that is actually the case.

[ckoopmann]

Testing

• Overall consistent and I like what @FlattestWhite has suggested. When we read the output from a test it should make sense without much context.
• Further more on naming, I understand that you created the context of some tests by the name of the function and the test names as "should work". Being more descriptive on these names would help others understand the context in which a user would care to call the function or how it is crucial to user requirement. I recognize this as a nit for development but these tests provide such awesome documentation to power users/devs and should strive for as good as possible.

I agree "should work" is a terrible test name. I replaced all of its occurences with more informative names.

Coverage

I think 100% test coverage is a bad requirement but we should make an effort to understand the coverage our tests provide and have it guide discussions.

# I utilized this command to get the following results. All tests passed. COVERAGE=true INTEGRATIONTEST=true FORK=true VERBOSE=true node --max-old-space-size=4096 ./node_modules/.bin/hardhat coverage --testfiles test/integration/ethereum/fixedRebalanceExtension.spec.ts

File % Stmts % Branch % Funcs % Lines Uncovered Lines FixedRebalanceExtension.sol 93.02 62.5 92.59 93.02 416,417,420

• _getFCashComponentsAndMaturities and _absoluteToRelativeMaturity are internal functions of the extension that are not covered by these tests. Removing them, increased all but branch coverage to 100%. The compiler should remove these functions from the deployed bytecode if they are never referenced. I suggest being more explicit and removing the unused code.
• Testing every branch (especially in internal functions) is not super important to me, but understanding the gap is important. As a general rule, the else case of a require statement is already well tested (the call reverts). Branches caused by if statements are harder to assess for criticality of appropriate coverage.

I removed the unused methods and reached 100% line coverage. I looked at the missed branches added some extra coverage where it was worth the effort, but I agree we don't necessarily need 100% branch coverage.

Also I tried to add a CI step that reports the coverage from the integration test but it ended up overwriting the coverage data from the unit tests so I removed it again (see: https://github.com/IndexCoop/index-coop-smart-contracts/pull/122/commits/a7a8c0cba1d8a9da32b070db9488fb4f496cf261) . Would be great if we could find a way to aggregate those results somehow.

• This Extension has two if branches not covered by existing tests, Lines 219 and 432. Make sure that the else paths are reachable and if reached, the expected behavior would still take place.
• "access control" require statements and modifiers are worth having coverage for integration tests but more focused on the state changes that might affect access. For example, we might want to test us changing the operator and ensuring that the previous operator could no longer take privileged actions. Looks like this contract makes use of the onlyOperator modifier that presumably has been tested elsewhere, so we can assume it works but I still see it as such a critical path that we should make certain that this path works as expected.

Added a test that checks that operator only method is not available to original operator anymore after the operator was changed: https://github.com/IndexCoop/index-coop-smart-contracts/pull/122/commits/c6a9b669d1ac0f6c4487abe35fd030d8b3c1a1f7

• "Invalid state" require statements, I think merit integration testing if it is reasonable that such branch would be taken or if removing the condition would be catastrophic for assumptions further in the code.

  • Take for example: function rebalance(uint256 _share, uint256[] memory _minPositions) and the require statements require(_share > 0, "Share must be greater than 0"); require(_share <= 1 ether, "Share cannot exceed 100%"); In my testing removing these statements did not make any test fail and these calls are restricted to a privileged account. I therefore wouldn't worry about coverage for these cases.

I added tests that check for failures if these conditions are violated:

• All branches that are not covered seem to be internal and none of them are exposed through external functions accessible by unprivileged account. So no need to block on branch coverage.

I agree remaining missed branches don't seem to be security critical. Some of them are only added to avoid undefined behaviour if called on a set token that has a negative (i.e. debt) fCash position, which is a bit tricky to produce in tests and probably not worth the effort.

Security

• Seems like the only security assumptions that exist are that of a malicious operator due to all state changing functions being restricted. Unless there is a requirement to be less restrictive on who can modify the extension's state, I think this is appropriately secure enough. Further locking down the operator to reasonable checks would be great though. Is there not a way to get the valid maturities/ allocations from the notional protocol given the setToken passed in?

Actually the "validMaturities" is not needed really needed so I removed it (I did use this before in the _absoluteToRelativeMaturity function which I removed as per your suggestion above)

Note that the maturities that the operator sets are "relative" maturities (i.e. "3 months" ) whereas the notional contracts alwasy operate with "absolute maturities" (i.e. "2022-12-22 0:00"). To convert from one to the other the contract goes through all of the active markets on notional and selects the market with the longest absoluteMaturity that is less than relativeMaturity away from the current blocktime (see _relativeToAbsoluteMaturity for implementation)

Therefore any absolute maturity it returns will always be a valid active notional market. So in that sense we are already getting the valid maturities from notional contracts.

• We might want to consider reentrancy in the external functions. At a glance though and with my understanding of the Set Protocol, we shouldn't need to worry about that.
• We should be explicit about visibility for state variables. Most seem to be internal which is a good default, but I think some of these would make sense to be public and have the default getters exposed. Furthermore, it seems like almost all the values passed in to the constructor are immutably set to storage and if declared as immutable could be more efficient.

Agree with your assesment regarding security issues. We might want to review some of them (such as reentrancy) if we decide to open up some of those methods to be called by anyone.

[FlattestWhite]

:tada: This PR is included in version 0.12.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: