Bridging buffer manager

[juuroudojo]

closes #114

[pentatonictritones]

Some immediate things that have to be done:

1. You didn't npm i when you merged the master branch. This means that the git hooks that enforce pre-commit and pre-push rules were not active.

This has lead to you breaking many existing test files because you made extra changes to the resolver contracts + the correct formatting and linting were not done.

1) IbAlluoUSD and Handler "before each" hook for "Test upgradeability of original ibAlluo contract": Error: missing argument: in Contract constructor (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0) at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28) at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20) at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18) at ContractFactory. (node_modules\@ethersproject\contracts\src.ts\index.ts:1237:16) at step (node_modules\@ethersproject\contracts\lib\index.js:48:23) at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53) at E:\Coding Projects\liquidity-direction-protocol\node_modules\@ethersproject\contracts\lib\index.js:23:71 at new Promise () at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12) at ContractFactory.deploy (node_modules\@ethersproject\contracts\lib\index.js:1138:16)

2) IbAlluo and Exchange "before each" hook for "The exchange should be working normally: Check all swap combinations": Error: missing argument: in Contract constructor (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0) at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28) at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20) at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18) at ContractFactory. (node_modules\@ethersproject\contracts\src.ts\index.ts:1237:16) at step (node_modules\@ethersproject\contracts\lib\index.js:48:23) at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53) at E:\Coding Projects\liquidity-direction-protocol\node_modules\@ethersproject\contracts\lib\index.js:23:71 at new Promise () at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12) at ContractFactory.deploy (node_modules\@ethersproject\contracts\lib\index.js:1138:16)

3) Handler and different adapters "before each" hook for "Depositing 1 wbtc and immediately attempting to withdraw 0.5 should put you in the waiting list": Error: missing argument: in Contract constructor (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0) at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28) at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20) at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18) at ContractFactory. (node_modules\@ethersproject\contracts\src.ts\index.ts:1237:16) at step (node_modules\@ethersproject\contracts\lib\index.js:48:23) at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53) at E:\Coding Projects\liquidity-direction-protocol\node_modules\@ethersproject\contracts\lib\index.js:23:71 at new Promise () at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12) at ContractFactory.deploy (node_modules\@ethersproject\contracts\lib\index.js:1138:16)

4) IbAlluoUSD and Handler "before each" hook for "Test grabbing block number through timestamp using binary search": Error: missing argument: in Contract constructor (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0) at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28) at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20) at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18) at ContractFactory. (node_modules\@ethersproject\contracts\src.ts\index.ts:1237:16) at step (node_modules\@ethersproject\contracts\lib\index.js:48:23) at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53) at E:\Coding Projects\liquidity-direction-protocol\node_modules\@ethersproject\contracts\lib\index.js:23:71 at new Promise () at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12) at ContractFactory.deploy (node_modules\@ethersproject\contracts\lib\index.js:1138:16)

5) Superfluid resolver with StIbAlluo/IbAlluo "before each" hook for "When critical (8 hr or less buffer), checker should return true and liquidateSender should wrap ibAlluos to prevent liquidation.": Error: missing argument: in Contract constructor (count=3, expectedCount=4, code=MISSING_ARGUMENT, version=contracts/5.7.0) at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28) at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20) at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18) at ContractFactory. (node_modules\@ethersproject\contracts\src.ts\index.ts:1237:16) at step (node_modules\@ethersproject\contracts\lib\index.js:48:23) at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53) at E:\Coding Projects\liquidity-direction-protocol\node_modules\@ethersproject\contracts\lib\index.js:23:71 at new Promise () at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12) at ContractFactory.deploy (node_modules\@ethersproject\contracts\lib\index.js:1138:16)

Please fix these tests and next time make sure to run all tests before you push - though this is now taken care of with pre-push hooks running npx hardhat test.

It is important to see that other parts of the protocol are not broken by your changes.

[pentatonictritones]

About BufferManager.sol specifically, I will break it into two parts : The adapter refilling logic and the bridging logic.

Bridging Checker() -

1. Let's cut down on the mapping ibAlluoToToken because we can actually grab this token (USDC for USD for example) by querying the primaryToken of the adapter. Lower maintenance there. So if we change the primary token in the adapter for some reason, we don't need to come and reset it in the BufferManager.

2. For readability, can we extract the if condition for bridging line 133-134 into a separate view function?

3. What is the particular reason you don't put the minBridgeAmount in native token decimals and instead use wei then convert this to toekn decimals inside the condition? It makes it confusing what this minBridgeAmount should be enumerated in.

4. Overall the logic is buggy. At the moment, it will fail this case where:

   a. We need to refill the adapters but there are no pending withdrawals + there are funds in the buffermanager. b. Gelato calls checker and it says - no pending withdrawals?, then bridge this over to mainnet.

   In fact, it should also check that there are no required refills. There are multiple ways to approach this, including changing the order in which you do the checks --> do the refill check first in the loop, then the adapter pendign wtihdrawal.

5. Lines 126,127 - Don't redeclare these variables

6. Relay fee pct line 142 - please make sure that this is a uint stored inside the contract that we can change.

7. In the case that the adapter requires a refill, your gelato resolver will ignore the refill request if we do not have enough funds to fully top it up. eg. need 1000 USDC but we only have 800USDC in the buffer manager, it returns false.

   It should instead be - pull 200 usdc from gnosis and then send 800 + 200 = 1000 usdc back to the adapter - barring that we have not topped up more than the limit. More on this in my next comment about the Buffer refilling logic.

