Related to #70, but in this case the issue is that even if the function does not params, the generate docs still add an empty line in the code snippet:

function balanceSheet(
) external returns (contract IBalanceSheetV1)

Which should instead be:

function balanceSheet() external returns (contract IBalanceSheetV1)

When only 1/3 or 2/3 of packages have been modified, the Coveralls GitHub action deletes the previous coverage report for the non-modified packages.

These functions are tested as part of the integration tests for the trade functions, but we should go the extra mile and write unit tests for them.

TODOs:

• README in @hifi/amm
• README in @hifi/constants
• README in @hifi/deployers
• README in @hifi/flash-swap
• README in @hifi/helpers
• README in @hifi/protocol
• README in @hifi/proxy-target
• Root-level README

When the pool expires, some AMM functions are not available anymore. E.g getQuoteForSellingUnderlying, for which there should be an alternative like removeLiquidityAndRedeemHToken.

As spotted by @DeFi-David in #30.

Since d90b4d5, the build step in the integration-contracts.yaml workflow is doing too much work. It's building all the TypeChain bindings across all contracts packages. It should build only the non-contracts packages.

From IBalanceSheetV1.sol#L51, we notice that RepayBorrow is defined to have payer is the first param of the event and borrower as the second. But if we go to BalanceSheetV1.sol#L628, we see that the emitted event has the params in the opposite order.
I discovered this while investigating a bug in the subgraph that was causing the event-based debt amount calculation to return negative values.

