Add chainlink keepers interface for rebalancing #92

FlattestWhite · 2022-02-02T06:11:38Z

First implementation of FliRebalanceKeeper.

For the FlexibleLeverageExtension, we need to remove the nonEOA and probably add nonRetrancy guard or something.

ncitron

Good start! Left a few comments.

ncitron · 2022-02-10T05:02:01Z

contracts/keepers/FliRebalanceKeeper.sol

+    }
+
+    function getRebalanceCalldata() private returns (bytes memory) {
+        bytes memory shouldRebalanceCalldata = abi.encodeWithSignature("shouldRebalance()");


Can we create a new interface called for the leverage extension so we don't need to manually encode with strings here?

ncitron · 2022-02-10T05:03:17Z

contracts/keepers/FliRebalanceKeeper.sol

+        bytes memory shouldRebalanceResponse = Address.functionCall(address(fliExtension), shouldRebalanceCalldata, "Failed to execute shouldRebalance()");
+        (string[] memory exchangeNames, uint256[] memory shouldRebalances) = abi.decode(shouldRebalanceResponse, (string[], uint256[]));
+
+        uint256 shouldRebalance = shouldRebalances[0];


We might want to be able to select what exchange to use. Maybe we can store what index to use somewhere?

Shouldn't the fliExtension contract determine which exchange to use? Rather than this contract? Or are you suggesting to have a whitelist of exchanges that this contract can call?

At the moment we allow the caller of the FLI extension to determine what exchange to use (of a list of whitelisted exchanges). This is because determining the optimal exchange may be difficult or gas extensive to do on-chain. Lately we have just been hardcoding our bots to use the exchange we want, with a backup exchange already whitelisted in case we need to make changes quickly. For this contract since we have a bit less trust in the chainlink keepers, we probably want to hard code which exchange to use.

Hmmm - then we keep a list of allowable addresses that we can contact? What's the exchangeName that comes back from the fliExtension call - I'm assuming it's an address?

The exchange name is actually just a string which corresponds to the adapter that is used to do the trade part of the rebalance. I think its probably best to hardcode the index of the array that we want to use (and allow the owner of the contract to change it).

ncitron · 2022-02-10T05:04:51Z

contracts/keepers/FliRebalanceKeeper.sol

+
+        uint256 shouldRebalance = shouldRebalances[0];
+        if (shouldRebalance == 1) {
+            return abi.encodeWithSignature("rebalance(string)", exchangeNames[0]);


If we had created an interface like in my previous comments, we could replace this with

abi.encodeWithSelector(fliExtension.rebalance.selector, exchangeNames[0])

Which is a bit cleaner

ncitron · 2022-02-10T05:08:22Z

contracts/keepers/FliRebalanceKeeper.sol

+     * As checkUpkeep is not a view function, calling this function will actually consume gas.
+     * As such if a keeper calls this function, it will always return true so that performUpkeep will be called.
+     */    
+    function checkUpkeep(bytes calldata /* checkData */) external override onlyRegistry returns (bool, bytes memory) {


Even though checkUpkeep isn't a view function, it is called as if it were by the keepers. This is because you can call a non-view method and it runs as if it were executed, except state changes are not reflected. We can then just have this function do the same thing as getRebalanceCalldata. This will save us on gas since it can be run off-chain.

Finally I don't think we need to make this onlyRegistry since it won't perform any gatekeeped actions.

done - removed it

ncitron · 2022-02-10T05:08:39Z

contracts/keepers/FliRebalanceKeeper.sol

@@ -0,0 +1,79 @@
+/*
+    Copyright 2021 Index Cooperative.


FlattestWhite · 2022-02-16T07:37:31Z

ready for re-review @ncitron added the exchange index parameter

ncitron

Looking pretty good. I just noticed one major change related to how we want to pass data between checkUpkeep and performUpkeep. Also we need to add support for allowing smart contracts to make rebalance calls on AaveLeverageStrategyExtension.

Everything else is looking pretty good. Great job on the tests.

ncitron · 2022-02-21T07:41:11Z

contracts/keepers/FliRebalanceKeeper.sol

+    address public registryAddress;                                 // Address of the chainlink keeper registry
+    uint256 public exchangeIndex;                                   // The index of the exchange to use
+
+    /* ============ Constructor ============ */


nit: newline

ncitron · 2022-02-21T07:42:39Z

contracts/keepers/FliRebalanceKeeper.sol

+
+    function getRebalanceCalldata() private returns (bytes memory) {
+        bytes memory shouldRebalanceCalldata = abi.encodeWithSelector(fliExtension.shouldRebalance.selector);
+        bytes memory shouldRebalanceResponse = Address.functionCall(address(fliExtension), shouldRebalanceCalldata, "Failed to execute shouldRebalance()");


We don't need to manually create this calldata. fliExtension.shouldRebalance() will work.

Also can we use shouldRebalanceWithBounds instead and allow bounds to be stored in a storage struct with a function for the owner to update it? The reason this function is better is it allows us to prevent reverts. For example, if we are set to rebalance at 2.2x leverage, we might fire off a tx, just to watch the leverage ratio decrease to 2.199x by the time it is included in a block. Since this is below the max leverge. The tx will revert. On polygon, this isn't a big deal, but on mainnet these reverts add up gas-wise. If we do it with shouldRebalanceWithBounds we can set the upper bound to 2.25x, meaning it wont revert if the leverage ration decreases slightly between the time we broadcast the tx and when it is actually included.

Added a LeverageSettings struct with options for bounds and added it as part of the constructor

ncitron · 2022-02-21T07:48:44Z

contracts/keepers/FliRebalanceKeeper.sol

+     */
+    function performUpkeep(bytes calldata performData) external override onlyRegistry {
+        require(performData.length > 0, "Invalid performData");
+        Address.functionCall(address(fliExtension), performData);


This line scares me a bit since it allows a keeper to perform any call on fliExtension. While I don't think this is an immediate security concern here, I still don't love it since this contract has extra privileges with fliExtension. Can we instead have checkUpkeep return the shouldRebalance enum calculated on line 76. Then we can ensure that this contract can only ever do one of the three rebalance functions instead of any arbitrary call.

Done - made the checkUpkeep return an encoded shouldRebalance enum and exchangeName. This is then decoded in performUpkeep before sending a transaction off to the fliExtension

ncitron · 2022-02-21T07:49:49Z

contracts/keepers/FliRebalanceKeeper.sol

+    constructor(IFlexibleLeverageStrategyExtension _fliExtension, address _registryAddress) public {
+        fliExtension = _fliExtension;
+        registryAddress = _registryAddress;
+        exchangeIndex = 0;


Can we allow exchangeIndex to be set in the constructor?

ncitron

This all looks great. We still need to update AaveLeverageStrategyExtension to allow non-EOA accounts to call it. I think the best way to do this is to update https://github.com/IndexCoop/index-coop-smart-contracts/blob/master/contracts/lib/BaseExtension.sol#L59 to check some stored flag that determines whether we want to skip the EOA check. Similar to how onlyAllowCaller works here https://github.com/IndexCoop/index-coop-smart-contracts/blob/master/contracts/lib/BaseExtension.sol#L145

FlattestWhite · 2022-03-01T07:17:24Z

This all looks great. We still need to update AaveLeverageStrategyExtension to allow non-EOA accounts to call it. I think the best way to do this is to update https://github.com/IndexCoop/index-coop-smart-contracts/blob/master/contracts/lib/BaseExtension.sol#L59 to check some stored flag that determines whether we want to skip the EOA check. Similar to how onlyAllowCaller works here https://github.com/IndexCoop/index-coop-smart-contracts/blob/master/contracts/lib/BaseExtension.sol#L145

Added a boolean to allow us to bypass the onlyEOA check. Although from the documentation, the check is there to stop sandwich/flash loan attacks. Wouldn't flipping the flag open us up to these type of attacks? Do we need to store an allowlist of contract addresses that are allowed to call this function?

FlattestWhite · 2022-03-24T02:53:09Z

🎉 This PR is included in version 0.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

FlattestWhite requested a review from ncitron February 9, 2022 05:13

ncitron reviewed Feb 10, 2022

View reviewed changes

FlattestWhite force-pushed the richard-chainlink-keepers-1 branch from 1250537 to 26b227b Compare February 10, 2022 19:31

FlattestWhite requested a review from ncitron February 13, 2022 21:56

ncitron reviewed Feb 21, 2022

View reviewed changes

FlattestWhite requested a review from ncitron February 22, 2022 05:44

ncitron reviewed Feb 23, 2022

View reviewed changes

ncitron approved these changes Mar 18, 2022

View reviewed changes

FlattestWhite added 20 commits March 18, 2022 17:57

add keeper

040e624

some progress

5e3d527

fix pragma

f7de80f

renaming

950e180

add nls

ffa7c10

remove comment around performData

85899e6

it compiles

b1dd3d1

linting rules

3386a86

add deploy utilities

2a78c19

.

b683ca5

add mock

ccc95c9

formatting

12612dc

added tests

aaff1af

reorder imports

b7b3239

remove console imports

247ac3a

update comment

c8bcc05

removed comments

1f4362c

address review comments

8fc76d9

review comments and fixed tests

9e90ecb

add exchange index parameter

dbfae6f

FlattestWhite added 3 commits March 18, 2022 17:58

changes requested from review

938a257

add flag for bypassing onlyEOA check and tests

7c82bfd

formatting

3fc0268

FlattestWhite force-pushed the richard-chainlink-keepers-1 branch from 2cc9230 to 3fc0268 Compare March 18, 2022 23:58

FlattestWhite added 2 commits March 18, 2022 17:59

fix conflicts

8ef9ffa

remove base extension changes

95a21de

FlattestWhite merged commit 4bb8242 into master Mar 19, 2022

FlattestWhite deleted the richard-chainlink-keepers-1 branch March 19, 2022 00:15

FlattestWhite added the released label Mar 24, 2022

ace3 pushed a commit to ace3/index-coop-smart-contracts that referenced this pull request Feb 2, 2024

Update package for public npm (make ethers a non-dev dep) (IndexCoop#92)

20c1b74

Add chainlink keepers interface for rebalancing #92

Add chainlink keepers interface for rebalancing #92

Uh oh!

Conversation

FlattestWhite commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncitron 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!

FlattestWhite commented Feb 16, 2022

Uh oh!

ncitron 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!

ncitron left a comment

Choose a reason for hiding this comment

Uh oh!

FlattestWhite commented Mar 1, 2022

Uh oh!

FlattestWhite commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FlattestWhite commented Feb 2, 2022 •

edited

Loading