[pentatonictritones]

Buffer Refilling In General

The logic is not following the specs of the ticket correctly but here are details for each function: https://app.dework.xyz/alluo/alluo-smart-contra?taskId=0b15da6d-5a43-4384-ac1e-a655caf0bc78

1. refillBuffer()

As mentioned in point 6. of https://github.com/GetAlluo/liquidity-direction-protocol/pull/125#issuecomment-1360857863,

the refilledPerEpoch parameter should only be increased everytime we take funds from gnosis to top up the buffer.

At the moment, all it does is move funds from the adapter --> buffermanager --> adapter , which is pointless. It needs to be complemented with the gnosis sending funds to top up the buffer.

Please look at how the initial draft of the contract was written:

https://github.com/GetAlluo/liquidity-direction-protocol/pull/125/commits/d30226e5fa1d39add3aedd544a80de8f4127915a

But in particular, remember that the refilledPerEpoch should only be incremented with the funds taken from gnosis, not the funds existing in the buffer from other deposits.

2. adapterRequiredRefill()

There is a case where there is no return value : if actualAmount< expectedAmount && difference* 10000/expectedAmount <= 500.

This will cause the checker to revert.

3. confirmEpoch()

This needs to be tested. I know I wrote this as a draft but there are no tests for this, which is extremely important for this ticket.

[pentatonictritones]

Bridging + AnyCall swap()

1. Please make sure we are passing through the correct data payload that is contained in the VoteExecutorSlave.

I suppose we will need a catchup about this because a ticket has not been made for this bridging part so you might be confused with what needs to happen.

2. Can we also put in a check that there are no existing withdrawal requests and no adapters that need to be refilled?

[pentatonictritones]

More generally about testing:

There are insufficient unit and integration tests for these. As there are quite a few complex if statements, we should explore all the possible routes.

1. Check all permutations of the coniditions for which the checker() returns true for bridging:

2. Check all the permutations for which we refill the buffer and the manner in which the refilling happens (funds from buffermanager --> adapter, funds from buffer manager + gnosis -->adapter, funds only from gnosis --> adapter )

It might be helpful to extract the two gelato checker view functions and just have two separate checkers.

3. Unit tests for isAdapterPendingWithdrawals returning correct values

4. Unit tests for adapterRequiredRefill - please reference point 2. from https://github.com/GetAlluo/liquidity-direction-protocol/pull/125#issuecomment-1360864379

5. Unit tests for _confirmEpoch and the different cases for the epoch system to make sure it is working as intended.

eg. everytime time exceeds epoch length, we are correctly starting a new epoch

6. Unit tests for refillBuffer and in particular point 1 from https://github.com/GetAlluo/liquidity-direction-protocol/pull/125#issuecomment-1360864379

We can discuss integration tests later on after these are complete.

[pentatonictritones]

All comments from above, down to this one were made at commit :

32d2a6b

[pentatonictritones]

Reviewing https://github.com/GetAlluo/liquidity-direction-protocol/pull/125/commits/e96807c9345e5851c96c32bcc95ccc573e62a07d

1. VoteExecutorSlave Line 203: Please make getEntries a view function.

2. VoteExecutorSlave: Create an admin function that allows gnosis to set the entries array manually in the case of mulitchain anycall error. It will also make testing much easier.

3. Write unit tests for the two functions above.

4. BufferManager: Please refactor adapterRequiredRefill using inverted ifs.

5. BufferManager: Can you write direct unit tests for ConfirmEpoch because I wrote this epoch thing ages ago and Ididnt have a chance to test every branch.

I am still reviewing other core functionalities but will need more time for that.

Once u make changes above and finish addressing the points from the previous review, can you start deploying everything to Mumbai so that by the time I finish my review, we can easily integrate it into mumbai but upgrading the contracts.

The only thing is, please talk to Alex about contracts which are overlapping, especially the adapters. I asked him to make them upgradeable so there will be some conflict there.

[pentatonictritones]

Review as of my https://github.com/GetAlluo/liquidity-direction-protocol/pull/125/commits/f90176a43550bd7f74c4c5b5cebd6805bfe89dd6 that fixed a bug with confirmEpoch.

Generally, most comments below are about refactoring and making it more readable because it is quite confusing at the moment to review different cases myself.

Once you do this, I'll do a final check of any logic that might be buggy.

Contract:

1. In initialize, please grant GELATO role to gnosis.

2. checker() please refactor 131-132 if statements.

3. canRefill() please refactor:

   • Get rid of lines 405-408
   • Get rid of necessary if statements 410-415

4. refillBuffer()

   • Remove lines 318-320. In general you use this but there are much more easier and readable ways to do this.
   • Refactor this entire function please. There is lots of unnecessary confusing lines. In particular, work around the if else statements in 322-323 where it uses multiple return statements.

5. Please refactor canBridge() to clean up double if.

6. Please create interface files for CallProxy and ISpokePool and import them instead.

7. I'll ask you to add some events later down the line so we can keep track of things better.

Testing

1. Write unit test for speedUp.

2. Write unit tests for confirmEpoch and go through different test cases to confirm what I went through with you during the debugging session I did.

3. Test line 358.

4. Write unit test for setRelayer fee pct (just as a formality).