See enableTransparentWorkspaces. Without this we have to run the prepare-package task before being able to run the tests in @hifi/flash-swap and @hifi/proxy-target`.

@scorpion9979's #39 made the HifiFlashSwapUniswapV2 contract more general purpose, but it came at a cost of reducing test coverage.

Prior to the PR, we had a GodModeUniswapV2Pair contract that we were deploying via waffle.deployContract. Now we're using the original UniswapV2Pair contracts and we're deploying them via UniswapV2Factory. This makes it hard to know the order of the two pool tokens in advance (read about how ordering works here).

Two potential solutions, ordered by how good they would be:

1. Use the hardhat_setCode JSON-RPC method to deploy the tokens at known addresses.
2. Run a while loop and make new GodModeErc20 deployments until a new token ordering is found.

The 1st solution would be ideal, but hardhat_setCode has bugs: NomicFoundation/hardhat#1883. The 2nd is not nice because it involves non-determinism.

As explained in #45, the order of the tokens when deploying the UniswapV2Pair contract via UniswapV2Factory depends upon the MNEMONIC used to run the tests.

This is not great, because the tests must be aware that the USDC<>WBTC pair could be ordered in any of the two ways possible. Look at how ugly this blob of code is:

After Hardhat fixes NomicFoundation/hardhat#1883, we should switch to using their hardhat_setCode JSON-RPC method.

See the documentation.

Right now, root project scripts fail unless each package is supplied with a copy of .env. All packages should use the root .env.

Errors are currently defined in each respective package, not a package shared by all other packages.

Moving them to a single package would be beneficial due to two reasons:

1. Centralising errors would make it easier to inspect them when they occur in production.
2. In certain cases, this would DRY-ify errors. Some @hifi/protocol errors are tested for in other packages.

In HifiFlashUniswapV2.sol, there's this line asserting that the sender address is the pair contract that was initialized at the constructor. Having the pairs set at the constructor makes the flash swap contract very difficult to adapt to new listed collateral tokens. Instead, I would suggest initializing the constructor with the Uniswap V2 init hash and factory contract address (they would change depending on which chain and Uniswap clone is used). The pair contract address to be used in the assertion would then be computed from the collateral and underlying token addresses as follows:

(address token0, address token1) = sortTokens(underlying, collateral);
address pair = address(uint(keccak256(abi.encodePacked(
  hex'ff',
  factory,
  keccak256(abi.encodePacked(token0, token1)),
  initCodeHash
))));

This might be more costly in terms of gas, but it's a lot more dynamic and doesn't require any maintenance.

Specifically fix the repo URLs.

Because of two reasons:

1. Checking for {over,under}flow is cheaper.
2. Certiain operations like modulo are cheaper

Use the newly implemented getImplementationAddress function in the Hardhat plugin to read the implementation address. This is to replace the current workaround to use a locally defined storage slot:

One implication of #60 being merged is that the flash-swap contract is going to have to contain collateral token balances to insure that it would always have enough collateral to repay the 0.3% Uniswap V2 fee, especially in the case where USDC is used as collateral. Thus, we will need to add a token withdraw method in order for the contract owner to be able to withdraw tokens contained by the flash-swap contract.

After running yarn install, a node_modules folder gets generated in each contracts package. There is a symlink to that package itself:

While working on #67, I realized that there is an edge case which is currently not handled by the Uniswap v2 flash swap contract:

AssertionError: Expected transaction to be reverted with FlashUniswapV2__TurnoutNotSatisfied, but other exception was thrown: Error: VM Exception while processing transaction: reverted with panic code 0x12 (Division or modulo division by zero)

That error originates in the getRepayAmount function, when the underlyingReserves happens to match underlyingAmount. The Uniswap v2 pair contract will check for the latter to not be greater than the former, but it will allow it to pass when they are equal. That is because it is possible to have a big amount of one token and a zero amount of the other token in a Uniswap v2 pool.

I can't think off a solution off the top of my head.

The lack of NatSpec comments is causing solidity-docgen to skip the arguments in the event definitions.

Add a script to deploy HifiPoolRegistry contract.
Depends on hifi-finance/hifi-amm#19 and hifi-finance/hifi-amm#20.
Migrated from: hifi-finance/hifi-deployers#1

There are many number constants duplicated across the YieldSpace tests. We should use one of the following tools to enable shared snapshots in a declarative way:

• mocha-chai-jest-snapshot
• snap-shot-it

Currently, the project's environment variables are loaded from the .env file stored in the local project's main directory. The .env has those variables stored in plaintext format. Due to the critical nature of some of those environment variables, we need to seek a solution that provides more security. Possible options include:

• encrypted-env
• secure-env

I merged #52 but I removed the tests out of it before the merge.

The accounts in the balance sheet are collecting bond and collateral dust, even after bond maturation and flash-swap liquidation. This is due to the way flash liquidation works. The liquidator bot flash borrows USDC in order to mint the htokens used to liquidate positions. When the 18-decimal repay amount is scaled down to 6 decimals, there will be some bond dust in most cases as 12 decimal places would be cut off. In order to liquidate the entire bond, we will need to:

1. Offset the USDC amount to be borrowed by 1e-6 from the client side.
2. Enable minting slightly more htokens than needed as a result of scaling up the borrowed USDC amount. The end result is that bonds would be fully liquidated, but the flash-swap contract would accumulate htoken dust. This is a compromise we're willing to make.

Problem Statement

In the hTokenOutForUnderlyingIn and the underlyingOutForHTokenIn functions, the last operation is a subtraction:

hTokenOut = hTokenReserves - newHTokenReserves;

And:

normalizedUnderlyingOut = normalizedUnderlyingReserves - newNormalizedUnderlyingReserves;

The task is to derive a mathematical proof that it is not possible for this subtraction to revert.

Suggestions

• Hack around with Mathematica. Build on top of my set of inequalities.
• Use Echidna to fuzz the inputs and see if the fuzzer can find anything. Add an "assert" statement to let Echidna know that "hTokenReserves" must be greater than or equal to "newHTokenReserves", ditto for the other two vars.

The @actions/setup-node@v2 action is used in four jobs in the CI workflow, which causes the following error:

Unable to reserve cache with key node-cache-Linux-yarn-ed73988f6a7bd1a0270433a2d2ff89c15016643cf8f05a2fb87a270786528399, another job may be creating this cache.

All of the jobs attempt to write to the cache using the same key.

We can do this once Waffle adds support for checking against a subset of args emitted in events.

Thanks to 32dd173 and 7f042ab, the CollateralFlashUniswapV2 and UnderlyingFlashUniswapV2 have become very much alike.

By passing an additional variable to the flash swap ABI-encoded data, say "repayToken", we could potentially merge the contracts. That will remove a big chunk of technical debt.

See https://github.com/hifi-finance/hifi/actions/runs/1124159735.

The generate-docs scripts generates a pretty Markdown report that we use in our official docs. The problem is that this report needs some polishing before is easy for use.

One issue is related to the "Name" column in table under the "Returned Values" section. When the returned value does not have a name, but the NatSpec comments make a reference to it, in the table we find the first word after the @return keyword, which often is "The".

To solve this, we should add an #if statement, and check whether the returned value has a name or not.

An implication of #60 being merged is that the flash-swap contract will have to contain collateral tokens to ensure it's able to pay the 0.3% Uniswap fee in some edge cases where the entire liquidated collateral is not enough to repay the flash swap. Instead of having the flash-swap contain multiple token balances, it would be better to only contain the token shared by all pairs, which is the underlying (USDC), and use it to pay the swap fee when those edge cases are met. This would make liquidation a lot easier to manage.

It would reduce the number of contract calls the protocol admin has to undertake when initializing the protocol.

Ditto for "collateralizationRatio" and "liquidationIncentive " in "listCollateral".

Most of the yarn scripts fail unless ETHERSCAN_API_KEY was provided in .env.

It's nice for open-source projects to have a detailed CHANGELOG for tracking progress over time.

A "pow" function with a normalization factor would joggle with the numbers in such a way that it staves off overflows when the time to maturity is large.

See Yield's pow function, this PDF document:

• Long_Maturity_Overflowing_192e18.pdf

And this algorithm explanation:

Unfortunately, the script I wrote last month is still failing in certain circumstances. I will switch back to running integration on all packages, regardless of what has been modified.

I saved my work on the test/diffed-integration-contracts branch. We should revisit it at some later point.

All arguments come normalized in the functions defined in YieldSpace.sol, except for the timeToMaturity argument, which is normalized internally in the library. The style would be more consistent if we asked users to normalize all function arguments before they call any function.

Consider implementing a library for the normalize and the denormalize functions. See how I implemented them in the amm.

Modify HifiPool deployer script to call HifiPoolRegistry.trackPool(pool) as soon as the pool is deployed.
This is critical for the subgraph, as any pool events that may have been emitted before the pool was added to registry would not be captured during subgraph sync. This is outlined in The Graph docs:

Note: A new data source will only process the calls and events for the block in which it was created and all following blocks, but will not process historical data, i.e., data that is contained in prior blocks.

• @paulrberg/contracts
• mathjs (blocked by prb-math)
• prb-math
• typechain and its plugins

Upgrading from prb-math v2.4.0 to v2.4.2 is safe because there is no Solidity code change between these two versions.

But I'm not sure it would be okay to upgrade the OpenZeppelin packages since we used the current versions to ship our upgradeable proxies to production (on Polygon, that is).

That is, on line 254 in HifiPool.

This is not a priority because there isn't enough DAI or USDC in existence for a phantom overflow to be possible.

Our use of the Zero constant is not consistent. The amm packages defines zero as bn("0").

See the Hardhat guide.

The current approach is to expect the end user to know the path of the Hifi-related contract they want to interact with. Refer to the code snippets in the README to see how verbose the import path is.

It would be a much nicer API if we exposed all types and factories via an index.ts file.